Bug 110611 - document.getElementsByTagName should return an HTMLCollection
Summary: document.getElementsByTagName should return an HTMLCollection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
: 78909 (view as bug list)
Depends on: 147979 148039 148080 148092 148116
Blocks: 148117
  Show dependency treegraph
 
Reported: 2013-02-22 08:02 PST by Boris Zbarsky
Modified: 2017-07-18 08:47 PDT (History)
13 users (show)

See Also:


Attachments
WIP Patch (62.59 KB, patch)
2015-08-21 12:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (62.60 KB, patch)
2015-08-21 12:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (62.62 KB, patch)
2015-08-21 12:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (64.82 KB, patch)
2015-08-21 13:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (609.62 KB, application/zip)
2015-08-21 14:14 PDT, Build Bot
no flags Details
Patch (80.70 KB, patch)
2015-08-21 15:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (80.43 KB, patch)
2015-08-21 16:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (599.05 KB, application/zip)
2015-08-21 17:49 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Zbarsky 2013-02-22 08:02:16 PST
See http://dom.spec.whatwg.org/#document

Note that the list WebKit returns isn't quite what a NodeList should be anyway; it's got various HTMLCollection-like features (e.g. named access to nodes).
Comment 1 Alexey Proskuryakov 2013-02-22 10:18:55 PST
DOM 3 Core says it should be a NodeList, not an HTMLCollection. Perhaps the new spec should be fixed to not introduce arbitrary incompatibilities?
Comment 2 Anne van Kesteren 2013-02-22 11:27:45 PST
ap, that would not describe WebKit's implementation.
Comment 3 Boris Zbarsky 2013-02-22 12:22:03 PST
WebKit doesn't make it what DOM3 calls a NodeList, as I already said.  The spec was changed to HTMLCollection based in part on what WebKit contributors said WebKit was willing to implement and what it was implementing right now.

Note that both Trident and Gecko return an HTMLCollection here.
Comment 4 Alexey Proskuryakov 2013-02-22 12:54:19 PST
> WebKit doesn't make it what DOM3 calls a NodeList, as I already said.

Yes, however the object returned by getElementsByTagName is a NodeList, which is observable.
Comment 5 Boris Zbarsky 2013-02-22 14:21:04 PST
Yes, but given that both Gecko and Trident return HTMLCollections there there are not very likely to be web compat issues with changing the toString and prototype.  Many fewer issues than all the other options for aligning the different implementations, for sure.
Comment 6 Alexey Proskuryakov 2013-02-22 14:25:43 PST
Was Gecko ever in compliance with DOM 3 Core in this regard? If so, when did it change?
Comment 7 Boris Zbarsky 2013-02-22 14:32:42 PST
I don't believe it was, no.  We used to use the same object for nearly all nodelists and htmlcollections (the one exception, I think, was .childNodes), and it always claimed to be an HTMLCollection.
Comment 8 Chris Dumez 2015-08-21 12:04:48 PDT
Created attachment 259639 [details]
WIP Patch
Comment 9 WebKit Commit Bot 2015-08-21 12:07:07 PDT
Attachment 259639 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Chris Dumez 2015-08-21 12:10:06 PDT
Created attachment 259641 [details]
WIP patch
Comment 11 WebKit Commit Bot 2015-08-21 12:12:17 PDT
Attachment 259641 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Chris Dumez 2015-08-21 12:57:26 PDT
Created attachment 259646 [details]
WIP patch
Comment 13 WebKit Commit Bot 2015-08-21 13:00:10 PDT
Attachment 259646 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Chris Dumez 2015-08-21 13:37:02 PDT
Created attachment 259650 [details]
WIP patch
Comment 15 WebKit Commit Bot 2015-08-21 13:38:41 PDT
Attachment 259650 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Build Bot 2015-08-21 14:14:44 PDT
Comment on attachment 259650 [details]
WIP patch

Attachment 259650 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/86974

New failing tests:
platform/mac/fast/dom/wrapper-classes-objc.html
Comment 17 Build Bot 2015-08-21 14:14:48 PDT
Created attachment 259657 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 18 Chris Dumez 2015-08-21 15:36:58 PDT
Created attachment 259670 [details]
Patch
Comment 19 WebKit Commit Bot 2015-08-21 15:38:35 PDT
Attachment 259670 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Darin Adler 2015-08-21 16:00:54 PDT
Comment on attachment 259670 [details]
Patch

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

> Source/WebCore/bindings/objc/DOMHTML.mm:149
> +    return kit(WTF::getPtr(createFragmentFromMarkup(*core(self), markupString, [baseURL absoluteString])));

