Diff
Modified: trunk/LayoutTests/ChangeLog (204367 => 204368)
--- trunk/LayoutTests/ChangeLog 2016-08-11 02:08:17 UTC (rev 204367)
+++ trunk/LayoutTests/ChangeLog 2016-08-11 02:21:42 UTC (rev 204368)
@@ -1,3 +1,16 @@
+2016-08-10 Chris Dumez <cdu...@apple.com>
+
+ Optimization in Node.insertBefore() is not spec-compliant
+ https://bugs.webkit.org/show_bug.cgi?id=160746
+
+ Reviewed by Ryosuke Niwa.
+
+ Add layout test to make sure mutation observers / listeners are always
+ notified when Node.insertBefore() is called.
+
+ * fast/dom/Node/insertBefore-no-op-mutationobserver-expected.txt: Added.
+ * fast/dom/Node/insertBefore-no-op-mutationobserver.html: Added.
+
2016-08-10 Ryosuke Niwa <rn...@webkit.org>
Move document.defineElement to customElements.define
Added: trunk/LayoutTests/fast/dom/Node/insertBefore-no-op-mutationobserver-expected.txt (0 => 204368)
--- trunk/LayoutTests/fast/dom/Node/insertBefore-no-op-mutationobserver-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/dom/Node/insertBefore-no-op-mutationobserver-expected.txt 2016-08-11 02:21:42 UTC (rev 204368)
@@ -0,0 +1,13 @@
+Tests that calls of insertBefore() lead to mutation events even if they are no-ops.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Mutation observer was notified
+PASS wasMutationEventListenerCalled is true
+PASS Mutation observer was notified
+PASS wasMutationEventListenerCalled is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/dom/Node/insertBefore-no-op-mutationobserver.html (0 => 204368)
--- trunk/LayoutTests/fast/dom/Node/insertBefore-no-op-mutationobserver.html (rev 0)
+++ trunk/LayoutTests/fast/dom/Node/insertBefore-no-op-mutationobserver.html 2016-08-11 02:21:42 UTC (rev 204368)
@@ -0,0 +1,57 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<div id="testDiv"><p></p><p></p></div>
+<script>
+description("Tests that calls of insertBefore() lead to mutation events even if they are no-ops.");
+jsTestIsAsync = true;
+
+var testDiv = document.getElementById("testDiv");
+var p1 = testDiv.firstChild;
+var p2 = testDiv.lastChild;
+
+var operations = [
+ function() { testDiv.insertBefore(p1, p1); },
+ function() { testDiv.insertBefore(p1, p2); }
+];
+var currentOperation = 0;
+
+function testMutationObserver()
+{
+ var config = { childList: true };
+ var observer = new MutationObserver(function(mutations) {
+ testPassed("Mutation observer was notified");
+ observer.disconnect();
+ testMutationEventListener();
+ });
+
+ observer.observe(testDiv, config);
+ operations[currentOperation]();
+}
+
+function subtreeModifiedHandler()
+{
+ wasMutationEventListenerCalled = true;
+}
+
+function testMutationEventListener()
+{
+ wasMutationEventListenerCalled = false;
+ testDiv.addEventListener("DOMSubtreeModified", subtreeModifiedHandler);
+ operations[currentOperation]();
+ shouldBeTrue("wasMutationEventListenerCalled");
+ testDiv.removeEventListener("DOMSubtreeModified", subtreeModifiedHandler);
+
+ currentOperation++;
+ if (currentOperation < operations.length)
+ testMutationObserver();
+ else
+ finishJSTest();
+}
+
+testMutationObserver();
+</script>
+<script src=""
+</body>
+</html>
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (204367 => 204368)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2016-08-11 02:08:17 UTC (rev 204367)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2016-08-11 02:21:42 UTC (rev 204368)
@@ -1,3 +1,14 @@
+2016-08-10 Chris Dumez <cdu...@apple.com>
+
+ Optimization in Node.insertBefore() is not spec-compliant
+ https://bugs.webkit.org/show_bug.cgi?id=160746
+
+ Reviewed by Ryosuke Niwa.
+
+ Rebaseline W3C test now that more checks are passing.
+
+ * web-platform-tests/dom/ranges/Range-mutations-expected.txt:
+
2016-08-09 Chris Dumez <cdu...@apple.com>
Optimization in Node.replaceChild() is not spec-compliant
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/dom/ranges/Range-mutations-expected.txt (204367 => 204368)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/dom/ranges/Range-mutations-expected.txt 2016-08-11 02:08:17 UTC (rev 204367)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/dom/ranges/Range-mutations-expected.txt 2016-08-11 02:21:42 UTC (rev 204368)
@@ -5507,18 +5507,12 @@
FAIL detachedXmlComment.nodeValue += "foo", with selected range collapsed at (detachedXmlComment, detachedXmlComment.length) assert_equals: Sanity check: selection must have exactly one range after adding the range expected 1 but got 0
PASS detachedXmlComment.nodeValue += detachedXmlComment.nodeValue, with unselected range collapsed at (detachedXmlComment, detachedXmlComment.length)
FAIL detachedXmlComment.nodeValue += detachedXmlComment.nodeValue, with selected range collapsed at (detachedXmlComment, detachedXmlComment.length) assert_equals: Sanity check: selection must have exactly one range after adding the range expected 1 but got 0
-FAIL testDiv.insertBefore(paras[0], paras[1]), with unselected range collapsed at (paras[0], 0) assert_equals: Wrong start container expected Element node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
-</p><p id="b" s... but got Element node <p id="a">Äb̈c̈d̈ëf̈g̈ḧ
-</p>
+PASS testDiv.insertBefore(paras[0], paras[1]), with unselected range collapsed at (paras[0], 0)
FAIL testDiv.insertBefore(paras[0], paras[1]), with selected range collapsed at (paras[0], 0) assert_equals: Sanity check: selection's range must initially be the same as the range we added expected object "" but got object ""
-FAIL testDiv.insertBefore(paras[0], paras[1]), with unselected range on paras[0] from 0 to 1 assert_equals: Wrong start container expected Element node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
-</p><p id="b" s... but got Element node <p id="a">Äb̈c̈d̈ëf̈g̈ḧ
-</p>
+PASS testDiv.insertBefore(paras[0], paras[1]), with unselected range on paras[0] from 0 to 1
FAIL testDiv.insertBefore(paras[0], paras[1]), with selected range on paras[0] from 0 to 1 assert_equals: Sanity check: selection's range must initially be the same as the range we added expected object "Äb̈c̈d̈ëf̈g̈ḧ
" but got object "Äb̈c̈d̈ëf̈g̈ḧ"
-FAIL testDiv.insertBefore(paras[0], paras[1]), with unselected range collapsed at (paras[0], 1) assert_equals: Wrong start container expected Element node <div id="test"><p id="a">Äb̈c̈d̈ëf̈g̈ḧ
-</p><p id="b" s... but got Element node <p id="a">Äb̈c̈d̈ëf̈g̈ḧ
-</p>
+PASS testDiv.insertBefore(paras[0], paras[1]), with unselected range collapsed at (paras[0], 1)
FAIL testDiv.insertBefore(paras[0], paras[1]), with selected range collapsed at (paras[0], 1) assert_equals: Sanity check: selection's range must initially be the same as the range we added expected object "" but got object ""
PASS testDiv.insertBefore(paras[0], paras[1]), with unselected range on testDiv from 0 to 2
FAIL testDiv.insertBefore(paras[0], paras[1]), with selected range on testDiv from 0 to 2 assert_equals: Sanity check: selection's range must initially be the same as the range we added expected object "Äb̈c̈d̈ëf̈g̈ḧ
@@ -5526,9 +5520,9 @@
" but got object "Äb̈c̈d̈ëf̈g̈ḧ
Ijklmnop
"
-FAIL testDiv.insertBefore(paras[0], paras[1]), with unselected range collapsed at (testDiv, 1) assert_equals: Wrong start offset expected 0 but got 1
+PASS testDiv.insertBefore(paras[0], paras[1]), with unselected range collapsed at (testDiv, 1)
FAIL testDiv.insertBefore(paras[0], paras[1]), with selected range collapsed at (testDiv, 1) assert_equals: Sanity check: selection's range must initially be the same as the range we added expected object "" but got object ""
-FAIL testDiv.insertBefore(paras[0], paras[1]), with unselected range on testDiv from 1 to 2 assert_equals: Wrong start offset expected 0 but got 1
+PASS testDiv.insertBefore(paras[0], paras[1]), with unselected range on testDiv from 1 to 2
FAIL testDiv.insertBefore(paras[0], paras[1]), with selected range on testDiv from 1 to 2 assert_equals: Sanity check: selection's range must initially be the same as the range we added expected object "Ijklmnop
" but got object ""
PASS testDiv.insertBefore(paras[0], paras[1]), with unselected range collapsed at (testDiv, 2)
Modified: trunk/Source/WebCore/ChangeLog (204367 => 204368)
--- trunk/Source/WebCore/ChangeLog 2016-08-11 02:08:17 UTC (rev 204367)
+++ trunk/Source/WebCore/ChangeLog 2016-08-11 02:21:42 UTC (rev 204368)
@@ -1,3 +1,40 @@
+2016-08-10 Chris Dumez <cdu...@apple.com>
+
+ Optimization in Node.insertBefore() is not spec-compliant
+ https://bugs.webkit.org/show_bug.cgi?id=160746
+
+ Reviewed by Ryosuke Niwa.
+
+ We have an optimization in Node.insertBefore(newNode, refChild) to avoid
+ doing any work when newNode == refChild or newNode.nextSibling == refChild.
+
+ This optimization is not in the specification:
+ - https://dom.spec.whatwg.org/#concept-node-replace
+
+ The issue is that this optimization is observable with DOM mutation
+ observers / listeners or DOM ranges.
+
+ This patch addresses the issue by dropping the optimization. This case
+ does not seem common enough to be worth optimizing for. However, if it
+ turns out to regress the performance of things we care about, we could
+ fallback to doing the optimization only when it is not observable.
+
+ Test: fast/dom/Node/insertBefore-no-op-mutationobserver.html
+
+ * dom/ContainerNode.cpp:
+ (WebCore::checkAcceptChild):
+ Move refChild->parent() == parent check from insertBefore() to our
+ pre-insertion check function, right after checking if newNode contains
+ parent. This was done in order to match more closely the specification
+ and to make sure that exception are thrown in the correct order:
+ - https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity (steps 2 and 3)
+
+ (WebCore::ContainerNode::insertBefore):
+ 1. Drop the (refChild->previousSibling() == &newChild || refChild == &newChild)
+ optimization as it is not spc-compliant.
+ 2. If refChild is newNode, then set refChild to newChild.nextSibling as per:
+ - https://dom.spec.whatwg.org/#concept-node-pre-insert (step 3)
+
2016-08-10 Ryosuke Niwa <rn...@webkit.org>
Move document.defineElement to customElements.define
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (204367 => 204368)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2016-08-11 02:08:17 UTC (rev 204367)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2016-08-11 02:21:42 UTC (rev 204368)
@@ -183,6 +183,8 @@
ASSERT(isChildTypeAllowed(newParent, newChild));
if (containsConsideringHostElements(newChild, newParent))
return HIERARCHY_REQUEST_ERR;
+ if (operation == Document::AcceptChildOperation::InsertOrAdd && refChild && refChild->parentNode() != &newParent)
+ return NOT_FOUND_ERR;
return 0;
}
@@ -194,6 +196,9 @@
if (containsConsideringHostElements(newChild, newParent))
return HIERARCHY_REQUEST_ERR;
+ if (operation == Document::AcceptChildOperation::InsertOrAdd && refChild && refChild->parentNode() != &newParent)
+ return NOT_FOUND_ERR;
+
if (is<Document>(newParent)) {
if (!downcast<Document>(newParent).canAcceptChild(newChild, refChild, operation))
return HIERARCHY_REQUEST_ERR;
@@ -235,27 +240,20 @@
// If it is, it can be deleted as a side effect of sending mutation events.
ASSERT(refCount() || parentOrShadowHostNode());
- Ref<ContainerNode> protectedThis(*this);
-
ec = 0;
- // insertBefore(node, 0) is equivalent to appendChild(node)
- if (!refChild)
- return appendChild(newChild, ec);
-
// Make sure adding the new child is OK.
if (!ensurePreInsertionValidity(newChild, refChild, ec))
return false;
- // NOT_FOUND_ERR: Raised if refChild is not a child of this node
- if (refChild->parentNode() != this) {
- ec = NOT_FOUND_ERR;
- return false;
- }
+ if (refChild == &newChild)
+ refChild = newChild.nextSibling();
- if (refChild->previousSibling() == &newChild || refChild == &newChild) // nothing to do
- return true;
+ // insertBefore(node, null) is equivalent to appendChild(node)
+ if (!refChild)
+ return appendChildWithoutPreInsertionValidityCheck(newChild, ec);
+ Ref<ContainerNode> protectedThis(*this);
Ref<Node> next(*refChild);
NodeVector targets;
@@ -640,8 +638,6 @@
bool ContainerNode::appendChild(Node& newChild, ExceptionCode& ec)
{
- Ref<ContainerNode> protectedThis(*this);
-
// Check that this node is not "floating".
// If it is, it can be deleted as a side effect of sending mutation events.
ASSERT(refCount() || parentOrShadowHostNode());
@@ -652,6 +648,13 @@
if (!ensurePreInsertionValidity(newChild, nullptr, ec))
return false;
+ return appendChildWithoutPreInsertionValidityCheck(newChild, ec);
+}
+
+bool ContainerNode::appendChildWithoutPreInsertionValidityCheck(Node& newChild, ExceptionCode& ec)
+{
+ Ref<ContainerNode> protectedThis(*this);
+
NodeVector targets;
collectChildrenAndRemoveFromOldParent(newChild, targets, ec);
if (ec)
Modified: trunk/Source/WebCore/dom/ContainerNode.h (204367 => 204368)
--- trunk/Source/WebCore/dom/ContainerNode.h 2016-08-11 02:08:17 UTC (rev 204367)
+++ trunk/Source/WebCore/dom/ContainerNode.h 2016-08-11 02:21:42 UTC (rev 204368)
@@ -117,6 +117,7 @@
private:
void removeBetween(Node* previousChild, Node* nextChild, Node& oldChild);
+ bool appendChildWithoutPreInsertionValidityCheck(Node&, ExceptionCode&);
void insertBeforeCommon(Node& nextChild, Node& oldChild);
void appendChildCommon(Node&);
