Make HTMLAllCollection named property getter behave more according to spec

Make HTMLAllCollection named property getter behave more according to spec

Make HTMLAllCollection named property getter behave more according to spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#htmlallcollection-0

The following changes were made:
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://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#htmlallcollection-0
   This change is also consistent with IE11. Mozilla does not seem to
   support enumerating document.all?
3. The named property getter returns more types of elements by name (a, area,
   frame, frameset, iframe). Its still returns select and input elements as
   well, even though this is against spec, for backward compatibility.
   This behavior is consistent with IE11 and Firefox 27.

This CL also gets rid of the custom bindings for namedItem() now that the
bindings generator has better support for returning union types.

R=haraken, arv, tkent
BUG=341269
TEST=fast/dom/htmlallcollection-enumerated-properties.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167126

[Inactive, 2014-02-11 20:36:00]

https://codereview.chromium.org/158563004/diff/130001/LayoutTests/fast/dom/ht...
File LayoutTests/fast/dom/htmlallcollection-enumerated-properties-expected.txt
(right):

https://codereview.chromium.org/158563004/diff/130001/LayoutTests/fast/dom/ht...
LayoutTests/fast/dom/htmlallcollection-enumerated-properties-expected.txt:7:
FAIL htmlAllCollection.__proto__.__proto__ should be [object Object]. Was
[object Object].
We don't follow the spec here.

[arv (Not doing code reviews), 2014-02-11 20:43:35]

FYI: Mozilla does support document.all


On Tue, Feb 11, 2014 at 3:36 PM, <ch.dumez@samsung.com> wrote:

> Reviewers: arv, haraken, tkent,
>
>
> https://codereview.chromium.org/158563004/diff/130001/
> LayoutTests/fast/dom/htmlallcollection-enumerated-properties-expected.txt
> File
> LayoutTests/fast/dom/htmlallcollection-enumerated-properties-expected.txt
> (right):
>
> https://codereview.chromium.org/158563004/diff/130001/
> LayoutTests/fast/dom/htmlallcollection-enumerated-properties-expected.txt#
> newcode7
> LayoutTests/fast/dom/htmlallcollection-enumerated-
> properties-expected.txt:7:
> FAIL htmlAllCollection.__proto__.__proto__ should be [object Object].
> Was [object Object].
> We don't follow the spec here.
>
> Description:
> Make HTMLAllCollection named property getter behave more according to spec
>
> Make HTMLAllCollection named property getter behave more according to spec:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-
> interfaces.html#htmlallcollection-0
>
> The following changes were made:
> 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://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-
> interfaces.html#htmlallcollection-0
>    This change is also consistent with IE11. Mozilla does not support
>    document.all.
> 3. The named property getter returns more types of elements by name (a,
> area,
>    frame, frameset, iframe). Its still returns select and input elements as
>    well, even though this is against spec, for backward compatibility.
>    This behavior is consistent with IE11.
>
> This CL also gets rid of the custom bindings for namedItem() now that the
> bindings generator has better support for returning union types.
>
> R=haraken, arv, tkent
> BUG=341269
> TEST=fast/dom/htmlallcollection-enumerated-properties.html
>
> Please review this at https://codereview.chromium.org/158563004/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+128, -35 lines):
>   A LayoutTests/fast/dom/htmlallcollection-enumerated-
> properties-expected.txt
>   A LayoutTests/fast/dom/htmlallcollection-enumerated-properties.html
>   M LayoutTests/inspector/console/console-format-collections-expected.txt
>   M Source/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp
>   M Source/core/html/HTMLAllCollection.h
>   M Source/core/html/HTMLAllCollection.cpp
>   M Source/core/html/HTMLAllCollection.idl
>   M Source/core/html/HTMLCollection.h
>   M Source/core/html/HTMLCollection.cpp
>
>
>


-- 
erik

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[arv (Not doing code reviews), 2014-02-11 20:48:41]

