NodeList should not have a named getter
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Aug 2015 22:29:26 +0000 (22:29 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Aug 2015 22:29:26 +0000 (22:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=148117

Reviewed by Darin Adler.

Source/WebCore:

Drop the named property getter on NodeList to match the specification:
https://dom.spec.whatwg.org/#interface-nodelist

This change could be a bit risky but:
1. Firefox and IE never had this named property getter on NodeList.
2. Chrome successfuly dropped this named property getter in early 2014:
   https://src.chromium.org/viewvc/blink?revision=166356&view=revision
3. Our named property getter on NodeList is only partly functional:
   It only matches by 'id' attribute, not by 'name' attribute.
4. Our APIs that were wrongly returning a NodeList instead of an
   HTMLCollection (getElementsByTagName() / getElementsByClassName())
   have been fixed in r188735 and r188809. HTMLCollection does have
   a named property getter.
5. This named getter adds code complexity.

* bindings/js/JSNodeListCustom.cpp:
(WebCore::createWrapper): Deleted.
* dom/ChildNodeList.cpp:
(WebCore::ChildNodeList::invalidateCache): Deleted.
* dom/ChildNodeList.h:
* dom/LiveNodeList.cpp:
* dom/LiveNodeList.h:
* dom/NodeList.h:
* dom/NodeList.idl:
* dom/StaticNodeList.cpp:
(WebCore::StaticElementList::length): Deleted.
(WebCore::StaticElementList::item): Deleted.
* dom/StaticNodeList.h:
* html/HTMLCollection.h:

LayoutTests:

* fast/dom/childnode-item-after-itemname-expected.txt: Removed.
* fast/dom/childnode-item-after-itemname.html: Removed.
Drop test as it is no longer relevant now that NodeList no longer
has a named property getter.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@188829 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/childnode-item-after-itemname-expected.txt [deleted file]
LayoutTests/fast/dom/childnode-item-after-itemname.html [deleted file]
LayoutTests/fast/dom/named-items-with-symbol-name-expected.txt
LayoutTests/fast/dom/named-items-with-symbol-name.html
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSNodeListCustom.cpp
Source/WebCore/dom/ChildNodeList.cpp
Source/WebCore/dom/ChildNodeList.h
Source/WebCore/dom/LiveNodeList.cpp
Source/WebCore/dom/LiveNodeList.h
Source/WebCore/dom/NodeList.h
Source/WebCore/dom/NodeList.idl
Source/WebCore/dom/StaticNodeList.cpp
Source/WebCore/dom/StaticNodeList.h
Source/WebCore/html/HTMLCollection.h

index 08dc8ce..6e15433 100644 (file)
@@ -1,3 +1,15 @@
+2015-08-22  Chris Dumez  <cdumez@apple.com>
+
+        NodeList should not have a named getter
+        https://bugs.webkit.org/show_bug.cgi?id=148117
+
+        Reviewed by Darin Adler.
+
+        * fast/dom/childnode-item-after-itemname-expected.txt: Removed.
+        * fast/dom/childnode-item-after-itemname.html: Removed.
+        Drop test as it is no longer relevant now that NodeList no longer
+        has a named property getter.
+
 2015-08-22  Alexey Proskuryakov  <ap@apple.com>
 
         Tweak a test that became flaky on some machines after r188793.
diff --git a/LayoutTests/fast/dom/childnode-item-after-itemname-expected.txt b/LayoutTests/fast/dom/childnode-item-after-itemname-expected.txt
deleted file mode 100644 (file)
index 097f91a..0000000
+++ /dev/null
@@ -1,3 +0,0 @@
-This tests accessing the first element in childNodes after accessing the second element by its id. WebKit should retrieve each element correctly, and you should see 2 1 below:
-
-2 1
diff --git a/LayoutTests/fast/dom/childnode-item-after-itemname.html b/LayoutTests/fast/dom/childnode-item-after-itemname.html
deleted file mode 100644 (file)
index b7dfcce..0000000
+++ /dev/null
@@ -1,18 +0,0 @@
-<!DOCTYPE html>
-<html>
-<body>
-<p>This tests accessing the first element in childNodes after accessing the second element by its id.
-WebKit should retrieve each element correctly, and you should see 2 1 below:</p>
-<span id="testItem"></span><span id="tests" style="display: none;">1<span id="testItem">2</span></span>
-<script>
-
-if (window.testRunner)
-    testRunner.dumpAsText();
-
-var tests = document.querySelector('#tests');
-document.writeln(tests.childNodes['testItem'].textContent);
-document.writeln(tests.childNodes[0].textContent);
-
-</script>
-</body>
-</html>
index 5b0acea..5166590 100644 (file)
@@ -3,11 +3,6 @@ This tests Symbol property names with a number of named items collections.
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-[object NodeList]
-PASS querySelectorAllList.length === 4 is true
-PASS querySelectorAllList[Symbol()] === undefined is true
-PASS querySelectorAllList[Symbol('div')] === undefined is true
-PASS querySelectorAllList['Symbol(div)'] instanceof HTMLDivElement is true
 
 [object HTMLCollection]
 PASS getElementsByTagNameList.length === 4 is true
index 7210103..60e84af 100644 (file)
 <script>
 description('This tests Symbol property names with a number of named items collections.');
 
-var querySelectorAllList = document.querySelectorAll('div');
-debug(String(querySelectorAllList));
-shouldBeTrue("querySelectorAllList.length === 4");
-shouldBeTrue("querySelectorAllList[Symbol()] === undefined");
-shouldBeTrue("querySelectorAllList[Symbol('div')] === undefined");
-shouldBeTrue("querySelectorAllList['Symbol(div)'] instanceof HTMLDivElement");
-
 var getElementsByTagNameList = document.getElementsByTagName('div');
 debug("\n" + String(getElementsByTagNameList));
 shouldBeTrue("getElementsByTagNameList.length === 4");
index ad8e015..599300f 100644 (file)
@@ -1,3 +1,40 @@
+2015-08-22  Chris Dumez  <cdumez@apple.com>
+
+        NodeList should not have a named getter
+        https://bugs.webkit.org/show_bug.cgi?id=148117
+
+        Reviewed by Darin Adler.
+
+        Drop the named property getter on NodeList to match the specification:
+        https://dom.spec.whatwg.org/#interface-nodelist
+
+        This change could be a bit risky but:
+        1. Firefox and IE never had this named property getter on NodeList.
+        2. Chrome successfuly dropped this named property getter in early 2014:
+           https://src.chromium.org/viewvc/blink?revision=166356&view=revision
+        3. Our named property getter on NodeList is only partly functional:
+           It only matches by 'id' attribute, not by 'name' attribute.
+        4. Our APIs that were wrongly returning a NodeList instead of an
+           HTMLCollection (getElementsByTagName() / getElementsByClassName())
+           have been fixed in r188735 and r188809. HTMLCollection does have
+           a named property getter.
+        5. This named getter adds code complexity.
+
+        * bindings/js/JSNodeListCustom.cpp:
+        (WebCore::createWrapper): Deleted.
+        * dom/ChildNodeList.cpp:
+        (WebCore::ChildNodeList::invalidateCache): Deleted.
+        * dom/ChildNodeList.h:
+        * dom/LiveNodeList.cpp:
+        * dom/LiveNodeList.h:
+        * dom/NodeList.h:
+        * dom/NodeList.idl:
+        * dom/StaticNodeList.cpp:
+        (WebCore::StaticElementList::length): Deleted.
+        (WebCore::StaticElementList::item): Deleted.
+        * dom/StaticNodeList.h:
+        * html/HTMLCollection.h:
+
 2015-08-22  Andreas Kling  <akling@apple.com>
 
         [JSC] Static hash tables should be 100% compile-time constant.
index 2fd9f33..7f02e24 100644 (file)
@@ -51,18 +51,6 @@ bool JSNodeListOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handl
     return false;
 }
 
-// FIXME: NodeList should not have a named getter. It currently has one because getElementByTagName()
-// returns a NodeList instead of an HTMLCollection.
-bool JSNodeList::nameGetter(ExecState* exec, PropertyName propertyName, JSValue& value)
-{
-    auto item = impl().namedItem(propertyNameToAtomicString(propertyName));
-    if (!item)
-        return false;
-
-    value = toJS(exec, globalObject(), item);
-    return true;
-}
-
 JSC::JSValue createWrapper(JSDOMGlobalObject& globalObject, NodeList& nodeList)
 {
     // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
index def9eb2..95baa2f 100644 (file)
@@ -81,23 +81,6 @@ void ChildNodeList::collectionTraverseBackward(Node*& current, unsigned count) c
         current = current->previousSibling();
 }
 
-Node* ChildNodeList::namedItem(const AtomicString& name) const
-{
-    // FIXME: According to the spec child node list should not have namedItem().
-    if (m_parent.get().inDocument()) {
-        Element* element = m_parent.get().treeScope().getElementById(name);
-        if (element && element->parentNode() == m_parent.ptr())
-            return element;
-        if (!element || !m_parent.get().treeScope().containsMultipleElementsWithId(name))
-            return nullptr;
-    }
-    for (auto& element : childrenOfType<Element>(m_parent)) {
-        if (element.hasID() && element.idForStyleResolution() == name)
-            return const_cast<Element*>(&element);
-    }
-    return nullptr;
-}
-
 void ChildNodeList::invalidateCache()
 {
     m_indexCache.invalidate(*this);
index d521cf3..09edbf2 100644 (file)
@@ -48,7 +48,6 @@ private:
 
     virtual unsigned length() const override { return 0; }
     virtual Node* item(unsigned) const override { return nullptr; }
-    virtual Node* namedItem(const AtomicString&) const override { return nullptr; }
     virtual size_t memoryCost() const override { return 0; }
 
     virtual bool isEmptyNodeList() const override { return true; }
@@ -83,7 +82,6 @@ private:
 
     virtual unsigned length() const override;
     virtual Node* item(unsigned index) const override;
-    virtual Node* namedItem(const AtomicString&) const override;
     virtual size_t memoryCost() const override { return m_indexCache.memoryCost(); }
 
     virtual bool isChildNodeList() const override { return true; }
index aa7b2e4..0135eac 100644 (file)
@@ -50,35 +50,4 @@ ContainerNode& LiveNodeList::rootNode() const
     return ownerNode();
 }
 
-Node* LiveNodeList::namedItem(const AtomicString& elementId) const
-{
-    // FIXME: Why doesn't this look into the name attribute like HTMLCollection::namedItem does?
-    Node& rootNode = this->rootNode();
-
-    if (rootNode.inDocument()) {
-        Element* element = rootNode.treeScope().getElementById(elementId);
-        if (element && elementMatches(*element) && element->isDescendantOf(&rootNode))
-            return element;
-        if (!element)
-            return nullptr;
-        // In the case of multiple nodes with the same name, just fall through.
-    }
-
-    if (elementId.isEmpty())
-        return nullptr;
-
-    unsigned length = this->length();
-    for (unsigned i = 0; i < length; i++) {
-        Node* node = item(i);
-        if (!is<Element>(*node))
-            continue;
-        Element& element = downcast<Element>(*node);
-        // FIXME: This should probably be using getIdAttribute instead of idForStyleResolution.
-        if (element.hasID() && element.idForStyleResolution() == elementId)
-            return node;
-    }
-
-    return nullptr;
-}
-
 } // namespace WebCore
