Document title changed twice when setting document.title
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jan 2017 00:14:37 +0000 (00:14 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jan 2017 00:14:37 +0000 (00:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167065

Reviewed by Darin Adler.

Source/WebCore:

Setting document.title would call the document title to be set twice
first to the empty string and then to the new title. This is because
setting document.title is equivalent to setting title.textContent [1],
which first removes all children and then inserts the new one [2], and
we call updateTitle() for each step. This is because
HTMLTitleElement::childrenChanged() is called twice (once for the
removal of the existing children, and a second time when the new child
is inserted), and childrenChanged() calls document::titleElementTextChanged().

Since no JS event is fired between those 2 mutations, it is safe (i.e. non
observable from JS) to update the title only once after both mutations have
taken place. To achieve this, add a new replaceAllChildren() function
which implements [3]. This replaceAllChildren() has the benefit of
calling ContainerNode::childrenChanged() only once, after both mutations
have taken place, thus avoiding unnecessary work. This fixes the issue
when setting the title and should be performance-positive in general.

[1] https://html.spec.whatwg.org/#document.title
[2] https://dom.spec.whatwg.org/#dom-node-textcontent
[3] https://dom.spec.whatwg.org/#concept-node-replace-all

Test: fast/dom/Node/textContent-mutationEvents.html

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::notifyChildInserted):
(WebCore::ContainerNode::removeChild):
(WebCore::ContainerNode::replaceAllChildren):
(WebCore::ContainerNode::rebuildSVGExtensionsElementsIfNecessary):
(WebCore::ContainerNode::removeChildren):
(WebCore::ContainerNode::updateTreeAfterInsertion):
* dom/ContainerNode.h:
Add new replaceAllChildrenWith() function which implements [3]
in a way that calls ContainerNode::childrenChanged() only once.

* dom/Element.cpp:
(WebCore::Element::childrenChanged):
Deal with new AllChildrenReplaced ChildChange type.

* dom/Node.cpp:
(WebCore::Node::setTextContent):
Call replaceAllChildrenWith() as per the specification:
- https://dom.spec.whatwg.org/#dom-node-textcontent

* dom/Range.cpp:
(WebCore::Range::surroundContents):
Call replaceAllChildrenWith(nullptr) as per the specification:
- https://dom.spec.whatwg.org/#dom-range-surroundcontents

Tools:

Add WebKit2GTK API test that was written by Michael Catanzaro.

* TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:
(testWebViewTitleChange):
(beforeAll):

LayoutTests:

* fast/dom/Node/textContent-mutationEvents-expected.txt: Added.
* fast/dom/Node/textContent-mutationEvents.html: Added.
Add layout test to make sure that the mutation events are properly
fired when setting Node.textContent.

* fast/dom/title-text-property-2-expected.txt:
* fast/dom/title-text-property-2.html:
* fast/dom/title-text-property-expected.txt:
* http/tests/globalhistory/history-delegate-basic-title-expected.txt:
Update / rebaseline existing tests now that we no longer temporarily
reset document.title to the empty string when overriding the title.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/Node/textContent-mutationEvents-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Node/textContent-mutationEvents.html [new file with mode: 0644]
LayoutTests/fast/dom/title-text-property-2-expected.txt
LayoutTests/fast/dom/title-text-property-2.html
LayoutTests/fast/dom/title-text-property-expected.txt
LayoutTests/http/tests/globalhistory/history-delegate-basic-title-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/ContainerNode.h
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/Range.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp

index 6b3f420..4766d9a 100644 (file)
@@ -1,3 +1,22 @@
+2017-01-17  Chris Dumez  <cdumez@apple.com>
+
+        Document title changed twice when setting document.title
+        https://bugs.webkit.org/show_bug.cgi?id=167065
+
+        Reviewed by Darin Adler.
+
+        * fast/dom/Node/textContent-mutationEvents-expected.txt: Added.
+        * fast/dom/Node/textContent-mutationEvents.html: Added.
+        Add layout test to make sure that the mutation events are properly
+        fired when setting Node.textContent.
+
+        * fast/dom/title-text-property-2-expected.txt:
+        * fast/dom/title-text-property-2.html:
+        * fast/dom/title-text-property-expected.txt:
+        * http/tests/globalhistory/history-delegate-basic-title-expected.txt:
+        Update / rebaseline existing tests now that we no longer temporarily
+        reset document.title to the empty string when overriding the title.
+
 2017-01-17  Zalan Bujtas  <zalan@apple.com>
 
         Editing nested RTL-LTR content makes the process unresponsive.
diff --git a/LayoutTests/fast/dom/Node/textContent-mutationEvents-expected.txt b/LayoutTests/fast/dom/Node/textContent-mutationEvents-expected.txt
new file mode 100644 (file)
index 0000000..9caa9af
--- /dev/null
@@ -0,0 +1,44 @@
+Tests that the DOM mutation events are fired correctly when Node.textContent is set.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+title.textContent = 'New title';
+
+* DOMNodeRemoved fired
+PASS firedDOMNodeRemoved is false
+PASS firedDOMNodeInserted is false
+PASS firedDOMSubtreeModified is false
+PASS document.title is "Original title"
+
+* DOMNodeInserted fired
+PASS firedDOMNodeRemoved is true
+PASS firedDOMNodeInserted is false
+PASS firedDOMSubtreeModified is false
+PASS document.title is "New title"
+
+* DOMSubtreeModified fired
+PASS firedDOMNodeRemoved is true
+PASS firedDOMNodeInserted is true
+PASS firedDOMSubtreeModified is false
+PASS document.title is "New title"
+
+PASS firedDOMNodeRemoved is true
+PASS firedDOMNodeInserted is true
+PASS firedDOMSubtreeModified is true
+PASS document.title is "New title"
+PASS firedMutationObserver is false
+
+* Mutation observer invoked
+PASS firedMutationObserver is false
+PASS testMutations.length is 1
+PASS mutation.type is "childList"
+PASS mutation.target is title
+PASS mutation.removedNodes.length is 1
+PASS mutation.removedNodes[0].data is "Original title"
+PASS mutation.addedNodes.length is 1
+PASS mutation.addedNodes[0].data is "New title"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/Node/textContent-mutationEvents.html b/LayoutTests/fast/dom/Node/textContent-mutationEvents.html
new file mode 100644 (file)
index 0000000..960e15a
--- /dev/null
@@ -0,0 +1,79 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Original title</title>
+<script src="../../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+description("Tests that the DOM mutation events are fired correctly when Node.textContent is set.");
+jsTestIsAsync = true;
+
+var firedDOMNodeRemoved = false;
+var firedDOMNodeInserted = false;
+var firedDOMSubtreeModified = false;
+var firedMutationObserver = false;
+
+var title = document.getElementsByTagName("title")[0];
+var observer = new MutationObserver(function(mutations) {
+    debug("");
+    debug("* Mutation observer invoked");
+
+    shouldBeFalse("firedMutationObserver");
+    firedMutationObserver = true;
+
+    testMutations = mutations;
+    shouldBe("testMutations.length", "1");
+    mutation = mutations[0];
+    shouldBeEqualToString("mutation.type", "childList");
+    shouldBe("mutation.target", "title");
+    shouldBe("mutation.removedNodes.length", "1");
+    shouldBeEqualToString("mutation.removedNodes[0].data", "Original title");
+    shouldBe("mutation.addedNodes.length", "1");
+    shouldBeEqualToString("mutation.addedNodes[0].data", "New title");
+
+    finishJSTest();
+});
+observer.observe(title, { childList: true });
+
+title.addEventListener("DOMNodeRemoved", function() {
+    debug("");
+    debug("* DOMNodeRemoved fired");
+    shouldBeFalse("firedDOMNodeRemoved");
+    shouldBeFalse("firedDOMNodeInserted");
+    shouldBeFalse("firedDOMSubtreeModified");
+    firedDOMNodeRemoved = true;
+    shouldBeEqualToString("document.title", "Original title");
+});
+title.addEventListener("DOMNodeInserted", function() {
+    debug("");
+    debug("* DOMNodeInserted fired");
+    shouldBeTrue("firedDOMNodeRemoved");
+    shouldBeFalse("firedDOMNodeInserted");
+    shouldBeFalse("firedDOMSubtreeModified");
+    firedDOMNodeInserted = true;
+    shouldBeEqualToString("document.title", "New title");
+});
+title.addEventListener("DOMSubtreeModified", function() {
+    debug("");
+    debug("* DOMSubtreeModified fired");
+    shouldBeTrue("firedDOMNodeRemoved");
+    shouldBeTrue("firedDOMNodeInserted");
+    shouldBeFalse("firedDOMSubtreeModified");
+    firedDOMSubtreeModified = true;
+    shouldBeEqualToString("document.title", "New title");
+});
+
+evalAndLog("title.textContent = 'New title';");
+
+debug("");
+shouldBeTrue("firedDOMNodeRemoved");
+shouldBeTrue("firedDOMNodeInserted");
+shouldBeTrue("firedDOMSubtreeModified");
+shouldBeEqualToString("document.title", "New title");
+shouldBeFalse("firedMutationObserver");
+
+</script>
+<script src="../../../resources/js-test-post.js"></script>
+</body>
+</html>
index 0911f32..213459e 100644 (file)
@@ -1,6 +1,8 @@
-TITLE CHANGED: ''
-TITLE CHANGED: '1. setting document.title'
-TITLE CHANGED: ''
-TITLE CHANGED: '2. setting title.text'
+CONSOLE MESSAGE: line 10: Setting document.title to TITLE1
+TITLE CHANGED: 'TITLE1'
+CONSOLE MESSAGE: line 15: Setting title element's text to TITLE2
+TITLE CHANGED: 'TITLE2'
+CONSOLE MESSAGE: line 19: Should not set title
+CONSOLE MESSAGE: line 25: Should reset title to the empty string
 TITLE CHANGED: ''
 
index fc979dd..f880967 100644 (file)
@@ -6,19 +6,23 @@ function runTests() {
         testRunner.dumpAsText();
         testRunner.dumpTitleChanges();
     }
-    
-    document.title = '1. setting document.title';
+
+    console.log("Setting document.title to TITLE1");    
+    document.title = 'TITLE1';
 
     title = document.getElementsByTagName('title').item(0);
     
-    title.text = '2. setting title.text';
+    console.log("Setting title element's text to TITLE2");
+    title.text = 'TITLE2';
     
     newTitle = document.createElement('title');
-    newTitle.appendChild(document.createTextNode('3. appending a new title node (should not trigger a title change)'));
+    console.log("Should not set title");
+    newTitle.appendChild(document.createTextNode('FAIL'));
     
     document.getElementsByTagName('head').item(0).appendChild(newTitle);
-    
+   
     // Remove both title elements
+    console.log("Should reset title to the empty string");
     titleElements = document.getElementsByTagName('title');
     while (titleElements.length) {
         titleElement = titleElements[titleElements.length - 1];
index aa1b281..0a1ece0 100644 (file)
@@ -1,4 +1,3 @@
-TITLE CHANGED: ''
 TITLE CHANGED: 'This is the new title'
 Original title is: 'Original Title'
 Setting new title to: 'This is the new title'
index 8ec728a..ed25a80 100644 (file)
@@ -1,5 +1,4 @@
 WebView navigated to url "http://127.0.0.1:8000/globalhistory/history-delegate-basic-title.html" with title "" with HTTP equivalent method "GET".  The navigation was successful and was not a client redirect.
 WebView updated the title for history URL "http://127.0.0.1:8000/globalhistory/history-delegate-basic-title.html" to "Test Title 1".
-WebView updated the title for history URL "http://127.0.0.1:8000/globalhistory/history-delegate-basic-title.html" to "".
 WebView updated the title for history URL "http://127.0.0.1:8000/globalhistory/history-delegate-basic-title.html" to "Test Title 2".
 This test sees if the history delegate is notified of title changes.
index 2890a8d..5f9e858 100644 (file)
@@ -1,3 +1,58 @@
+2017-01-17  Chris Dumez  <cdumez@apple.com>
+
+        Document title changed twice when setting document.title
+        https://bugs.webkit.org/show_bug.cgi?id=167065
+
+        Reviewed by Darin Adler.
+
+        Setting document.title would call the document title to be set twice
+        first to the empty string and then to the new title. This is because
+        setting document.title is equivalent to setting title.textContent [1],
+        which first removes all children and then inserts the new one [2], and
+        we call updateTitle() for each step. This is because
+        HTMLTitleElement::childrenChanged() is called twice (once for the
+        removal of the existing children, and a second time when the new child
+        is inserted), and childrenChanged() calls document::titleElementTextChanged().
+
+        Since no JS event is fired between those 2 mutations, it is safe (i.e. non
+        observable from JS) to update the title only once after both mutations have
+        taken place. To achieve this, add a new replaceAllChildren() function
+        which implements [3]. This replaceAllChildren() has the benefit of
+        calling ContainerNode::childrenChanged() only once, after both mutations
+        have taken place, thus avoiding unnecessary work. This fixes the issue
+        when setting the title and should be performance-positive in general.
+
+        [1] https://html.spec.whatwg.org/#document.title
+        [2] https://dom.spec.whatwg.org/#dom-node-textcontent
+        [3] https://dom.spec.whatwg.org/#concept-node-replace-all
+
+        Test: fast/dom/Node/textContent-mutationEvents.html
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::notifyChildInserted):
+        (WebCore::ContainerNode::removeChild):
+        (WebCore::ContainerNode::replaceAllChildren):
+        (WebCore::ContainerNode::rebuildSVGExtensionsElementsIfNecessary):
+        (WebCore::ContainerNode::removeChildren):
+        (WebCore::ContainerNode::updateTreeAfterInsertion):
+        * dom/ContainerNode.h:
+        Add new replaceAllChildrenWith() function which implements [3]
+        in a way that calls ContainerNode::childrenChanged() only once.
+
+        * dom/Element.cpp:
+        (WebCore::Element::childrenChanged):
+        Deal with new AllChildrenReplaced ChildChange type.
+
+        * dom/Node.cpp:
+        (WebCore::Node::setTextContent):
+        Call replaceAllChildrenWith() as per the specification:
+        - https://dom.spec.whatwg.org/#dom-node-textcontent
+
+        * dom/Range.cpp:
+        (WebCore::Range::surroundContents):
+        Call replaceAllChildrenWith(nullptr) as per the specification:
+        - https://dom.spec.whatwg.org/#dom-range-surroundcontents
+
 2017-01-17  Joseph Pecoraro  <pecoraro@apple.com>
 
         ENABLE(USER_TIMING) Not Defined for Apple Windows or OS X Ports
index 7dadebd..cca660e 100644 (file)
@@ -56,6 +56,7 @@
 #include "SVGDocumentExtensions.h"
 #include "SVGElement.h"
 #include "SVGNames.h"
+#include "SVGUseElement.h"
 #include "SelectorQuery.h"
 #include "TemplateContentDocumentFragment.h"
 #include <algorithm>
@@ -327,19 +328,26 @@ void ContainerNode::appendChildCommon(Node& child)
     m_lastChild = &child;
 }
 
