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. """
Created attachment 261947 [details] Patch
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?
<rdar://problem/22879779>
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*).
Created attachment 262011 [details] Patch
Comment on attachment 262011 [details] Patch Clearing flags on attachment: 262011 Committed r190280: <http://trac.webkit.org/changeset/190280>
All reviewed patches have been landed. Closing bug.