index 0104266..ddd285a 100644 (file)
@@ -45,8 +45,6 @@ public:
     LiveNodeList(ContainerNode& ownerNode, NodeListInvalidationType);
     virtual ~LiveNodeList();
 
-    virtual Node* namedItem(const AtomicString&) const override final;
-
     virtual bool elementMatches(Element&) const = 0;
     virtual bool isRootedAtDocument() const = 0;
 
index 3221952..822a892 100644 (file)
@@ -39,7 +39,6 @@ public:
     // DOM methods & attributes for NodeList
     virtual unsigned length() const = 0;
     virtual Node* item(unsigned index) const = 0;
-    virtual Node* namedItem(const AtomicString&) const = 0;
 
     // Other methods (not part of DOM)
     virtual bool isLiveNodeList() const { return false; }
index 033de99..7ed36ee 100644 (file)
 
 [
     CustomIsReachable,
-    CustomNamedGetter,
     CustomToJSObject,
     JSCustomHeader,
     SkipVTableValidation,
     ReportExtraMemoryCost,
 ] interface NodeList {
-
     getter Node item(unsigned long index);
-
     readonly attribute unsigned long length;
-
 };
 
index 5f0233b..6c68c6e 100644 (file)
@@ -43,18 +43,6 @@ Node* StaticNodeList::item(unsigned index) const
     return nullptr;
 }
 
