Update HTMLCollection's named property getter to behave according to spec

Update HTMLCollection's named property getter to behave according to spec

Update HTMLCollection's named property getter to behave according to spec:
http://dom.spec.whatwg.org/#htmlcollection

1. namedItem() should be marked as named getter in IDL as per the spec.
   This also means we can get rid of the anonymous named getter in our
   IDL.
2. 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://dom.spec.whatwg.org/#htmlcollection
   Firefox 26 and IE11 both show the names while enumerating. However, IE11
   does not show the ids when enumerating, which is incorrect.
3. The argument to namedItem() named getter should be mandatory. This is
   consistent with the spec, Firefox 26 and IE11.

IDL interfaces inheriting HTMLCollection will need similar work (e.g.
HTMLFormControlsCollection). This will be taken care of in follow-up
patches to limit patch size and facilitate review.

R=haraken, arv, tkent
BUG=341269
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166623

[haraken, 2014-02-06 02:47:41]

LGTM

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
File Source/core/html/HTMLCollection.cpp (right):

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.cpp:499: // The supported property names are the
values from the list returned by these steps:

You can add a spec link:
http://dom.spec.whatwg.org/#htmlcollection

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.cpp:511: if (!nameAttribute.isEmpty() && (type()
!= DocAll || nameShouldBeVisibleInDocumentAll(toHTMLElement(*element)))) {

I'm just curious: Where is this condition speced?

[Inactive, 2014-02-06 02:55:39]

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
File Source/core/html/HTMLCollection.cpp (right):

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.cpp:499: // The supported property names are the
values from the list returned by these steps:
On 2014/02/06 02:47:41, haraken wrote:
> 
> You can add a spec link:
> http://dom.spec.whatwg.org/#htmlcollection

Will do.

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.cpp:511: if (!nameAttribute.isEmpty() && (type()
!= DocAll || nameShouldBeVisibleInDocumentAll(toHTMLElement(*element)))) {
On 2014/02/06 02:47:41, haraken wrote:
> 
> I'm just curious: Where is this condition speced?

Right, this is spec'd at:
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interf...

The same condition is used below in HTMLCollection::updateNameCache().

This does not have an impact yet though, HTMLAllCollection.idl needs to be
updated so that this method gets called for HTMLAllCollection as well.

[Inactive, 2014-02-06 03:01:23]

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
File Source/core/html/HTMLCollection.cpp (right):

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.cpp:511: if (!nameAttribute.isEmpty() && (type()
!= DocAll || nameShouldBeVisibleInDocumentAll(toHTMLElement(*element)))) {
On 2014/02/06 02:55:39, Chris Dumez wrote:
> On 2014/02/06 02:47:41, haraken wrote:
> > 
> > I'm just curious: Where is this condition speced?
> 
> Right, this is spec'd at:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interf...
> 
> The same condition is used below in HTMLCollection::updateNameCache().
> 
> This does not have an impact yet though, HTMLAllCollection.idl needs to be
> updated so that this method gets called for HTMLAllCollection as well.

Oops, the HTMLAllCollection spec says "with the id of an element preceding its
name if it contributes both" so this won't be sufficient. (HTMLCollection shows
the name before the id)

I will keep this change out of this change for now. For HTMLAllCollection, I
will likely override supportedPropertyNames() instead then.

[Inactive, 2014-02-06 03:03:28]

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
File Source/core/html/HTMLCollection.cpp (right):

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.cpp:499: // The supported property names are the
values from the list returned by these steps:
On 2014/02/06 02:47:41, haraken wrote:
> 
> You can add a spec link:
> http://dom.spec.whatwg.org/#htmlcollection

Done.

https://codereview.chromium.org/148323008/diff/50001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.cpp:511: if (!nameAttribute.isEmpty() && (type()
!= DocAll || nameShouldBeVisibleInDocumentAll(toHTMLElement(*element)))) {
On 2014/02/06 03:01:24, Chris Dumez wrote:
> On 2014/02/06 02:55:39, Chris Dumez wrote:
> > On 2014/02/06 02:47:41, haraken wrote:
> > > 
> > > I'm just curious: Where is this condition speced?
> > 
> > Right, this is spec'd at:
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interf...
> > 
> > The same condition is used below in HTMLCollection::updateNameCache().
> > 
> > This does not have an impact yet though, HTMLAllCollection.idl needs to be
> > updated so that this method gets called for HTMLAllCollection as well.
> 
> Oops, the HTMLAllCollection spec says "with the id of an element preceding its
> name if it contributes both" so this won't be sufficient. (HTMLCollection
shows
> the name before the id)
> 
> I will keep this change out of this change for now. For HTMLAllCollection, I
> will likely override supportedPropertyNames() instead then.

Done.

[haraken, 2014-02-06 03:30:29]

LGTM

[tkent, 2014-02-06 03:53:27]

public API change LGTM

[Inactive, 2014-02-06 04:36:27]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-06 04:36:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/148323008/130001

[commit-bot: I haz the power, 2014-02-06 06:27:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-06 06:27:28]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[Inactive, 2014-02-06 13:22:08]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-06 13:22:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/148323008/130001

[arv (Not doing code reviews), 2014-02-06 16:53:10]

LGTM

[commit-bot: I haz the power, 2014-02-06 17:37:08]

Change committed as 166623