Bug 16478 – Redundant isElement checks in NodeLists

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Smith]

A small (0.6-1.6% on one test I made) speedup for getElementsByClassName is possible by removing what appears to be a redundant check of testNode->isElement in nodeMatches. A similar check exists in TagNodeList, but not other subclasses of NodeList. Searching for nodeMatches project-wide in WebCore turns up four callsites, three of which have their own isElement checks, and the last of which uses getElementByID to get the node. olliej suggested putting an ASSERT in instead of a check, so as to make sure that no future callsites accidentally leave out the isElement check.

[David Smith]

Created attachment 17966 [details]
Patch. Passes run-webkit-tests

[Darin Adler]

Comment on attachment 17966 [details]
Patch. Passes run-webkit-tests

If the parameters are really guaranteed to be Element, then the function's signature needs to change to Element* -- the patch is fine, but doesn't go far enough.

[David Smith]

(In reply to comment #2)
> (From update of attachment 17966 [details] [edit])
> If the parameters are really guaranteed to be Element, then the function's
> signature needs to change to Element* -- the patch is fine, but doesn't go far
> enough.
> 

It seemed odd/bad to me to be declaring stuff that takes an Element* in Node.h. Is this not a problem?

[Eric Seidel (no email)]

Comment on attachment 17966 [details]
Patch. Passes run-webkit-tests

It makes little sense to me to have an ASSERT for a specific subclass when the method signature expects the base class.  The better way to change this is to change the message signature, and then the callers to static_cast when necessary.

[David Smith]

This was fixed at some point.