-Node* StaticNodeList::namedItem(const AtomicString& elementId) const
-{
-    if (elementId.isEmpty())
-        return nullptr;
-    for (unsigned i = 0, length = m_nodes.size(); i < length; ++i) {
-        Node& node = const_cast<Node&>(m_nodes[i].get());
-        if (is<Element>(node) && downcast<Element>(node).getIdAttribute() == elementId)
-            return &node;
-    }
-    return nullptr;
-}
-
 unsigned StaticElementList::length() const
 {
     return m_elements.size();
@@ -67,16 +55,4 @@ Node* StaticElementList::item(unsigned index) const
     return nullptr;
 }
 
-Node* StaticElementList::namedItem(const AtomicString& elementId) const
-{
-    if (elementId.isEmpty())
-        return nullptr;
-    for (unsigned i = 0, length = m_elements.size(); i < length; ++i) {
-        Element& element = const_cast<Element&>(m_elements[i].get());
-        if (element.getIdAttribute() == elementId)
-            return &element;
-    }
-    return nullptr;
-}
-
 } // namespace WebCore
index 260eb22..bbf93d5 100644 (file)
@@ -53,7 +53,6 @@ public:
 
     virtual unsigned length() const override;
     virtual Node* item(unsigned index) const override;
-    virtual Node* namedItem(const AtomicString&) const override;
 
 private:
     StaticNodeList() { }
@@ -77,7 +76,6 @@ public:
 
     virtual unsigned length() const override;
     virtual Node* item(unsigned index) const override;
-    virtual Node* namedItem(const AtomicString&) const override;
 
 private:
     StaticElementList()
index c39e296..3b4c14a 100644 (file)
@@ -65,7 +65,7 @@ public:
 
     // DOM API
     virtual Element* item(unsigned index) const override = 0; // Tighten return type from NodeList::item().
-    virtual Element* namedItem(const AtomicString& name) const override = 0; // Tighten return type from NodeList::namedItem().
+    virtual Element* namedItem(const AtomicString& name) const = 0;
     RefPtr<NodeList> tags(const String&);
 
     // Non-DOM API