Bug 115584

Summary: Unify ways to cache named item in HTMLCollections
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, graouts, kling, koivisto, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 117836    
Attachments:
Description Flags
Work in progress
none
Work in progress 2
none
Work in progress 3
none
Cleanup
koivisto: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 none

Description Ryosuke Niwa 2013-05-04 00:01:48 PDT
Right now, HTMLCollection uses TreeScope's DocumentOrderedMap for id-attribute names and manual tree traversal for name-attribute names. In addition HTMLNameCollection has a special hash counted set on HTMLDocument as a fast path. We need to unify all these cases.
Comment 1 Ryosuke Niwa 2013-05-04 00:02:27 PDT
Created attachment 200518 [details]
Work in progress
Comment 2 Ryosuke Niwa 2013-05-04 12:57:32 PDT
Created attachment 200538 [details]
Work in progress 2
Comment 3 Radar WebKit Bug Importer 2013-05-06 11:59:29 PDT
<rdar://problem/13818072>
Comment 4 Ryosuke Niwa 2013-05-06 12:25:57 PDT
Created attachment 200766 [details]
Work in progress 3
Comment 5 Ryosuke Niwa 2013-05-06 13:14:28 PDT
Created attachment 200771 [details]
Cleanup
Comment 6 Build Bot 2013-05-06 14:31:36 PDT
Comment on attachment 200771 [details]
Cleanup

Attachment 200771 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/377274

New failing tests:
css3/filters/custom/filter-fallback-to-software.html
Comment 7 Build Bot 2013-05-06 14:31:37 PDT
Created attachment 200815 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 8 Antti Koivisto 2013-05-06 14:45:55 PDT
Comment on attachment 200771 [details]
Cleanup

r=me
Comment 9 Ryosuke Niwa 2013-05-06 17:19:13 PDT
Committed r149652: <http://trac.webkit.org/changeset/149652>
Comment 10 Darin Adler 2013-05-06 17:24:49 PDT
Comment on attachment 200771 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=200771&action=review

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:110
> +    return toJS(exec, thisObj->globalObject(), WTF::getPtr(collection));

It’s fine to use getPtr in automatically generated code where we need to be generic, but in handwritten code we should just call collection.get().
Comment 11 Antoine Quint 2013-06-20 10:46:07 PDT
This changed caused http://webkit.org/b/117836.
Comment 12 Adam Barth 2013-08-02 15:32:26 PDT
When I merged this patch to Blink, I found that it broke the behavior below.  I haven't checked whether WebKit had the same issue.

$ cat LayoutTests/fast/dom/window-polluter-tricky.html 
<script src="../js/resources/js-test-pre.js"></script>
<form name="Taco" id="Taco"></form>
<applet name="Bravo" id="Bravo"></applet>
<script>
shouldBe("window.Taco", "document.getElementById('Taco')");
shouldBe("window.Bravo", "document.getElementById('Bravo')");

shouldBe("document.Taco", "document.getElementById('Taco')");
shouldBe("document.Bravo", "document.getElementById('Bravo')");
</script>
<script src="../js/resources/js-test-post.js"></script>
Comment 13 Ryosuke Niwa 2013-08-02 15:48:35 PDT
(In reply to comment #12)
> When I merged this patch to Blink, I found that it broke the behavior below.  I haven't checked whether WebKit had the same issue.

This patch has a lot of dependencies to my other refactoring patches so maybe you're missing some of that. FWIW, I can't reproduce the bug on ToT WebKit.
Comment 14 Adam Barth 2013-08-02 16:06:40 PDT
> This patch has a lot of dependencies to my other refactoring patches so maybe you're missing some of that.

Yes, merging it wasn't trivial.  :)

> FWIW, I can't reproduce the bug on ToT WebKit.

That's good to hear.  Looks like it was probably fixed in http://trac.webkit.org/changeset/150187.  You might want to land the test in any case so that you don't regress the behavior in the future.