LGTM

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:65: || element.hasLocalName(inputTag) //
FIXME: Not according to spec.
If IE11 behaves this way we should get the spec changed.

[Inactive, 2014-02-11 20:50:36]

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:65: || element.hasLocalName(inputTag) //
FIXME: Not according to spec.
On 2014/02/11 20:48:42, arv wrote:
> If IE11 behaves this way we should get the spec changed.

Yes, based on my testing IE11 return the names of input and select elements.

[Inactive, 2014-02-11 20:55:09]

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:65: || element.hasLocalName(inputTag) //
FIXME: Not according to spec.
On 2014/02/11 20:50:37, Chris Dumez wrote:
> On 2014/02/11 20:48:42, arv wrote:
> > If IE11 behaves this way we should get the spec changed.
> 
> Yes, based on my testing IE11 return the names of input and select elements.

http://jsfiddle.net/jAx2t/3/

[Inactive, 2014-02-11 21:02:38]

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:65: || element.hasLocalName(inputTag) //
FIXME: Not according to spec.
On 2014/02/11 20:55:10, Chris Dumez wrote:
> On 2014/02/11 20:50:37, Chris Dumez wrote:
> > On 2014/02/11 20:48:42, arv wrote:
> > > If IE11 behaves this way we should get the spec changed.
> > 
> > Yes, based on my testing IE11 return the names of input and select elements.
> 
> http://jsfiddle.net/jAx2t/3/

I filed a bug against the spec:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24622

[Inactive, 2014-02-11 21:08:31]

I don't quite understand Firefox 27's behavior:
http://jsfiddle.net/jAx2t/5/

When I load this is Firefox 27, document.all is undefined and we cannot
enumerate document.all. This led me to believe that Mozilla did not support
document.all.
However, document.all.inputName1 seems to return the correct value...

[arv (Not doing code reviews), 2014-02-11 21:28:43]

On 2014/02/11 21:08:31, Chris Dumez wrote:
> I don't quite understand Firefox 27's behavior:
> http://jsfiddle.net/jAx2t/5/
> 
> When I load this is Firefox 27, document.all is undefined and we cannot
> enumerate document.all. This led me to believe that Mozilla did not support
> document.all.
> However, document.all.inputName1 seems to return the correct value...

document.all is just crazy like that.

[haraken, 2014-02-12 01:23:00]

