Bug 149562 – Object.getOwnPropertyNames() does not return named properties

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Property enumeration does not enumerate named properties for interfaces with named property getters such as HTMLCollection.

This is documented by Web IDL at:
https://heycam.github.io/webidl/#property-enumeration

The exact order of the named properties for HTMLCollection is defined at https://dom.spec.whatwg.org/#interface-htmlcollection:
"""
The supported property names, all unenumerable, are the values from the list returned by these steps:

Let result be an empty list.
For each element represented by the collection, in tree order, run these substeps:
If element has an ID which is neither the empty string nor is in result, append element’s ID to result.
If element is in the HTML namespace and has a name attribute whose value is neither the empty string nor is in result, append element’s name attribute value to result.
Return result.
"""

[Chris Dumez]

Created attachment 261947 [details]
Patch

[Darin Adler]

Comment on attachment 261947 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261947&action=review

> Source/WebCore/ChangeLog:15
> +        This is because these are always enumerable for existing DOM interfaces:

You mean "never enumerable" or "always not enumerable" or "always DontEnum".

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2781
> +            push(@implContent, "    for (unsigned i = 0, count = thisObject->impl().length(); i < count; ++i)\n");
> +            push(@implContent, "        propertyNames.add(Identifier::from(state, i));\n");

Seems so inefficient to keep calling length over and over again, but I suppose that’s nothing compared to allocating all the identifiers!

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2787
> +            push(@implContent, "        for (auto& propertyName : thisObject->impl().supportedPropertyNames()) {\n");
> +            push(@implContent, "            propertyNames.add(Identifier::fromString(state, propertyName));\n");

Is creating an Identifier from an AtomicString inexpensive?

> Source/WebCore/html/HTMLCollection.h:143
> +    return (m_idMap.size() + m_nameMap.size()) * sizeof(Element*) + m_propertyNames.size() * sizeof(AtomicString);

Seems really strange to consider only the cost of the pointer to the AtomicStringImpl as part of the memory cost. What about the cost of the block holding the characters of the string?

[Radar WebKit Bug Importer]

<rdar://problem/22879779>

[Chris Dumez]

Comment on attachment 261947 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261947&action=review

>> Source/WebCore/ChangeLog:15
>> +        This is because these are always enumerable for existing DOM interfaces:
> 
> You mean "never enumerable" or "always not enumerable" or "always DontEnum".

Ok, I meant unenumerable.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2781
>> +            push(@implContent, "        propertyNames.add(Identifier::from(state, i));\n");
> 
> Seems so inefficient to keep calling length over and over again, but I suppose that’s nothing compared to allocating all the identifiers!

We don't, we initialize count from thisObject->impl().length() at the beginning of the loop.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2787
>> +            push(@implContent, "            propertyNames.add(Identifier::fromString(state, propertyName));\n");
> 
> Is creating an Identifier from an AtomicString inexpensive?

Yes, fairly inexpensive. Identifier::fromString(ExecState*, const AtomicString&) is inline and its implementation merely initializes the internal m_string member to atomicString.string(), which is cheap. Identifier does not seem to have any member besides String m_string.

>> Source/WebCore/html/HTMLCollection.h:143
>> +    return (m_idMap.size() + m_nameMap.size()) * sizeof(Element*) + m_propertyNames.size() * sizeof(AtomicString);
> 
> Seems really strange to consider only the cost of the pointer to the AtomicStringImpl as part of the memory cost. What about the cost of the block holding the characters of the string?

It is a very gross estimation, similarly to the pre-existing code which only considers sizeof(Element*).

[Chris Dumez]

Created attachment 262011 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262011 [details]
Patch

Clearing flags on attachment: 262011

Committed r190280: <http://trac.webkit.org/changeset/190280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.