Bug 20176 - querySelectorAll id optimization no longer working
Summary: querySelectorAll id optimization no longer working
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: David Smith
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-25 17:00 PDT by David Smith
Modified: 2008-07-27 15:06 PDT (History)
0 users

See Also:

Attachments
Patch; Passes run-webkit-tests and doesn't regress http://ejohn.org/apps/selectortest/#target (2.40 KB, patch)
2008-07-25 17:06 PDT, David Smith 		sam: review-
Details | Formatted Diff | Diff
patch (12.22 KB, patch)
2008-07-27 14:55 PDT, Sam Weinig 		oliver: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 2008-07-25 17:00:02 PDT 
Regresses the time for the #speech5 test on webkit.org/perf/slickspeed from 1-3ms to 10-12+ ms.
Comment 1 David Smith 2008-07-25 17:06:07 PDT 
Created attachment 22488 [details]
Patch; Passes run-webkit-tests and doesn't regress http://ejohn.org/apps/selectortest/#target
Comment 2 Eric Seidel (no email) 2008-07-26 22:41:41 PDT 
Comment on attachment 22488 [details]
Patch; Passes run-webkit-tests and doesn't regress http://ejohn.org/apps/selectortest/#target

Looks good.
Comment 3 Sam Weinig 2008-07-26 22:51:03 PDT 
Comment on attachment 22488 [details]
Patch; Passes run-webkit-tests and doesn't regress http://ejohn.org/apps/selectortest/#target

The check shouldn't be necessary, and can be made an ASSERT as far as I can tell.  It should also read querySelector->m_attr== idAttr, not querySelector->m_attr.localName() == idAttr.

I believe this will also break some uses of querySelector("#camalCase"); in quirks mode.  We probably need to disable the optimization in quirks mode methinks.
Comment 4 Sam Weinig 2008-07-27 14:55:26 PDT 
Created attachment 22511 [details]
patch
Comment 5 Sam Weinig 2008-07-27 15:06:39 PDT 
Fixed in r35406.