LGTM except for arv's concern about the spec. Probably we can exclude the
controversial part from this CL.

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:79: for (Element* element =
traverseToFirstElement(root); element; element = traverseNextElement(*element,
root)) {

We're duplicating this code in several files. It seems that the difference
between the duplicated code is just a condition in line 81 and 89. Probably we
can define a method for the condition instead of duplicating the entire loop.

You can do it in a follow-up if you agree with the idea.

[Inactive, 2014-02-12 01:26:19]

On 2014/02/12 01:23:00, haraken wrote:
> LGTM except for arv's concern about the spec. Probably we can exclude the
> controversial part from this CL.

There is no controversial part in this CL as far as I know. I kept backward
compatibility. Arv's request was about trying to get the spec updated to match
our behavior (return select and input elements by name as well) as this is also
the behavior of other browsers.

[Inactive, 2014-02-12 01:31:05]

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:79: for (Element* element =
traverseToFirstElement(root); element; element = traverseNextElement(*element,
root)) {
On 2014/02/12 01:23:01, haraken wrote:
> 
> We're duplicating this code in several files. It seems that the difference
> between the duplicated code is just a condition in line 81 and 89. Probably we
> can define a method for the condition instead of duplicating the entire loop.
> 
> You can do it in a follow-up if you agree with the idea.

If it was just about the condition, I would have simply added it to the
HTMLCollection's implementation (this was my first attempt if you remember).
However, the order of id / name is also reversed. I am trying to get the DOM
spec fixed so that the id / name order is consistent between HTMLCollection and
its subclasses:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24624

If this goes through, I will refactor the code to avoid a lot of the
duplication. If you insist, I can also try and get HTMLCollection's
supportedPropertyNames() to work for HTMLAllCollection in this CL already.

[Inactive, 2014-02-12 01:32:36]

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:65: || element.hasLocalName(inputTag) //
FIXME: Not according to spec.
On 2014/02/11 20:48:42, arv wrote:
> If IE11 behaves this way we should get the spec changed.

Would you like me to remove those 2 FIXME comments as we behave the same way as
other browsers and probably don't want to "fix" this (but fix the spec instead).

[haraken, 2014-02-12 01:34:32]

On 2014/02/12 01:31:05, Chris Dumez wrote:
>
https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
> File Source/core/html/HTMLAllCollection.cpp (right):
> 
>
https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
> Source/core/html/HTMLAllCollection.cpp:79: for (Element* element =
> traverseToFirstElement(root); element; element = traverseNextElement(*element,
> root)) {
> On 2014/02/12 01:23:01, haraken wrote:
> > 
> > We're duplicating this code in several files. It seems that the difference
> > between the duplicated code is just a condition in line 81 and 89. Probably
we
> > can define a method for the condition instead of duplicating the entire
loop.
> > 
> > You can do it in a follow-up if you agree with the idea.
> 
> If it was just about the condition, I would have simply added it to the
> HTMLCollection's implementation (this was my first attempt if you remember).
> However, the order of id / name is also reversed. I am trying to get the DOM
> spec fixed so that the id / name order is consistent between HTMLCollection
and
> its subclasses:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24624

Thanks, makes sense. Then it sounds reasonable to duplicate the code rather than
doing something more complicated to share the code.

[Inactive, 2014-02-12 16:28:41]

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
File Source/core/html/HTMLAllCollection.cpp (right):

https://codereview.chromium.org/158563004/diff/170001/Source/core/html/HTMLAl...
Source/core/html/HTMLAllCollection.cpp:79: for (Element* element =
traverseToFirstElement(root); element; element = traverseNextElement(*element,
root)) {
On 2014/02/12 01:31:05, Chris Dumez wrote:
> On 2014/02/12 01:23:01, haraken wrote:
> > 
> > We're duplicating this code in several files. It seems that the difference
> > between the duplicated code is just a condition in line 81 and 89. Probably
we
> > can define a method for the condition instead of duplicating the entire
loop.
> > 
> > You can do it in a follow-up if you agree with the idea.
> 
> If it was just about the condition, I would have simply added it to the
> HTMLCollection's implementation (this was my first attempt if you remember).
> However, the order of id / name is also reversed. I am trying to get the DOM
> spec fixed so that the id / name order is consistent between HTMLCollection
and
> its subclasses:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24624
> 
> If this goes through, I will refactor the code to avoid a lot of the
> duplication. If you insist, I can also try and get HTMLCollection's
> supportedPropertyNames() to work for HTMLAllCollection in this CL already.

I am updating the id/name order in HTMLCollection with
https://codereview.chromium.org/157813013/ (the DOM spec was just updated). Once
that other CL lands, I will rebase this patch and get rid of the code
duplication.

[Inactive, 2014-02-13 18:50:44]

Now that I landed my patch to fix the properties order on HTMLCollection, I have
rebased this patch so that we no longer override supportedPropertyNames() and no
longer duplicate the code. Please take another look.

[arv (Not doing code reviews), 2014-02-13 18:57:01]

LGTM

[Inactive, 2014-02-13 19:01:46]

On 2014/02/13 18:57:01, arv wrote:
> LGTM

Thanks arv.

tkent or jochen, could I please get API owner stamping? :)

[tkent, 2014-02-13 23:02:51]

public API change lgtm

[tkent, 2014-02-13 23:02:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:03:06]

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

[commit-bot: I haz the power, 2014-02-14 00:09:01]

Change committed as 167126