-void ContainerNode::notifyChildInserted(Node& child, ChildChangeSource source)
+inline auto ContainerNode::changeForChildInsertion(Node& child, ChildChangeSource source, ReplacedAllChildren replacedAllChildren) -> ChildChange
+{
+    if (replacedAllChildren == ReplacedAllChildren::Yes)
+        return { AllChildrenReplaced, nullptr, nullptr, source };
+
+    return {
+        child.isElementNode() ? ElementInserted : child.isTextNode() ? TextInserted : NonContentsChildChanged,
+        ElementTraversal::previousSibling(child),
+        ElementTraversal::nextSibling(child),
+        source
+    };
+}
+
+void ContainerNode::notifyChildInserted(Node& child, const ChildChange& change)
 {
     ChildListMutationScope(*this).childAdded(child);
 
     NodeVector postInsertionNotificationTargets;
     notifyChildNodeInserted(*this, child, postInsertionNotificationTargets);
 
-    ChildChange change;
-    change.type = child.isElementNode() ? ElementInserted : child.isTextNode() ? TextInserted : NonContentsChildChanged;
-    change.previousSiblingElement = ElementTraversal::previousSibling(child);
-    change.nextSiblingElement = ElementTraversal::nextSibling(child);
-    change.source = source;
-
     childrenChanged(change);
 
     for (auto& target : postInsertionNotificationTargets)
@@ -375,7 +383,7 @@ void ContainerNode::parserInsertBefore(Node& newChild, Node& nextChild)
 
     newChild.updateAncestorConnectedSubframeCountForInsertion();
 
-    notifyChildInserted(newChild, ChildChangeSourceParser);
+    notifyChildInserted(newChild, changeForChildInsertion(newChild, ChildChangeSourceParser));
 }
 
 ExceptionOr<void> ContainerNode::replaceChild(Node& newChild, Node& oldChild)
