[CLEANUP] Add CssInliner::querySelectorAll method
#1316
Conversation
That will never happen unless there is a bug in |
JakeQZ
force-pushed
the
cleanup/query-selector-all
branch
from
5c03b33
to
a809329
Compare
|
Can improve this. One instance that could call the new method is missed. And excpetion handling logic (and/or its definition) can be made more precise. |
oliverklee
changed the title
CssInliner::querySelectorAll method
|
Will be ready for review when I repush. I'm marking as ready for review now, to see if we get a Coveralls comment on repush when it's not in draft status. |
JakeQZ
force-pushed
the
cleanup/query-selector-all
branch
3 times, most recently
from
2b5b3f0
to
dbf26c9
Compare
This `private` method provides functionality that is used in three separate places (albeit a one-liner), and includes taking care of exception handling according to the debug mode setting. The new method and parameter names match the equivalent in the [Web API]( https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll ). There will be a tiny performance impact due to additional method calls. But the benefits include code re-use and common error handling. Also add `ensureNodeIsElement` method for use with the result from `querySelectorAll` to ensure type safety. `querySelectorAll` should always return a `DOMNodeList` of `DOMElement`s. But this cannot be guaranteed if there is a bug in CssSelector whereby it returns an XPath expression which may match non-element nodes. And it would (probably) be sub-optimal for `querySelectorAll` to have an extra loop checking this, when it can be done in the caller's loop over the returned data.
JakeQZ
force-pushed
the
cleanup/query-selector-all
branch
from
dbf26c9
to
1b425c8
Compare
Oh. I've just noticed that Coveralls updates its comment, instead of providing a new one. Surprisingly, coverage has now increased, despite some of the newly-added exception paths not being exercised. Though I suppose the old exception paths weren't being exercised either, and there were more of them, as this PR also now commons-up some of the exception handling logic. Anyway, it's ready for review now.. |
Yeah. I had the same facial expression some time ago when I noticed the same thing … ;-) |
This
privatemethod provides functionality that is used in three separate places (albeit a one-liner), and includes taking care of exception handling according to the debug mode setting.The new method and parameter names match the equivalent in the Web API.
There will be a tiny performance impact due to additional method calls. But the benefits include code re-use and common error handling.
Also add
ensureNodeIsElementmethod for use with the result fromquerySelectorAllto ensure type safety.querySelectorAllshould always return aDOMNodeListofDOMElements. But this cannot be guaranteed if there is a bug in CssSelector whereby it returns an XPath expression which may match non-element nodes. And it would (probably) be sub-optimal forquerySelectorAllto have an extra loop checking this, when it can be done in the caller's loop over the returned data.