772869 - getOwnPropertyNames for proxy-based DOM list bindings should add names for named items

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make getOwnPropertyNames work correctly for WebIDL proxy bindings.

Ms2ger, can you please look over the imptests change?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: With the lone mochitest unexpected pass fixed

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 673223 [details] [diff] [review]
With the lone mochitest unexpected pass fixed

Review of attachment 673223 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLPropertiesCollection.cpp
@@ +370,5 @@
>        } else {          
>          aContent = element->GetNextNode(aElement);
>        }                 
>      }                     
>    }                    

I wouldn't mind if you removed all this trailing whitespace :)

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +2569,5 @@
> +nsFormControlList::GetSupportedNames(nsTArray<nsString>& aNames)
> +{
> +  FlushPendingNotifications();
> +  // Just enumerate mNameLookupTable.  This won't guarantee order, but
> +  // that's OK.

Hm. WebIDL does define an order...

::: dom/imptests/html/tests/submission/Opera/microdata/test_001.html
@@ +1530,5 @@
>  }, 'properties.item must give each property in tree order');
>  test(function () {
>  	var testEl = makeEl('div',{itemscope:'itemscope'},'<div itemprop="foo"></div><div itemprop="bar"><div itemprop="foo"></div></div><div itemprop="baz qux"></div>');
> +	testEl.properties.something = "another";
> +	names = Object.getOwnPropertyNames(testEl.properties);

`var` or inline it in the assert_array_equals call.

@@ +1532,5 @@
>  	var testEl = makeEl('div',{itemscope:'itemscope'},'<div itemprop="foo"></div><div itemprop="bar"><div itemprop="foo"></div></div><div itemprop="baz qux"></div>');
> +	testEl.properties.something = "another";
> +	names = Object.getOwnPropertyNames(testEl.properties);
> +        assert_array_equals(names, ["0", "1", "2", "3", "foo", "bar", "baz",
> +                                    "qux", "something"]);

Code style here appears to have spaces after ( and before ), as hideous as that may be.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Hm. WebIDL does define an order...

WebIDL says this:

  If the object supports named properties, then the object’s supported property names are
  enumerated next, in the order given in the definition of the set of supported property
  names.

And the HTML5 spec says:

  The supported property names consist of the values of all the id and name attributes of
  all the elements represented by the collection.

That's it.  So there is no order defined in this case.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I wouldn't mind if you removed all this trailing whitespace :)

I would, so no.  ;)

I made the test changes you asked for.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 673223 [details] [diff] [review]
With the lone mochitest unexpected pass fixed

Review of attachment 673223 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsContentList.cpp
@@ +647,5 @@
> +    nsGenericHTMLElement* el = nsGenericHTMLElement::FromContent(content);
> +    if (el) {
> +      // XXXbz should we be checking for particular tags here?  How
> +      // stable is this part of the spec?
> +      // Note: HasName means the names is exposed on the document,

nsINode::HasName()? Also, 'the names is exposed' sounds wrong to me.

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +2569,5 @@
> +nsFormControlList::GetSupportedNames(nsTArray<nsString>& aNames)
> +{
> +  FlushPendingNotifications();
> +  // Just enumerate mNameLookupTable.  This won't guarantee order, but
> +  // that's OK.

WebIDL does talk about an order, but I don't see where this one would be defined.

::: content/html/content/src/nsHTMLTableElement.cpp
@@ +307,5 @@
> +    if (coll) {
> +      coll->GetSupportedNames(names);
> +      for (uint32_t i = 0; i < names.Length(); ++i) {
> +        if (!aNames.Contains(names[i])) {
> +          aNames.AppendElement(names[i]);

Given this code it might make sense to have the virtual GetSupportedNames take a |nsTArray<nsIAtom>&| and have the one taking |nsTArray<nsString>& aNames| call that and then copy? You'd still call the virtual one here but wouldn't need to check with Contains and copy here.
I wish we could use the nsTArray<nsIAtom> directly to convert to the jsid array, but it seems it'll make things a bit complicated.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> nsINode::HasName()? Also, 'the names is exposed' sounds wrong to me.

Fixed, on both counts.

> WebIDL does talk about an order, but I don't see where this one would be defined.

WebIDL doesn't leaves the definition of the order to the spec defining the interface.  In this case that spec does not define an order.  See comment 4.  I fixed the comment to make that clearer.

> You'd still call the virtual one here but wouldn't need to check with Contains and copy
> here.

I still would...  The problem is what happens when I have elements with the same id in different row groups.  I want to only end up with that name in the output array once, no?  Hence the Contains() checks.

Using nsIAtom would make the Contains() call faster, though.  I guess I can do that if you'd prefer.

Note that there are no string copies going on here, by the way.  Just refcounting of stringbuffers.  I agree that refcounting atoms would be faster, though.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, but we wouldn't need to refcount the atoms, since we'd just turn them into strings.

OK, going to do what you suggested.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hrm.  Except form.elements doesn't have the names in atomized form.  Nor does the HTMLPropertiesCollection.

So I'll leave it as-is for now.  :(

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bb44a0caae4

I filed bug 808606 as a followup.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9bb44a0caae4