@@ -534,13 +542,7 @@ ExceptionOr<void> ContainerNode::removeChild(Node& oldChild)
         notifyChildRemoved(child, prev, next, ChildChangeSourceAPI);
     }
 
-
-    if (document().svgExtensions()) {
-        Element* shadowHost = this->shadowHost();
-        if (!shadowHost || !shadowHost->hasTagName(SVGNames::useTag))
-            document().accessSVGExtensions().rebuildElements();
-    }
-
+    rebuildSVGExtensionsElementsIfNecessary();
     dispatchSubtreeModifiedEvent();
 
     return { };
@@ -598,6 +600,65 @@ void ContainerNode::parserRemoveChild(Node& oldChild)
     notifyChildRemoved(oldChild, prev, next, ChildChangeSourceParser);
 }
 
+// https://dom.spec.whatwg.org/#concept-node-replace-all
+void ContainerNode::replaceAllChildren(std::nullptr_t)
+{
+    ChildListMutationScope mutation(*this);
+    removeChildren();
+}
+
+// https://dom.spec.whatwg.org/#concept-node-replace-all
+void ContainerNode::replaceAllChildren(Ref<Node>&& node)
+{
+    // This function assumes the input node is not a DocumentFragment and is parentless to decrease complexity.
+    ASSERT(!is<DocumentFragment>(node));
+    ASSERT(!node->parentNode());
+
+    if (!hasChildNodes()) {
+        // appendChildWithoutPreInsertionValidityCheck() can only throw when node has a parent and we already asserted it doesn't.
+        auto result = appendChildWithoutPreInsertionValidityCheck(node);
+        ASSERT_UNUSED(result, !result.hasException());
+        return;
+    }
+
+    Ref<ContainerNode> protectedThis(*this);
+    ChildListMutationScope mutation(*this);
+
+    // If node is not null, adopt node into parent’s node document.
+    treeScope().adoptIfNeeded(node);
+
+    // Remove all parent's children, in tree order.
+    willRemoveChildren(*this);
+
+    {
+        WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
+        {
+            NoEventDispatchAssertion assertNoEventDispatch;
+            while (RefPtr<Node> child = m_firstChild) {
+                removeBetween(nullptr, child->nextSibling(), *child);
+                notifyChildNodeRemoved(*this, *child);
+            }
+
+            // If node is not null, insert node into parent before null.
+            ASSERT(!ensurePreInsertionValidity(node, nullptr).hasException());
+            InspectorInstrumentation::willInsertDOMNode(document(), *this);
+
+            appendChildCommon(node);
+        }
+
+        updateTreeAfterInsertion(node, ReplacedAllChildren::Yes);
+    }
+
+    rebuildSVGExtensionsElementsIfNecessary();
+    dispatchSubtreeModifiedEvent();
+}
+
+inline void ContainerNode::rebuildSVGExtensionsElementsIfNecessary()
+{
+    if (document().svgExtensions() && !is<SVGUseElement>(shadowHost()))
+        document().accessSVGExtensions().rebuildElements();
+}
+
 // this differs from other remove functions because it forcibly removes all the children,
 // regardless of read-only status or event exceptions, e.g.
 void ContainerNode::removeChildren()
