Bug 16478 - Redundant isElement checks in NodeLists
Summary: Redundant isElement checks in NodeLists
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Trivial
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-12-17 11:00 PST by David Smith
Modified: 2008-10-23 01:15 PDT (History)
0 users

See Also:


Attachments
Patch. Passes run-webkit-tests (2.70 KB, patch)
2007-12-17 11:03 PST, David Smith
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 2007-12-17 11:00:58 PST
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.
Comment 1 David Smith 2007-12-17 11:03:10 PST
Created attachment 17966 [details]
Patch. Passes run-webkit-tests
Comment 2 Darin Adler 2007-12-17 11:33:27 PST
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.
Comment 3 David Smith 2007-12-17 13:19:15 PST
(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?
Comment 4 Eric Seidel (no email) 2007-12-17 14:22:26 PST
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.
Comment 5 David Smith 2008-10-23 01:15:30 PDT
This was fixed at some point.