Bug 148117 - NodeList should not have a named getter
Summary: NodeList should not have a named getter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://dom.spec.whatwg.org/#interfac...
Keywords: WebExposed
Depends on: 110611 147980
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-17 20:12 PDT by Chris Dumez
Modified: 2015-09-23 19:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (13.19 KB, patch)
2015-08-21 19:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.21 KB, patch)
2015-08-21 19:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (537.86 KB, application/zip)
2015-08-21 20:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (674.67 KB, application/zip)
2015-08-21 20:15 PDT, Build Bot
no flags Details
Patch (15.13 KB, patch)
2015-08-21 20:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (537.14 KB, application/zip)
2015-08-21 21:04 PDT, Build Bot
no flags Details
Patch (15.07 KB, patch)
2015-08-21 21:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Test (2.42 KB, patch)
2015-08-24 11:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-08-17 20:12:06 PDT
NodeList should not have a named getter. We currently have one because several of our Web APIs (getElementsByTagName / getElementsByClassName) return a NodeList instead of an HTMLCollection.
One we fix those APIs, we may be able to drop the named getter on NodeList.
Comment 1 Chris Dumez 2015-08-21 19:31:41 PDT
Created attachment 259699 [details]
Patch
Comment 2 Chris Dumez 2015-08-21 19:37:16 PDT
Created attachment 259700 [details]
Patch
Comment 3 Build Bot 2015-08-21 20:11:22 PDT
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
Comment 4 Build Bot 2015-08-21 20:11:25 PDT
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 5 Build Bot 2015-08-21 20:15:55 PDT
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
Comment 6 Build Bot 2015-08-21 20:15:58 PDT
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
Comment 7 Chris Dumez 2015-08-21 20:29:45 PDT
Created attachment 259703 [details]
Patch
Comment 8 Build Bot 2015-08-21 21:04:22 PDT
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
Comment 9 Build Bot 2015-08-21 21:04:25 PDT
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
Comment 10 Chris Dumez 2015-08-21 21:05:59 PDT
Created attachment 259708 [details]
Patch
Comment 11 Darin Adler 2015-08-22 14:42:55 PDT
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 12 Darin Adler 2015-08-22 14:45:38 PDT
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 13 WebKit Commit Bot 2015-08-22 15:29:38 PDT
Comment on attachment 259708 [details]
Patch

Clearing flags on attachment: 259708

Committed r188829: <http://trac.webkit.org/changeset/188829>
Comment 14 WebKit Commit Bot 2015-08-22 15:29:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Chris Dumez 2015-08-24 10:02:50 PDT
(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>.
Comment 16 Geoffrey Garen 2015-08-24 10:43:24 PDT
Should we add a test that verifies the absence of the name getter?
Comment 17 Chris Dumez 2015-08-24 10:44:19 PDT
(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 :)
Comment 18 Chris Dumez 2015-08-24 11:03:06 PDT
Reopening to attach new patch.
Comment 19 Chris Dumez 2015-08-24 11:03:11 PDT
Created attachment 259758 [details]
Test
Comment 20 Chris Dumez 2015-08-24 11:03:47 PDT
(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 21 Geoffrey Garen 2015-08-24 13:59:52 PDT
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 22 WebKit Commit Bot 2015-08-24 14:44:46 PDT
Comment on attachment 259758 [details]
Test

Clearing flags on attachment: 259758

Committed r188885: <http://trac.webkit.org/changeset/188885>
Comment 23 WebKit Commit Bot 2015-08-24 14:44:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Chris Dumez 2015-09-23 19:20:34 PDT
According to the perf bots, this was a ~9% progression on Dromaeo DOM Query subtest and ~3.8% overall on Dromaeo DOM Core.
Comment 25 Ryosuke Niwa 2015-09-23 19:23:43 PDT
(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!