@@ -626,12 +687,7 @@ void ContainerNode::removeChildren()
         childrenChanged(change);
     }
 
-    if (document().svgExtensions()) {
-        Element* shadowHost = this->shadowHost();
-        if (!shadowHost || !shadowHost->hasTagName(SVGNames::useTag))
-            document().accessSVGExtensions().rebuildElements();
-    }
-
+    rebuildSVGExtensionsElementsIfNecessary();
     dispatchSubtreeModifiedEvent();
 }
 
@@ -709,7 +765,7 @@ void ContainerNode::parserAppendChild(Node& newChild)
 
     newChild.updateAncestorConnectedSubframeCountForInsertion();
 
-    notifyChildInserted(newChild, ChildChangeSourceParser);
+    notifyChildInserted(newChild, changeForChildInsertion(newChild, ChildChangeSourceParser));
 }
 
 void ContainerNode::childrenChanged(const ChildChange& change)
@@ -792,11 +848,11 @@ static void dispatchChildRemovalEvents(Node& child)
     }
 }
 
-void ContainerNode::updateTreeAfterInsertion(Node& child)
+void ContainerNode::updateTreeAfterInsertion(Node& child, ReplacedAllChildren replacedAllChildren)
 {
     ASSERT(child.refCount());
 
-    notifyChildInserted(child, ChildChangeSourceAPI);
+    notifyChildInserted(child, changeForChildInsertion(child, ChildChangeSourceAPI, replacedAllChildren));
 
     dispatchChildInsertionEvents(child);
 }
