Bug 115584 - Unify ways to cache named item in HTMLCollections
Summary: Unify ways to cache named item in HTMLCollections
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 117836
  Show dependency treegraph
 
Reported: 2013-05-04 00:01 PDT by Ryosuke Niwa
Modified: 2013-08-02 16:06 PDT (History)
7 users (show)

See Also:


Attachments
Work in progress (39.57 KB, patch)
2013-05-04 00:02 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Work in progress 2 (42.15 KB, patch)
2013-05-04 12:57 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Work in progress 3 (43.62 KB, patch)
2013-05-06 12:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Cleanup (52.86 KB, patch)
2013-05-06 13:14 PDT, Ryosuke Niwa
koivisto: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (578.85 KB, application/zip)
2013-05-06 14:31 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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.