Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(475)

Issue 147053007: Make HTMLOptionsCollection's named property getter behave more according to spec (Closed)

Created:
6 years, 10 months ago by Inactive
Modified:
6 years, 10 months ago
CC:
blink-reviews, Nils Barth (inactive), kojih, arv+blink, jsbell+bindings_chromium.org, sof, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, watchdog-blink-watchlist_google.com, Inactive
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Make HTMLOptionsCollection's named property getter behave more according to spec Make HTMLOptionsCollection's named property getter behave more according to spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#htmloptionscollection The changes are: - namedGetter() method should be marked as a named property getter in IDL. As a result, we can remove the anonymous named property getter from our IDL. - namedGetter() argument is now mandatory. This is consistent with Firefox 27 and other HTML Collections' namedGetter. The argument is optional in IE11 though. - The named getter should be enumerable as per Web IDL, meaning that elements id / names should be enumerated. The supported property names are spec'd at: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#htmloptionscollection This is consistent with the behavior of Firefox 27. In IE11, HTMLSelectElement.options seems to return an HTMLSelectElement instead of an HTMLOptionsCollection. This CL also gets rid of the custom bindings code for the named property getter as our bindings generator now has better support for returning union types. The named property getter still does not completely match the specification though (or the behavior of Firefox 27). It is supposed to always return the first matching Element. However, in case of duplicates, we currently return a NodeList type containing all matching Elements. I did not change this behavior as this was our previous behavior and IE11 also seems to return all matching Elements in this case. R=haraken, arv, tkent BUG=341269 TEST=fast/dom/htmloptionscollection-enumerated-properties.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166976

Patch Set 1 #

Patch Set 2 : Update copyright #

Patch Set 3 : Fix tiny typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -31 lines) Patch
A LayoutTests/fast/dom/htmloptionscollection-enumerated-properties.html View 1 2 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/htmloptionscollection-enumerated-properties-expected.txt View 1 chunk +13 lines, -0 lines 0 comments Download
M Source/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp View 1 chunk +0 lines, -26 lines 0 comments Download
M Source/core/html/HTMLOptionsCollection.h View 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/html/HTMLOptionsCollection.cpp View 1 4 chunks +29 lines, -1 line 0 comments Download
M Source/core/html/HTMLOptionsCollection.idl View 1 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Inactive
6 years, 10 months ago (2014-02-11 22:54:16 UTC) #1
arv (Not doing code reviews)
LGTM
6 years, 10 months ago (2014-02-11 22:59:20 UTC) #2
tkent
public API change LGTM.
6 years, 10 months ago (2014-02-12 01:03:15 UTC) #3
Inactive
The CQ bit was checked by ch.dumez@samsung.com
6 years, 10 months ago (2014-02-12 01:15:05 UTC) #4
haraken
LGTM
6 years, 10 months ago (2014-02-12 01:17:39 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/147053007/70001
6 years, 10 months ago (2014-02-12 01:59:19 UTC) #6
commit-bot: I haz the power
6 years, 10 months ago (2014-02-12 07:49:16 UTC) #7
Message was sent while issue was closed.
Change committed as 166976

Powered by Google App Engine
This is Rietveld 408576698