index 39c401f..cbf3542 100644 (file)
@@ -53,6 +53,8 @@ public:
     ExceptionOr<void> replaceChild(Node& newChild, Node& oldChild);
     WEBCORE_EXPORT ExceptionOr<void> removeChild(Node& child);
     WEBCORE_EXPORT ExceptionOr<void> appendChild(Node& newChild);
+    void replaceAllChildren(Ref<Node>&&);
+    void replaceAllChildren(std::nullptr_t);
 
     // These methods are only used during parsing.
     // They don't send DOM mutation events or handle reparenting.
@@ -62,11 +64,12 @@ public:
     void parserInsertBefore(Node& newChild, Node& refChild);
 
     void removeChildren();
+
     void takeAllChildrenFrom(ContainerNode*);
 
     void cloneChildNodes(ContainerNode& clone);
 
-    enum ChildChangeType { ElementInserted, ElementRemoved, TextInserted, TextRemoved, TextChanged, AllChildrenRemoved, NonContentsChildChanged };
+    enum ChildChangeType { ElementInserted, ElementRemoved, TextInserted, TextRemoved, TextChanged, AllChildrenRemoved, NonContentsChildChanged, AllChildrenReplaced };
     enum ChildChangeSource { ChildChangeSourceParser, ChildChangeSourceAPI };
     struct ChildChange {
         ChildChangeType type;
@@ -120,10 +123,13 @@ private:
     void insertBeforeCommon(Node& nextChild, Node& oldChild);
     void appendChildCommon(Node&);
 
-    void notifyChildInserted(Node& child, ChildChangeSource);
+    void notifyChildInserted(Node& child, const ChildChange&);
     void notifyChildRemoved(Node& child, Node* previousSibling, Node* nextSibling, ChildChangeSource);
 
-    void updateTreeAfterInsertion(Node& child);
+    enum class ReplacedAllChildren { No, Yes };
+    void updateTreeAfterInsertion(Node& child, ReplacedAllChildren = ReplacedAllChildren::No);
+    static ChildChange changeForChildInsertion(Node&, ChildChangeSource, ReplacedAllChildren = ReplacedAllChildren::No);
+    void rebuildSVGExtensionsElementsIfNecessary();
 
     bool isContainerNode() const = delete;
 
index 685f913..88e3e99 100644 (file)
@@ -2035,6 +2035,7 @@ void Element::childrenChanged(const ChildChange& change)
             // For elements, we notify shadowRoot in Element::insertedInto and Element::removedFrom.
             break;
         case AllChildrenRemoved:
+        case AllChildrenReplaced:
             shadowRoot->didRemoveAllChildrenOfShadowHost();
             break;
         case TextInserted:
index 6b96d78..bb3ab99 100644 (file)
@@ -1507,12 +1507,12 @@ ExceptionOr<void> Node::setTextContent(const String& text)
         return setNodeValue(text);
     case ELEMENT_NODE:
     case DOCUMENT_FRAGMENT_NODE: {
-        auto container = makeRef(downcast<ContainerNode>(*this));
-        ChildListMutationScope mutation(container);
-        container->removeChildren();
+        auto& container = downcast<ContainerNode>(*this);
         if (text.isEmpty())
-            return { };
-        return container->appendChild(document().createTextNode(text));
+            container.replaceAllChildren(nullptr);
+        else
+            container.replaceAllChildren(document().createTextNode(text));
+        return { };
     }
     case DOCUMENT_NODE:
     case DOCUMENT_TYPE_NODE:
index 330e825..4d3f17b 100644 (file)
@@ -1107,11 +1107,8 @@ ExceptionOr<void> Range::surroundContents(Node& newParent)
         return fragment.releaseException();
 
     // Step 4: If newParent has children, replace all with null within newParent.
-    while (auto* child = newParent.firstChild()) {
-        auto result = downcast<ContainerNode>(newParent).removeChild(*child);
-        if (result.hasException())
-            return result.releaseException();
-    }
+    if (newParent.hasChildNodes())
+        downcast<ContainerNode>(newParent).replaceAllChildren(nullptr);
 
     // Step 5: Insert newParent into context object.
     auto insertResult = insertNode(newParent);
index 0cc5d6e..5ff8cd2 100644 (file)
@@ -1,3 +1,16 @@
+2017-01-17  Chris Dumez  <cdumez@apple.com>
+
+        Document title changed twice when setting document.title
+        https://bugs.webkit.org/show_bug.cgi?id=167065
+
+        Reviewed by Darin Adler.
+
+        Add WebKit2GTK API test that was written by Michael Catanzaro.
+
+        * TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:
+        (testWebViewTitleChange):
+        (beforeAll):
+
 2017-01-17  Joseph Pecoraro  <pecoraro@apple.com>
 
         ENABLE(USER_TIMING) Not Defined for Apple Windows or OS X Ports
index d967307..215a17e 100644 (file)
@@ -947,6 +947,52 @@ static void testWebViewPreferredSize(WebViewTest* test, gconstpointer)
     g_assert_cmpint(naturalSize.height, ==, 615);
 }
 
