getElementsByTagName() should take a qualifiedName in parameter

third_party/WebKit/Source/core/html/HTMLTagCollection.h

Index: third_party/WebKit/Source/core/html/HTMLTagCollection.h
diff --git a/third_party/WebKit/Source/core/html/HTMLTagCollection.h b/third_party/WebKit/Source/core/html/HTMLTagCollection.h
index 3c493ad0f6ced2a6ab74f8af8fb51cea34a85511..5ce6950b859a8efb1a421313557c022553da4554 100644
--- a/third_party/WebKit/Source/core/html/HTMLTagCollection.h
+++ b/third_party/WebKit/Source/core/html/HTMLTagCollection.h
@@ -36,17 +36,18 @@ class HTMLTagCollection final : public TagCollection {
  public:
   static HTMLTagCollection* Create(ContainerNode& root_node,
                                    CollectionType type,
-                                   const AtomicString& local_name) {
+                                   const AtomicString& qualified_name) {
     DCHECK_EQ(type, kHTMLTagCollectionType);
-    return new HTMLTagCollection(root_node, local_name);
+    return new HTMLTagCollection(root_node, qualified_name);
   }
 
   bool ElementMatches(const Element&) const;
 
  private:
-  HTMLTagCollection(ContainerNode& root_node, const AtomicString& local_name);
+  HTMLTagCollection(ContainerNode& root_node,
+                    const AtomicString& qualified_name);
 
-  AtomicString lowered_local_name_;
+  AtomicString lowered_qualified_name_;
 };
 
 DEFINE_TYPE_CASTS(HTMLTagCollection,
@@ -57,16 +58,9 @@ DEFINE_TYPE_CASTS(HTMLTagCollection,
 
 inline bool HTMLTagCollection::ElementMatches(
     const Element& test_element) const {
-  // Implements
-  // https://dom.spec.whatwg.org/#concept-getelementsbytagname
-  if (local_name_ != g_star_atom) {
-    const AtomicString& local_name =
-        test_element.IsHTMLElement() ? lowered_local_name_ : local_name_;
-    if (local_name != test_element.localName())
-      return false;
-  }
-  DCHECK_EQ(namespace_uri_, g_star_atom);
-  return true;
+  if (test_element.IsHTMLElement())

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

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]

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);
  }

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

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.)

[tkent, 2017/05/11 04:14:02]

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

+    return lowered_qualified_name_ == test_element.TagQName().ToString();
+  return qualified_name_ == test_element.TagQName().ToString();

[fs, 2017/05/08 14:20:36]

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.)

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

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 07:05:28]

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]

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]

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

 }
 
 }  // namespace blink