Reviewed by Darin.
authorap <ap@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Mar 2007 16:48:16 +0000 (16:48 +0000)
committerap <ap@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Mar 2007 16:48:16 +0000 (16:48 +0000)
        http://bugs.webkit.org/show_bug.cgi?id=13190
        XPath incorrectly handles namespaces on attributes

WebCore:
        * xml/XPathStep.cpp:
        (WebCore::XPath::Step::nodesInAxis): Added a special case for faster attribute lookup; gives a slight but
        measurable performance improvement for bug 13021.
        (WebCore::XPath::Step::nodeMatches): Fixed NameTest for attribute nodes.

        * xml/XPathStep.h:
        (WebCore::XPath::Step::NodeTest::NodeTest):
        (WebCore::XPath::Step::NodeTest::namespaceURI):
        (WebCore::XPath::Step::nodeTest):
        (WebCore::XPath::Step::setNodeTest):
        Move m_namespaceURI to NodeTest, where it belongs. Removed unused m_nodeTestData (oops!).

        * xml/XPathGrammar.y:
        * xml/XPathPath.cpp:
        (WebCore::XPath::LocationPath::optimizeStepPair):
        Accounted for the above change.

LayoutTests:
        * fast/xpath/attr-namespace-expected.txt: Added.
        * fast/xpath/attr-namespace.html: Added.

        * fast/xpath/xpath-namespaces-expected.txt:
        * fast/xpath/xpath-namespaces.html:
        Cleaned up; added a couple more cases (which passed anyway, but weren't tested for).

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

LayoutTests/ChangeLog
LayoutTests/fast/xpath/attr-namespace-expected.txt [new file with mode: 0644]
LayoutTests/fast/xpath/attr-namespace.html [new file with mode: 0644]
LayoutTests/fast/xpath/xpath-namespaces-expected.txt
LayoutTests/fast/xpath/xpath-namespaces.html
WebCore/ChangeLog
WebCore/xml/XPathGrammar.y
WebCore/xml/XPathPath.cpp
WebCore/xml/XPathStep.cpp
WebCore/xml/XPathStep.h

index 5853404bb19a22ef7c17c6e371140ab22df3ca7b..b97da65527ec6225137230b8f77b30465314a4c0 100644 (file)
@@ -1,3 +1,17 @@
+2007-03-28  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        http://bugs.webkit.org/show_bug.cgi?id=13190
+        XPath incorrectly handles namespaces on attributes
+
+        * fast/xpath/attr-namespace-expected.txt: Added.
+        * fast/xpath/attr-namespace.html: Added.
+
+        * fast/xpath/xpath-namespaces-expected.txt:
+        * fast/xpath/xpath-namespaces.html:
+        Cleaned up; added a couple more cases (which passed anyway, but weren't tested for).
+
 2007-03-27  Darin Adler  <darin@apple.com>
 
         * svg/hixie/text/003-expected.txt: Updated results for this one test that now has
diff --git a/LayoutTests/fast/xpath/attr-namespace-expected.txt b/LayoutTests/fast/xpath/attr-namespace-expected.txt
new file mode 100644 (file)
index 0000000..648034c
--- /dev/null
@@ -0,0 +1,15 @@
+PASS doc.evaluate("//@attr1", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 1
+PASS doc.evaluate("//@attr2", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 0
+PASS doc.evaluate("//@ns:attr2", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 1
+PASS doc.evaluate("//@ns:xmlns", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 1
+PASS doc.evaluate("//@xml:id", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength threw exception Error: NAMESPACE_ERR: DOM Exception 14.
+PASS doc.evaluate("//@xml:id", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 1
+PASS doc.evaluate("//@*", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 4
+PASS doc.evaluate("//@ns:*", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 2
+PASS doc.evaluate("//@xml:*", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 1
+PASS doc.evaluate("//@xmlns", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 0
+PASS doc.evaluate("//@xmlns:*", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength is 0
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/xpath/attr-namespace.html b/LayoutTests/fast/xpath/attr-namespace.html
new file mode 100644 (file)
index 0000000..cecd54c
--- /dev/null
@@ -0,0 +1,44 @@
+<html>
+<head>
+<link rel="stylesheet" href="../js/resources/js-test-style.css">
+<script src="../js/resources/js-test-pre.js"></script>
+<script src="xpath-test-pre.js"></script>
+</head>
+<body>
+<div id="console"></div>
+
+<script>
+function nsResolver(prefix) {
+    if (prefix == "ns")
+        return "foobarns";
+    if (prefix == "xml")
+        return "http://www.w3.org/XML/1998/namespace";
+    if (prefix == "xmlns")
+        return "http://www.w3.org/2000/xmlns/";
+    alert("Unexpected prefix " + prefix);
+}
+
+doc = (new DOMParser).parseFromString(
+    '<doc>' +
+    '  <elem attr1="1" ns:attr2="2" xml:id="3" ns:xmlns="4" xmlns:ns="foobarns" xmlns="barfoons"/>' +
+    '</doc>',
+    'application/xml');
+
+shouldBe('doc.evaluate("//@attr1", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '1');
+shouldBe('doc.evaluate("//@attr2", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '0');
+shouldBe('doc.evaluate("//@ns:attr2", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '1');
+shouldBe('doc.evaluate("//@ns:xmlns", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '1');
+shouldThrow('doc.evaluate("//@xml:id", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength');
+shouldBe('doc.evaluate("//@xml:id", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '1');
+shouldBe('doc.evaluate("//@*", doc, null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '4');
+shouldBe('doc.evaluate("//@ns:*", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '2');
+shouldBe('doc.evaluate("//@xml:*", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '1');
+shouldBe('doc.evaluate("//@xmlns", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '0');
+shouldBe('doc.evaluate("//@xmlns:*", doc, nsResolver, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null).snapshotLength', '0');
+
+var successfullyParsed = true;
+
+</script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>
index 8851cb89b9b67affdd0fa50b4c791d42c4c171e3..51f6178092d15db387d52d75ac01d46261506d5c 100644 (file)
@@ -1,3 +1,9 @@
 This tests that XPath expressions with prefixes work correctly.
-SUCCESS: test completed
+
+PASS /ns:foo
+PASS /ns:*
+PASS /foo:*
+PASS successfullyParsed is true
+
+TEST COMPLETE
 
index 1a7262a0eaebf50df30d4c8446e25955e71e000a..992c11e0603fff90285529070c9b0a3bc58e28ad 100644 (file)
@@ -1,43 +1,34 @@
 <html>
-  <head>
-    <script>
-function debug(str) {
-    var d = document.getElementById('console');
-    d.appendChild(document.createTextNode(str + "\n"));
-}
-    
-function runTests () {
-    if (window.layoutTestController)
-        layoutTestController.dumpAsText();
-        
-    var xmlString = '<ns:foo xmlns:ns="http://www.example.org"/>';
+<head>
+<link rel="stylesheet" href="../js/resources/js-test-style.css">
+<script src="../js/resources/js-test-pre.js"></script>
+<script src="xpath-test-pre.js"></script>
+</head>
+<body>
+<p>This tests that XPath expressions with prefixes work correctly.</p>
+<div id="console"></div>
+<script>
+    var xmlString = '<ns:foo xmlns:ns="http://www.example.org" xmlns:foo="urn:foobar"/>';
 
     var doc = (new DOMParser()).parseFromString(xmlString, "text/xml");
     var contextNode = doc.documentElement;
     var nsResolver = document.createNSResolver(contextNode);
 
     var expr = doc.createExpression("/ns:foo", nsResolver);
-    var result = expr.evaluate(contextNode, XPathResult.ANY_TYPE, null)
+    var result = expr.evaluate(contextNode, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);
+    checkSnapshot("/ns:foo", result, [doc.documentElement]);
 
-    var element = result.iterateNext();
+    var expr = doc.createExpression("/ns:*", nsResolver);
+    var result = expr.evaluate(contextNode, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);
+    checkSnapshot("/ns:*", result, [doc.documentElement]);
 
-    if (element == 0) {
-        debug('FAILURE: no result node was found');
-        return;
-    }
-  
-    if (element.nodeName != 'ns:foo') {
-        debug('FAILURE: did not find the correct node');
-        return;
-    }
-  
-    debug('SUCCESS: test completed')
-}
-    </script>
-  </head>
-<body onload="runTests()">
-This tests that XPath expressions with prefixes work correctly.
-<pre id="console">
-</pre>
+    var expr = doc.createExpression("/foo:*", nsResolver);
+    var result = expr.evaluate(contextNode, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);
+    checkSnapshot("/foo:*", result, []);
+
+    var successfullyParsed = true;
+
+</script>
+<script src="../js/resources/js-test-post.js"></script>
 </body>
 </html>
index d9c2e95d233d492cc48a9e62106867dbb4531ee8..36899c595250f23341921e2846945dffe1229483 100644 (file)
@@ -1,3 +1,27 @@
+2007-03-28  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        http://bugs.webkit.org/show_bug.cgi?id=13190
+        XPath incorrectly handles namespaces on attributes
+
+        * xml/XPathStep.cpp:
+        (WebCore::XPath::Step::nodesInAxis): Added a special case for faster attribute lookup; gives a slight but 
+        measurable performance improvement for bug 13021.
+        (WebCore::XPath::Step::nodeMatches): Fixed NameTest for attribute nodes.
+
+        * xml/XPathStep.h:
+        (WebCore::XPath::Step::NodeTest::NodeTest):
+        (WebCore::XPath::Step::NodeTest::namespaceURI):
+        (WebCore::XPath::Step::nodeTest):
+        (WebCore::XPath::Step::setNodeTest):
+        Move m_namespaceURI to NodeTest, where it belongs. Removed unused m_nodeTestData (oops!).
+
+        * xml/XPathGrammar.y:
+        * xml/XPathPath.cpp:
+        (WebCore::XPath::LocationPath::optimizeStepPair):
+        Accounted for the above change.
+
 2007-03-28  Oliver Hunt  <oliver@apple.com>
 
         rs=Hyatt.
index 9c891d7a20b70b2b00c56628ae3c55b962ba7f6d..e8c026c1e1362b7014ed01f98f063e6dc3c6ff6c 100644 (file)
@@ -198,10 +198,10 @@ Step:
         }
         
         if ($2) {
-            $$ = new Step(Step::ChildAxis, Step::NodeTest(Step::NodeTest::NameTest, localName), namespaceURI, *$2);
+            $$ = new Step(Step::ChildAxis, Step::NodeTest(Step::NodeTest::NameTest, localName, namespaceURI), *$2);
             PARSER->deletePredicateVector($2);
         } else
-            $$ = new Step(Step::ChildAxis, Step::NodeTest(Step::NodeTest::NameTest, localName), namespaceURI);
+            $$ = new Step(Step::ChildAxis, Step::NodeTest(Step::NodeTest::NameTest, localName, namespaceURI));
         PARSER->deleteString($1);
         PARSER->registerParseNode($$);
     }
@@ -227,10 +227,10 @@ Step:
         }
 
         if ($3) {
-            $$ = new Step($1, Step::NodeTest(Step::NodeTest::NameTest, localName), namespaceURI, *$3);
+            $$ = new Step($1, Step::NodeTest(Step::NodeTest::NameTest, localName, namespaceURI), *$3);
             PARSER->deletePredicateVector($3);
         } else
-            $$ = new Step($1, Step::NodeTest(Step::NodeTest::NameTest, localName), namespaceURI);
+            $$ = new Step($1, Step::NodeTest(Step::NodeTest::NameTest, localName, namespaceURI));
         PARSER->deleteString($2);
         PARSER->registerParseNode($$);
     }
index 35445bab89a3d75d63b6dc4b744896c7aa798a24..35f1a224ca62fcf25780880f096cd544d5257d50 100644 (file)
@@ -141,7 +141,7 @@ void LocationPath::optimizeStepPair(unsigned index)
 
         Step* second = m_steps[index + 1];
         if (second->axis() == Step::ChildAxis
-            && second->namespaceURI().isEmpty()
+            && second->nodeTest().namespaceURI().isEmpty()
             && second->nodeTest().kind() == Step::NodeTest::NameTest
             && second->nodeTest().data() == "*") {
 
index 11dbb8ff9a9196784b093446c579b021d317511e..5a04c0e851a4463dc5e5953f8e3b88b6e5cbcc91 100644 (file)
@@ -46,14 +46,6 @@ Step::Step(Axis axis, const NodeTest& nodeTest, const Vector<Predicate*>& predic
 {
 }
 
-Step::Step(Axis axis, const NodeTest& nodeTest, const String& namespaceURI, const Vector<Predicate*>& predicates)
-    : m_axis(axis)
-    , m_nodeTest(nodeTest)
-    , m_namespaceURI(namespaceURI)
-    , m_predicates(predicates)
-{
-}
-
 Step::~Step()
 {
     deleteAllValues(m_predicates);
@@ -192,6 +184,14 @@ NodeSet Step::nodesInAxis(Node* context) const
             if (context->nodeType() != Node::ELEMENT_NODE)
                 return nodes;
 
+            // Avoid lazily creating attribute nodes for attributes that we do not need anyway.
+            if (m_nodeTest.kind() == NodeTest::NameTest && m_nodeTest.data() != "*") {
+                RefPtr<Node> n = static_cast<Element*>(context)->getAttributeNodeNS(m_nodeTest.namespaceURI(), m_nodeTest.data());
+                if (n && n->namespaceURI() != "http://www.w3.org/2000/xmlns/") // In XPath land, namespace nodes are not accessible on the attribute axis.
+                    nodes.append(n.release());
+                return nodes;
+            }
+            
             NamedAttrMap* attrs = context->attributes();
             if (!attrs)
                 return nodes;
@@ -259,26 +259,35 @@ bool Step::nodeMatches(Node* node) const
             return true;
         case NodeTest::NameTest: {
             const String& name = m_nodeTest.data();
-            if (name == "*")
-                return node->nodeType() == primaryNodeType(m_axis) && (m_namespaceURI.isEmpty() || m_namespaceURI == node->namespaceURI());
+            const String& namespaceURI = m_nodeTest.namespaceURI();
 
             if (m_axis == AttributeAxis) {
+                ASSERT(node->isAttributeNode());
+
                 // In XPath land, namespace nodes are not accessible on the attribute axis.
-                if (name == "xmlns")
+                if (node->namespaceURI() == "http://www.w3.org/2000/xmlns/")
                     return false;
 
-                // FIXME: check the namespace!
-                return node->nodeName() == name;
-            } else if (m_axis == NamespaceAxis) {
+                if (name == "*")
+                    return namespaceURI.isEmpty() || node->namespaceURI() == namespaceURI;
+
+                return node->localName() == name && node->namespaceURI() == namespaceURI;
+            }
+
+            if (m_axis == NamespaceAxis) {
                 // Node test on the namespace axis is not implemented yet
-            } else {
-                // We use tagQName here because we don't want the element name in uppercase 
-                // like we get with HTML elements.
-                // Paths without namespaces should match HTML elements in HTML documents despite those having an XHTML namespace.
-                return node->nodeType() == Node::ELEMENT_NODE
-                        && static_cast<Element*>(node)->tagQName().localName() == name
-                        && ((node->isHTMLElement() && node->document()->isHTMLDocument() && m_namespaceURI.isNull()) || m_namespaceURI == node->namespaceURI());
+                return false;
             }
+            
+            if (name == "*")
+                return node->nodeType() == primaryNodeType(m_axis) && (namespaceURI.isEmpty() || namespaceURI == node->namespaceURI());
+
+            // We use tagQName here because we don't want the element name in uppercase 
+            // like we get with HTML elements.
+            // Paths without namespaces should match HTML elements in HTML documents despite those having an XHTML namespace.
+            return node->nodeType() == Node::ELEMENT_NODE
+                    && static_cast<Element*>(node)->tagQName().localName() == name
+                    && ((node->isHTMLElement() && node->document()->isHTMLDocument() && namespaceURI.isNull()) || namespaceURI == node->namespaceURI());
         }
     }
     ASSERT_NOT_REACHED();
index 479534730ad8140135f63ef37e20deb8d96b83c9..6874f0ad200dc5b4badca8c8ebc71c5ed486d45e 100644 (file)
@@ -56,32 +56,31 @@ namespace WebCore {
                     ElementNodeTest // XPath 2.0
                 };
                 
-                NodeTest(Kind kind, const String& data = String()) : m_kind(kind), m_data(data) {}
+                NodeTest(Kind kind) : m_kind(kind) {}
+                NodeTest(Kind kind, const String& data) : m_kind(kind), m_data(data) {}
+                NodeTest(Kind kind, const String& data, const String& namespaceURI) : m_kind(kind), m_data(data), m_namespaceURI(namespaceURI) {}
                 
                 Kind kind() const { return m_kind; }
                 const String data() const { return m_data; }
+                const String namespaceURI() const { return m_namespaceURI; }
                 
             private:
                 Kind m_kind;
                 String m_data;
+                String m_namespaceURI;
             };
 
             Step(Axis, const NodeTest& nodeTest, const Vector<Predicate*>& predicates = Vector<Predicate*>());
-            Step(Axis, const NodeTest& nodeTest, const String& namespaceURI, const Vector<Predicate*>& predicates = Vector<Predicate*>());
             ~Step();
 
             NodeSet evaluate(Node* context) const;
             
             Axis axis() const { return m_axis; }
             NodeTest nodeTest() const { return m_nodeTest; }
-            const String& nodeTestData() const { return m_nodeTestData; }
-            const String& namespaceURI() const { return m_namespaceURI; }
             const Vector<Predicate*>& predicates() const { return m_predicates; }
             
             void setAxis(Axis axis) { m_axis = axis; }
             void setNodeTest(NodeTest nodeTest) { m_nodeTest = nodeTest; }
-            void setNodeTestData(const String& nodeTestData) { m_nodeTestData = nodeTestData; }
-            void setNamespaceURI(const String& namespaceURI) { m_namespaceURI = namespaceURI; }
             void setPredicates(const Vector<Predicate*>& predicates) { m_predicates = predicates; }
             
         private:
@@ -93,8 +92,6 @@ namespace WebCore {
 
             Axis m_axis;
             NodeTest m_nodeTest;
-            String m_nodeTestData;
-            String m_namespaceURI;
             Vector<Predicate*> m_predicates;
         };