+class WebViewTitleTest: public WebViewTest {
+public:
+    MAKE_GLIB_TEST_FIXTURE(WebViewTitleTest);
+
+    static void titleChangedCallback(WebKitWebView* view, GParamSpec*, WebViewTitleTest* test)
+    {
+        test->m_webViewTitles.append(webkit_web_view_get_title(view));
+    }
+
+    WebViewTitleTest()
+    {
+        g_signal_connect(m_webView, "notify::title", G_CALLBACK(titleChangedCallback), this);
+    }
+
+    Vector<CString> m_webViewTitles;
+};
+
+static void testWebViewTitleChange(WebViewTitleTest* test, gconstpointer)
+{
+    g_assert_cmpint(test->m_webViewTitles.size(), ==, 0);
+
+    test->loadHtml("<head><title>Page Title</title></head>", nullptr);
+    test->waitUntilLoadFinished();
+    g_assert_cmpint(test->m_webViewTitles.size(), ==, 1);
+    g_assert_cmpstr(test->m_webViewTitles[0].data(), ==, "Page Title");
+
+    test->loadHtml("<head><title>Another Page Title</title></head>", nullptr);
+    test->waitUntilLoadFinished();
+    g_assert_cmpint(test->m_webViewTitles.size(), ==, 3);
+    /* Page title should be immediately unset when loading a new page. */
+    g_assert_cmpstr(test->m_webViewTitles[1].data(), ==, "");
+    g_assert_cmpstr(test->m_webViewTitles[2].data(), ==, "Another Page Title");
+
+    test->loadHtml("<p>This page has no title!</p>", nullptr);
+    test->waitUntilLoadFinished();
+    g_assert_cmpint(test->m_webViewTitles.size(), ==, 4);
+    g_assert_cmpstr(test->m_webViewTitles[3].data(), ==, "");
+
+    test->loadHtml("<script>document.title = 'one'; document.title = 'two'; document.title = 'three';</script>", nullptr);
+    test->waitUntilLoadFinished();
+    g_assert_cmpint(test->m_webViewTitles.size(), ==, 7);
+    g_assert_cmpstr(test->m_webViewTitles[4].data(), ==, "one");
+    g_assert_cmpstr(test->m_webViewTitles[5].data(), ==, "two");
+    g_assert_cmpstr(test->m_webViewTitles[6].data(), ==, "three");
+}
+
 static void serverCallback(SoupServer* server, SoupMessage* message, const char* path, GHashTable*, SoupClientContext*, gpointer)
 {
     if (message->method != SOUP_METHOD_GET) {
@@ -984,6 +1030,7 @@ void beforeAll()
     IsPlayingAudioWebViewTest::add("WebKitWebView", "is-playing-audio", testWebViewIsPlayingAudio);
     WebViewTest::add("WebKitWebView", "background-color", testWebViewBackgroundColor);
     WebViewTest::add("WebKitWebView", "preferred-size", testWebViewPreferredSize);
+    WebViewTitleTest::add("WebKitWebView", "title-change", testWebViewTitleChange);
 }
 
 void afterAll()