If writing by hand, I think we can just write .get(). The WTF::getPtr function exists so we can conveniently write code that handles a type without knowing whether it’s RefPtr<X> or X* in template code or automatically generated code.

> Source/WebCore/dom/NodeRareData.h:181
> +    void removeCachedCollectionWithQualifiedName(HTMLCollection* collection, const AtomicString& namespaceURI, const AtomicString& localName)

This function should take a reference rather than a pointer.

> Source/WebCore/dom/TagCollection.cpp:50
> +    , m_loweredLocalName(localName.lower())

Would be better to use an ASCII lowering function here rather than a full Unicode lowering function.

> Source/WebCore/editing/markup.cpp:680
> +    Ref<HTMLBodyElement> fakeBody = HTMLBodyElement::create(document);
> +    Ref<DocumentFragment> fragment = DocumentFragment::create(document);

Use auto here?

> Source/WebCore/editing/markup.cpp:689
> +    for (auto& attachment : descendantsOfType<HTMLAttachmentElement>(fragment)) {
> +        attachment.setFile(File::create(attachment.fastGetAttribute(webkitattachmentpathAttr)).ptr());
> +        attachment.removeAttribute(webkitattachmentpathAttr);
>      }

Any chance that the document will be mutated while iterating, perhaps if there are mutation event handlers involved? Is the old code’s behavior identical to the new code’s behavior in a case like that?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:447
> +        for (auto& plugInImageElement : descendantsOfType<HTMLPlugInImageElement>(*frame->document())) {

Why not just call this local “element”.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:449
> +            const URL& loadedURL = plugInImageElement.loadedUrl();
> +            String otherMimeType = plugInImageElement.loadedMimeType();

Do we need these local variables? I think the code will read better without them.

> Source/WebKit/mac/WebView/WebFrame.mm:871
> -    return kit(createFragmentFromMarkup(*document, markupString, baseURLString, DisallowScriptingContent).get());
> +    return kit(WTF::getPtr(createFragmentFromMarkup(*document, markupString, baseURLString, DisallowScriptingContent)));

Why this change? Is this a Ref now? Maybe use ptr() instead of WTF::getPtr().
Comment 21 Chris Dumez 2015-08-21 16:25:10 PDT
Comment on attachment 259670 [details]
Patch

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

>> Source/WebCore/bindings/objc/DOMHTML.mm:149
>> +    return kit(WTF::getPtr(createFragmentFromMarkup(*core(self), markupString, [baseURL absoluteString])));
> 
> If writing by hand, I think we can just write .get(). The WTF::getPtr function exists so we can conveniently write code that handles a type without knowing whether it’s RefPtr<X> or X* in template code or automatically generated code.

Ok, will do.

>> Source/WebCore/dom/NodeRareData.h:181
>> +    void removeCachedCollectionWithQualifiedName(HTMLCollection* collection, const AtomicString& namespaceURI, const AtomicString& localName)
> 
> This function should take a reference rather than a pointer.

Will do.

>> Source/WebCore/dom/TagCollection.cpp:50
>> +    , m_loweredLocalName(localName.lower())
> 
> Would be better to use an ASCII lowering function here rather than a full Unicode lowering function.

Ok, will change to localName.convertToASCIILowercase()

>> Source/WebCore/editing/markup.cpp:680
>> +    Ref<DocumentFragment> fragment = DocumentFragment::create(document);
> 
> Use auto here?

Ok, will do.

>> Source/WebCore/editing/markup.cpp:689
>>      }
> 
> Any chance that the document will be mutated while iterating, perhaps if there are mutation event handlers involved? Is the old code’s behavior identical to the new code’s behavior in a case like that?

