Bug 148117

Created attachment 259699 [details] Patch

Created attachment 259700 [details] Patch

Comment on attachment 259700 [details] Patch Attachment 259700 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/88405 New failing tests: fast/dom/named-items-with-symbol-name.html

Created attachment 259701 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 259700 [details] Patch Attachment 259700 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/88409 New failing tests: fast/dom/named-items-with-symbol-name.html

Created attachment 259702 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Created attachment 259703 [details] Patch

Comment on attachment 259703 [details] Patch Attachment 259703 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/88543 New failing tests: fast/dom/named-items-with-symbol-name.html

Created attachment 259707 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Created attachment 259708 [details] Patch

Comment on attachment 259708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259708&action=review > LayoutTests/fast/dom/named-items-with-symbol-name.html:19 > debug("\n" + String(getElementsByTagNameList)); Really should remove this unneeded newline.

Comment on attachment 259708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259708&action=review > Source/WebCore/ChangeLog:21 > + This change could be a bit risky but: > + 1. Firefox and IE never had this named property getter on NodeList. > + 2. Chrome successfuly dropped this named property getter in early 2014: > + https://src.chromium.org/viewvc/blink?revision=166356&view=revision > + 3. Our named property getter on NodeList is only partly functional: > + It only matches by 'id' attribute, not by 'name' attribute. > + 4. Our APIs that were wrongly returning a NodeList instead of an > + HTMLCollection (getElementsByTagName() / getElementsByClassName()) > + have been fixed in r188735 and r188809. HTMLCollection does have > + a named property getter. > + 5. This named getter adds code complexity. A few other thoughts, but I don’t think I should remove the commit queue flag. None of these fully address the issue of non-live-website content relying on this by accident. The change could break, say, iPhone apps that used this. I wonder how we can mitigate that risk? Items 3 and 5 are ways in which this patch improves things, but they are not relevant to risk, so I’m not sure they belong on this list. Spelling error: "successfully" is correct, "successfully" is wrong.

Comment on attachment 259708 [details] Patch Clearing flags on attachment: 259708 Committed r188829: <http://trac.webkit.org/changeset/188829>

All reviewed patches have been landed. Closing bug.

(In reply to comment #11) > Comment on attachment 259708 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259708&action=review > > > LayoutTests/fast/dom/named-items-with-symbol-name.html:19 > > debug("\n" + String(getElementsByTagNameList)); > > Really should remove this unneeded newline. Done in <http://trac.webkit.org/changeset/188863>.

Should we add a test that verifies the absence of the name getter?

(In reply to comment #16) > Should we add a test that verifies the absence of the name getter? I wasn't sure it was needed. But since you're suggesting it, I'll add one :)

Reopening to attach new patch.

Created attachment 259758 [details] Test

(In reply to comment #16) > Should we add a test that verifies the absence of the name getter? Just uploaded a patch that adds such test.

Comment on attachment 259758 [details] Test r=me It's a bit strange to test for something *not* being there, but I think it's good practice in order to confirm the difference before and after the code change.

Comment on attachment 259758 [details] Test Clearing flags on attachment: 259758 Committed r188885: <http://trac.webkit.org/changeset/188885>

All reviewed patches have been landed. Closing bug.

According to the perf bots, this was a ~9% progression on Dromaeo DOM Query subtest and ~3.8% overall on Dromaeo DOM Core.

(In reply to comment #24) > According to the perf bots, this was a ~9% progression on Dromaeo DOM Query > subtest and ~3.8% overall on Dromaeo DOM Core. Nice!