getElementsByTagName() should take a qualifiedName in parameter

getElementsByTagName() should take a qualifiedName in parameter

getElementsByTagName() should take a qualifiedName in parameter, not a
localName, according to the latest DOM specification:
https://dom.spec.whatwg.org/#dom-document-getelementsbytagname
https://dom.spec.whatwg.org/#concept-getelementsbytagname

Webkit patch:
https://trac.webkit.org/changeset/204441/

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/jBph1e0CekY

BUG=645165
Review-Url: https://codereview.chromium.org/2868823002
Cr-Commit-Position: refs/heads/master@{#471643}
Committed: https://chromium.googlesource.com/chromium/src/+/5025bccfe31b6c49a1e868d58568a89c75b3f338

[Shanmuga Pandi, 2017-05-08 12:01:08]

shanmuga.m@samsung.com changed reviewers:
+ foolip@chromium.org, fs@opera.com, tkent@chromium.org

[Shanmuga Pandi, 2017-05-08 12:01:09]

PTAL!

Thanks.

[Shanmuga Pandi, 2017-05-08 13:16:13]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 13:16:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[foolip, 2017-05-08 13:24:32]

On 2017/05/08 12:01:09, Shanmuga Pandi wrote:
> PTAL!
> 
> Thanks.

I won't have time to review this during this week, deferring to the other
reviewers

[fs, 2017-05-08 14:20:37]

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TagCollection.cpp (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TagCollection.cpp:42: return qualified_name_
== test_node.TagQName().ToString();
See comment in HTMLTagCollection.

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TagCollection.h:63: static TagCollectionNS*
Create(ContainerNode& root_node,
Is this even called?

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:63: return
qualified_name_ == test_element.TagQName().ToString();
This creates a temporary string for each comparison. Compare the two components
(prefix, localName) of QName instead. (This also means that the storage for the
filter needs to be different.)

[commit-bot: I haz the power, 2017-05-08 15:30:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 15:30:09]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-05-09 00:28:01]

Why do you introduce two new CollectionTypes?  Implementing new logic in
TagCollection would be simpler.

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/AllDescendantsCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/AllDescendantsCollection.h:1: // Copyright
2017 The Chromium Authors. All rights reserved.
If you copied the code from the WebKit patch, you should not change the
copyright notice.

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/AllDescendantsCollection.h:23: protected:
should be private?

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TagCollection.h:75: protected:
should be private?

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:61: if
(test_element.IsHTMLElement())
The specification says "if root’s node document is an HTML document".  Checking
IsHTMLElement() looks wrong.

https://dom.spec.whatwg.org/#concept-getelementsbytagname

[Shanmuga Pandi, 2017-05-10 06:35:45]

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:61: if
(test_element.IsHTMLElement())
On 2017/05/09 00:28:01, tkent wrote:
> The specification says "if root’s node document is an HTML document". 
Checking
> IsHTMLElement() looks wrong.
> 
> https://dom.spec.whatwg.org/#concept-getelementsbytagname

I have tried to replace the if condition with below options, but many
LayoutTests are failing on external/wpt/dom/*
1. if (test_element.GetDocument().IsHTMLDocument() ||
test_element.GetDocument().IsXHTMLDocument())
2. if (test_element.IsHTMLElement() &&
(test_element.GetDocument().IsHTMLDocument() ||
test_element.GetDocument().IsXHTMLDocument()))

Also
in ContainerNode::getElementsByTagName, we are already checking isHTMLDocument
or not like below.
  if (GetDocument().IsHTMLDocument()) {
    return EnsureCachedCollection<HTMLTagCollection>(kHTMLTagCollectionType,
                                                     qualified_name);
  }

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:63: return
qualified_name_ == test_element.TagQName().ToString();
On 2017/05/08 14:20:36, fs wrote:
> This creates a temporary string for each comparison. Compare the two
components
> (prefix, localName) of QName instead. (This also means that the storage for
the
> filter needs to be different.)

I think initially webkit tried this approach and later due to regression they
comparing TagQName string instead.
Related patch:
https://trac.webkit.org/changeset/208674/webkit
Please correct me if I misunderstood any.

[Shanmuga Pandi, 2017-05-10 06:37:14]

Description was changed from

==========
getElementsByTagName() should take a qualifiedName in parameter

getElementsByTagName() should take a qualifiedName in parameter, not a
localName, according to the latest DOM specification:
https://dom.spec.whatwg.org/#dom-document-getelementsbytagname
https://dom.spec.whatwg.org/#concept-getelementsbytagname

Webkit patch:
https://trac.webkit.org/changeset/204441/

BUG=645165
==========

to

==========
getElementsByTagName() should take a qualifiedName in parameter

getElementsByTagName() should take a qualifiedName in parameter, not a
localName, according to the latest DOM specification:
https://dom.spec.whatwg.org/#dom-document-getelementsbytagname
https://dom.spec.whatwg.org/#concept-getelementsbytagname

Webkit patch:
https://trac.webkit.org/changeset/204441/

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/jBph1e0CekY

BUG=645165
==========

[Shanmuga Pandi, 2017-05-10 07:05:28]

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:63: return
qualified_name_ == test_element.TagQName().ToString();
On 2017/05/08 14:20:36, fs wrote:
> This creates a temporary string for each comparison. Compare the two
components
> (prefix, localName) of QName instead. (This also means that the storage for
the
> filter needs to be different.)

Can we use Document::ParseQualifiedName() and store prefix and localName and
compare the two elements as your suggestion ?

[fs, 2017-05-10 08:34:15]

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:61: if
(test_element.IsHTMLElement())
On 2017/05/10 at 06:35:45, Shanmuga Pandi wrote:
> On 2017/05/09 00:28:01, tkent wrote:
> > The specification says "if root’s node document is an HTML document". 
Checking
> > IsHTMLElement() looks wrong.
> > 
> > https://dom.spec.whatwg.org/#concept-getelementsbytagname
> 
> I have tried to replace the if condition with below options, but many
LayoutTests are failing on external/wpt/dom/*
> 1. if (test_element.GetDocument().IsHTMLDocument() ||
test_element.GetDocument().IsXHTMLDocument())
> 2. if (test_element.IsHTMLElement() &&
(test_element.GetDocument().IsHTMLDocument() ||
test_element.GetDocument().IsXHTMLDocument()))
> 
> Also
> in ContainerNode::getElementsByTagName, we are already checking isHTMLDocument
or not like below.
>   if (GetDocument().IsHTMLDocument()) {
>     return EnsureCachedCollection<HTMLTagCollection>(kHTMLTagCollectionType,
>                                                      qualified_name);
>   }

FWIW, I think this "IsHTMLElement" check corresponds to the "Whose namespace is
the HTML namespace..." part (and also the "is not the HTML namespace" part by
association.)

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:63: return
qualified_name_ == test_element.TagQName().ToString();
On 2017/05/10 at 06:35:45, Shanmuga Pandi wrote:
> On 2017/05/08 14:20:36, fs wrote:
> > This creates a temporary string for each comparison. Compare the two
components
> > (prefix, localName) of QName instead. (This also means that the storage for
the
> > filter needs to be different.)
> 
> I think initially webkit tried this approach and later due to regression they
comparing TagQName string instead.
> Related patch:
> https://trac.webkit.org/changeset/208674/webkit
> Please correct me if I misunderstood any.

Fair enough. We could add a "CompareQualifiedName(const QualifiedName& qname,
const AtomicString& other_qname)" method that did a "component-wise" match. But
that is better as a separate CL in that case.

[Shanmuga Pandi, 2017-05-10 09:03:12]

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:63: return
qualified_name_ == test_element.TagQName().ToString();
On 2017/05/10 08:34:15, fs wrote:
> On 2017/05/10 at 06:35:45, Shanmuga Pandi wrote:
> > On 2017/05/08 14:20:36, fs wrote:
> > > This creates a temporary string for each comparison. Compare the two
> components
> > > (prefix, localName) of QName instead. (This also means that the storage
for
> the
> > > filter needs to be different.)
> > 
> > I think initially webkit tried this approach and later due to regression
they
> comparing TagQName string instead.
> > Related patch:
> > https://trac.webkit.org/changeset/208674/webkit
> > Please correct me if I misunderstood any.
> 
> Fair enough. We could add a "CompareQualifiedName(const QualifiedName& qname,
> const AtomicString& other_qname)" method that did a "component-wise" match.
But
> that is better as a separate CL in that case.

Ok. I will keep the current comparision as it is and will make separate CL as
per your suggestion.

[Shanmuga Pandi, 2017-05-10 12:17:22]

PTAL!

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TagCollection.h:63: static TagCollectionNS*
Create(ContainerNode& root_node,
On 2017/05/08 14:20:36, fs wrote:
> Is this even called?

Removed

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TagCollection.h:75: protected:
On 2017/05/09 00:28:01, tkent wrote:
> should be private?

Done.

[Shanmuga Pandi, 2017-05-10 12:19:34]

The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 12:20:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 14:25:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 14:25:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tkent, 2017-05-11 04:14:03]

Thank you for removing AllDescendantsCollection.  It made the code cleaner.

The logic looks ok.  However, the code still looks unnecessarily complex.
Can you split the change into multiple pieces?

1. HTMLTagCollection inherits from HTMLCollection instead of TagCollection
 We need to add local_name_ data member to HTMLTagCollection.

2. Rename TagCollection to TagCollectionNS

3. Implement qualified name support in HTMLTagCollection
  GetDocument().IsHTMLDocument() check in ContainerNode::getElementsByTagName()
should be moved
into HTMLTagCollection.

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTagCollection.h (right):

https://codereview.chromium.org/2868823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTagCollection.h:61: if
(test_element.IsHTMLElement())
On 2017/05/10 at 08:34:15, fs wrote:
> On 2017/05/10 at 06:35:45, Shanmuga Pandi wrote:
> > On 2017/05/09 00:28:01, tkent wrote:
> > > The specification says "if root’s node document is an HTML document". 
Checking
> > > IsHTMLElement() looks wrong.
> > > 
> > > https://dom.spec.whatwg.org/#concept-getelementsbytagname
> > 
> > I have tried to replace the if condition with below options, but many
LayoutTests are failing on external/wpt/dom/*
> > 1. if (test_element.GetDocument().IsHTMLDocument() ||
test_element.GetDocument().IsXHTMLDocument())
> > 2. if (test_element.IsHTMLElement() &&
(test_element.GetDocument().IsHTMLDocument() ||
test_element.GetDocument().IsXHTMLDocument()))
> > 
> > Also
> > in ContainerNode::getElementsByTagName, we are already checking
isHTMLDocument or not like below.
> >   if (GetDocument().IsHTMLDocument()) {
> >     return EnsureCachedCollection<HTMLTagCollection>(kHTMLTagCollectionType,
> >                                                      qualified_name);
> >   }
> 
> FWIW, I think this "IsHTMLElement" check corresponds to the "Whose namespace
is the HTML namespace..." part (and also the "is not the HTML namespace" part by
association.)

You're right.  "if root’s node document is an HTML document" is implemented in
ContainerNode::getElementsByTagName().
Sorry for the wrong comment!

[Shanmuga Pandi, 2017-05-11 11:45:11]

On 2017/05/11 04:14:03, tkent wrote:
> Thank you for removing AllDescendantsCollection.  It made the code cleaner.
> 
> The logic looks ok.  However, the code still looks unnecessarily complex.
> Can you split the change into multiple pieces?
> 
> 1. HTMLTagCollection inherits from HTMLCollection instead of TagCollection
>  We need to add local_name_ data member to HTMLTagCollection.
> 
> 2. Rename TagCollection to TagCollectionNS
> 
> 3. Implement qualified name support in HTMLTagCollection
>   GetDocument().IsHTMLDocument() check in
ContainerNode::getElementsByTagName()
> should be moved
> into HTMLTagCollection.
> 

I have tried your suggestion. But it is not working as expected.
Especially the below layout test is failing.
external/wpt/dom/nodes/Element-getElementsByTagName-change-document-HTMLNess.html


I am seeing TagCollection and HTMLTagCollection should be separate.
HTMLCollection::TraverseForwardToOffset and HTMLCollection::TraverseToFirst is
handling KTagCollectionType and KHTMLTagCollectionType separately.
Also AddCache implementation also maintaining atomic_name_caches_.
If containerNode document is changed to HTMLDocument to NonHTMLDocment and vice
versa, it should create new HTMLTagCollection, but here it took from cache.

Please let me know you comment.
Thanks

[tkent, 2017-05-12 07:36:48]

On 2017/05/11 at 11:45:11, shanmuga.m wrote:
> I have tried your suggestion. But it is not working as expected.
> Especially the below layout test is failing.
>
external/wpt/dom/nodes/Element-getElementsByTagName-change-document-HTMLNess.html
> 
> 
> I am seeing TagCollection and HTMLTagCollection should be separate.

Ah, you're right.
According to https://dom.spec.whatwg.org/#concept-getelementsbytagname, filters
of HTMLCollections returned by step 2 and step 3 shouldn't check HTML document. 
Sorry for consuming your time for my wrong proposal.

So, this CL lgtm as is.

[tkent, 2017-05-14 21:15:08]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-05-14 21:15:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-14 23:10:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494796508857270,
"parent_rev": "3639139a6216af6f66840f8348622d45ba06d5b9", "commit_rev":
"5025bccfe31b6c49a1e868d58568a89c75b3f338"}

[commit-bot: I haz the power, 2017-05-14 23:10:36]

Description was changed from

==========
getElementsByTagName() should take a qualifiedName in parameter

getElementsByTagName() should take a qualifiedName in parameter, not a
localName, according to the latest DOM specification:
https://dom.spec.whatwg.org/#dom-document-getelementsbytagname
https://dom.spec.whatwg.org/#concept-getelementsbytagname

Webkit patch:
https://trac.webkit.org/changeset/204441/

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/jBph1e0CekY

BUG=645165
==========

to

==========
getElementsByTagName() should take a qualifiedName in parameter

getElementsByTagName() should take a qualifiedName in parameter, not a
localName, according to the latest DOM specification:
https://dom.spec.whatwg.org/#dom-document-getelementsbytagname
https://dom.spec.whatwg.org/#concept-getelementsbytagname

Webkit patch:
https://trac.webkit.org/changeset/204441/

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/jBph1e0CekY

BUG=645165

Review-Url: https://codereview.chromium.org/2868823002
Cr-Commit-Position: refs/heads/master@{#471643}
Committed:
https://chromium.googlesource.com/chromium/src/+/5025bccfe31b6c49a1e868d58568...
==========

[commit-bot: I haz the power, 2017-05-14 23:10:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5025bccfe31b6c49a1e868d58568...