I have just checked. I thought we were safe because WebKit does not implement attribute-level DOM mutation events. However, by looking at the code, I see that the DOMSubtreeModified can be fired synchronously :(
Therefore, using descendantsOfType() is not safe.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:447
>> +        for (auto& plugInImageElement : descendantsOfType<HTMLPlugInImageElement>(*frame->document())) {
> 
> Why not just call this local “element”.

Ok.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:449
>> +            String otherMimeType = plugInImageElement.loadedMimeType();
> 
> Do we need these local variables? I think the code will read better without them.

Will drop them.

>> Source/WebKit/mac/WebView/WebFrame.mm:871
>> +    return kit(WTF::getPtr(createFragmentFromMarkup(*document, markupString, baseURLString, DisallowScriptingContent)));
> 
> Why this change? Is this a Ref now? Maybe use ptr() instead of WTF::getPtr().

Yes, it returns a Ref<> now, I'll use .ptr().
Comment 22 Chris Dumez 2015-08-21 16:35:32 PDT
Created attachment 259683 [details]
Patch
Comment 23 WebKit Commit Bot 2015-08-21 16:38:43 PDT
Attachment 259683 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Build Bot 2015-08-21 17:49:36 PDT
Comment on attachment 259683 [details]
Patch

Attachment 259683 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/87861

New failing tests:
http/tests/security/storage-blocking-strengthened-plugin.html
http/tests/security/storage-blocking-loosened-plugin.html
Comment 25 Build Bot 2015-08-21 17:49:41 PDT
Created attachment 259689 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 26 WebKit Commit Bot 2015-08-21 17:50:38 PDT
Comment on attachment 259683 [details]
Patch

Clearing flags on attachment: 259683

Committed r188809: <http://trac.webkit.org/changeset/188809>
Comment 27 WebKit Commit Bot 2015-08-21 17:50:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Michael Catanzaro 2015-08-22 11:59:19 PDT
GTK folks, I don't know how to fix the build and the DOM bindings after this. At least webkit_dom_document_get_elements_by_tag_name, webkit_dom_document_create_node_iterator, and webkit_dom_document_create_tree_walker (from our stable API) have disappeared. Probably more; those are just the functions we use in DOMNodeTest.cpp and DOMNodeFilterTest.cpp.
Comment 29 Carlos Garcia Campos 2015-08-24 04:03:35 PDT
(In reply to comment #28)
> GTK folks, I don't know how to fix the build and the DOM bindings after
> this. At least webkit_dom_document_get_elements_by_tag_name,
> webkit_dom_document_create_node_iterator, and
> webkit_dom_document_create_tree_walker (from our stable API) have
> disappeared. Probably more; those are just the functions we use in
> DOMNodeTest.cpp and DOMNodeFilterTest.cpp.

I fixed this in r188854, using getElementsByTagNameForObjC and getElementsByTagNameNSForObjC. Maybe these should be renamed as ForBindings or AsHTMLCollection or something like that.
Comment 30 Chris Dumez 2015-08-24 09:33:03 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > GTK folks, I don't know how to fix the build and the DOM bindings after
> > this. At least webkit_dom_document_get_elements_by_tag_name,
> > webkit_dom_document_create_node_iterator, and
> > webkit_dom_document_create_tree_walker (from our stable API) have
> > disappeared. Probably more; those are just the functions we use in
> > DOMNodeTest.cpp and DOMNodeFilterTest.cpp.
> 
> I fixed this in r188854, using getElementsByTagNameForObjC and
> getElementsByTagNameNSForObjC. Maybe these should be renamed as ForBindings
> or AsHTMLCollection or something like that.

Thanks for the fix.

'ForBindings' wouldn't be correct as JS bindings don't use it (only ObjC bindings, and not I assume the GTK ones). AsHTMLCollection wouldn't either as it returns a NodeList.
Maybe rename to getElementsByTagNameAsNodeList() would be a better name though, now that it is used by more than the ObjC bindings. Feel free to make this change.
Comment 31 Antti Koivisto 2015-08-24 10:00:01 PDT
Maybe something like getElementsByTagNameAsNodeListForLegacyBindings
Comment 32 Carlos Garcia Campos 2015-08-24 12:04:29 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > GTK folks, I don't know how to fix the build and the DOM bindings after
> > > this. At least webkit_dom_document_get_elements_by_tag_name,
> > > webkit_dom_document_create_node_iterator, and
> > > webkit_dom_document_create_tree_walker (from our stable API) have
> > > disappeared. Probably more; those are just the functions we use in
> > > DOMNodeTest.cpp and DOMNodeFilterTest.cpp.
> > 
> > I fixed this in r188854, using getElementsByTagNameForObjC and
> > getElementsByTagNameNSForObjC. Maybe these should be renamed as ForBindings
> > or AsHTMLCollection or something like that.
> 
> Thanks for the fix.
> 
> 'ForBindings' wouldn't be correct as JS bindings don't use it (only ObjC
> bindings, and not I assume the GTK ones). AsHTMLCollection wouldn't either
> as it returns a NodeList.
> Maybe rename to getElementsByTagNameAsNodeList() would be a better name
> though, now that it is used by more than the ObjC bindings. Feel free to
> make this change.

Oops, sure, I actually meant AsNodeList().
Comment 33 Chris Dumez 2017-07-18 08:47:21 PDT
*** Bug 78909 has been marked as a duplicate of this bug. ***