NodeList should not have a named getter

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 259699 [details]
Patch

[Chris Dumez]

Created attachment 259700 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

Created attachment 259703 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

Created attachment 259708 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 259708 [details]
Patch

Clearing flags on attachment: 259708

Committed r188829: <http://trac.webkit.org/changeset/188829>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(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>.

[Geoffrey Garen]

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

[Chris Dumez]

(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 :)

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 259758 [details]
Test

[Chris Dumez]

(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.

[Geoffrey Garen]

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.

[WebKit Commit Bot]

Comment on attachment 259758 [details]
Test

Clearing flags on attachment: 259758

Committed r188885: <http://trac.webkit.org/changeset/188885>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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

[Ryosuke Niwa]

(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!