- Revision
- 113267
- Author
- [email protected]
- Date
- 2012-04-04 17:05:24 -0700 (Wed, 04 Apr 2012)
Log Message
Delay post-insertion notifications until new DOM tree is complete
https://bugs.webkit.org/show_bug.cgi?id=82631
Reviewed by Ojan Vafai.
Source/WebCore:
When inserting a DocumentFragment, WebKit previously would update both
internal WebCore state and mutation event listeners after each node
was inserted. This is inconsistent not only with DOM4, but also
with (at least) Firefox and IE. Given the many bugs over the years in
WebKit due to this behavior, it seems better to delay notification
until the fragment is completely inserted.
The changes to the three core mutation methods below are similar:
the only logic remaining in the loop is checking that insertion is
possible and taking care of that insertion. The entire loop is then
wrapped in forbidEventDispatch/allowEventDispatch, effectively
asserting that none of the code inside will have side effects.
The one bit of logic added to the loop is resizing the targets
vector down to the set of nodes actually inserted as part of the
loop. This makes it possible to simply pass the vector on to
notifyChildrenInserted without having to also pass along a count of
actually-inserted nodes.
As for the code that used to live inside the loop that could have
side-effects, or depended on those side-effects, it has been moved
out, either above (the check that the refChild is still valid in
insertBefore) or after (the calls to notifyChildrenInserted).
Finally, it was necessary to retrofit ChildListMutationScope to take a
vector of added nodes instead of a single node at a time, due to the
assertions in isAddedNodeInOrder (now renamed to be plural). Note that
there is now a single call to ChildListMutationScope::childrenAdded,
inside notifyChildrenInserted.
Test: fast/events/domnodeinserted-entire-fragment.html
* dom/ChildListMutationScope.cpp:
(ChildListMutationScope::MutationAccumulator): Renamed method to be plural.
(WebCore::ChildListMutationScope::MutationAccumulator::areAddedNodesInOrder): Handle a NodeVector instead of a Node.
(WebCore::ChildListMutationScope::MutationAccumulator::childrenAdded): Handle adding a NodeVector instead of a Node.
(WebCore::ChildListMutationScope::MutationAccumulationRouter::childrenAdded): Renamed to be plural, pass NodeVector through.
* dom/ChildListMutationScope.h:
(WebCore::ChildListMutationScope::childrenAdded): ditto.
(MutationAccumulationRouter):
* dom/ContainerNode.cpp:
(WebCore): Renamed updateTreeAfterInsertion to notifyChildrenInserted.
(WebCore::ContainerNode::insertBefore): See main ChangeLog explanation.
(WebCore::ContainerNode::replaceChild): ditto.
(WebCore::ContainerNode::appendChild): ditto.
(WebCore::dispatchChildInsertionEvents): Remove MutationObserver handling.
(WebCore::notifyChildrenInserted): Handle a NodeVector of all inserted children,
and take on responsiblity for MutationObserver handling as well as dispatchSubtreeModifiedEvent.
LayoutTests:
* fast/events/domnodeinserted-entire-fragment-expected.txt: Added.
* fast/events/domnodeinserted-entire-fragment.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (113266 => 113267)
--- trunk/LayoutTests/ChangeLog 2012-04-05 00:03:04 UTC (rev 113266)
+++ trunk/LayoutTests/ChangeLog 2012-04-05 00:05:24 UTC (rev 113267)
@@ -1,3 +1,13 @@
+2012-04-04 Adam Klein <[email protected]>
+
+ Delay post-insertion notifications until new DOM tree is complete
+ https://bugs.webkit.org/show_bug.cgi?id=82631
+
+ Reviewed by Ojan Vafai.
+
+ * fast/events/domnodeinserted-entire-fragment-expected.txt: Added.
+ * fast/events/domnodeinserted-entire-fragment.html: Added.
+
2012-04-04 Chris Rogers <[email protected]>
RealtimeAnalyserNode should support smaller analysis sizes
Added: trunk/LayoutTests/fast/events/domnodeinserted-entire-fragment-expected.txt (0 => 113267)
--- trunk/LayoutTests/fast/events/domnodeinserted-entire-fragment-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/domnodeinserted-entire-fragment-expected.txt 2012-04-05 00:05:24 UTC (rev 113267)
@@ -0,0 +1,12 @@
+DOMNodeInserted events should be fired after all nodes in a DocumentFragment are inserted.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS container.childNodes.length is 3
+PASS container.childNodes.length is 3
+PASS container.childNodes.length is 3
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/events/domnodeinserted-entire-fragment.html (0 => 113267)
--- trunk/LayoutTests/fast/events/domnodeinserted-entire-fragment.html (rev 0)
+++ trunk/LayoutTests/fast/events/domnodeinserted-entire-fragment.html 2012-04-05 00:05:24 UTC (rev 113267)
@@ -0,0 +1,16 @@
+<div id=container></div>
+<script src=""
+<script>
+description('DOMNodeInserted events should be fired after all nodes in a DocumentFragment are inserted.');
+
+var fragment = document.createDocumentFragment();
+fragment.appendChild(document.createElement('a'));
+fragment.appendChild(document.createElement('b'));
+fragment.appendChild(document.createElement('i'));
+var container = document.getElementById('container');
+container.addEventListener('DOMNodeInserted', function(evt) {
+ shouldBe('container.childNodes.length', '3');
+});
+container.appendChild(fragment);
+</script>
+<script src=""
Modified: trunk/Source/WebCore/ChangeLog (113266 => 113267)
--- trunk/Source/WebCore/ChangeLog 2012-04-05 00:03:04 UTC (rev 113266)
+++ trunk/Source/WebCore/ChangeLog 2012-04-05 00:05:24 UTC (rev 113267)
@@ -1,3 +1,59 @@
+2012-04-04 Adam Klein <[email protected]>
+
+ Delay post-insertion notifications until new DOM tree is complete
+ https://bugs.webkit.org/show_bug.cgi?id=82631
+
+ Reviewed by Ojan Vafai.
+
+ When inserting a DocumentFragment, WebKit previously would update both
+ internal WebCore state and mutation event listeners after each node
+ was inserted. This is inconsistent not only with DOM4, but also
+ with (at least) Firefox and IE. Given the many bugs over the years in
+ WebKit due to this behavior, it seems better to delay notification
+ until the fragment is completely inserted.
+
+ The changes to the three core mutation methods below are similar:
+ the only logic remaining in the loop is checking that insertion is
+ possible and taking care of that insertion. The entire loop is then
+ wrapped in forbidEventDispatch/allowEventDispatch, effectively
+ asserting that none of the code inside will have side effects.
+
+ The one bit of logic added to the loop is resizing the targets
+ vector down to the set of nodes actually inserted as part of the
+ loop. This makes it possible to simply pass the vector on to
+ notifyChildrenInserted without having to also pass along a count of
+ actually-inserted nodes.
+
+ As for the code that used to live inside the loop that could have
+ side-effects, or depended on those side-effects, it has been moved
+ out, either above (the check that the refChild is still valid in
+ insertBefore) or after (the calls to notifyChildrenInserted).
+
+ Finally, it was necessary to retrofit ChildListMutationScope to take a
+ vector of added nodes instead of a single node at a time, due to the
+ assertions in isAddedNodeInOrder (now renamed to be plural). Note that
+ there is now a single call to ChildListMutationScope::childrenAdded,
+ inside notifyChildrenInserted.
+
+ Test: fast/events/domnodeinserted-entire-fragment.html
+
+ * dom/ChildListMutationScope.cpp:
+ (ChildListMutationScope::MutationAccumulator): Renamed method to be plural.
+ (WebCore::ChildListMutationScope::MutationAccumulator::areAddedNodesInOrder): Handle a NodeVector instead of a Node.
+ (WebCore::ChildListMutationScope::MutationAccumulator::childrenAdded): Handle adding a NodeVector instead of a Node.
+ (WebCore::ChildListMutationScope::MutationAccumulationRouter::childrenAdded): Renamed to be plural, pass NodeVector through.
+ * dom/ChildListMutationScope.h:
+ (WebCore::ChildListMutationScope::childrenAdded): ditto.
+ (MutationAccumulationRouter):
+ * dom/ContainerNode.cpp:
+ (WebCore): Renamed updateTreeAfterInsertion to notifyChildrenInserted.
+ (WebCore::ContainerNode::insertBefore): See main ChangeLog explanation.
+ (WebCore::ContainerNode::replaceChild): ditto.
+ (WebCore::ContainerNode::appendChild): ditto.
+ (WebCore::dispatchChildInsertionEvents): Remove MutationObserver handling.
+ (WebCore::notifyChildrenInserted): Handle a NodeVector of all inserted children,
+ and take on responsiblity for MutationObserver handling as well as dispatchSubtreeModifiedEvent.
+
2012-04-04 Chris Rogers <[email protected]>
RealtimeAnalyserNode should support smaller analysis sizes
Modified: trunk/Source/WebCore/dom/ChildListMutationScope.cpp (113266 => 113267)
--- trunk/Source/WebCore/dom/ChildListMutationScope.cpp 2012-04-05 00:03:04 UTC (rev 113266)
+++ trunk/Source/WebCore/dom/ChildListMutationScope.cpp 2012-04-05 00:05:24 UTC (rev 113267)
@@ -55,14 +55,14 @@
MutationAccumulator(PassRefPtr<Node>, PassOwnPtr<MutationObserverInterestGroup> observers);
~MutationAccumulator();
- void childAdded(PassRefPtr<Node>);
+ void childrenAdded(const NodeVector&);
void willRemoveChild(PassRefPtr<Node>);
void enqueueMutationRecord();
private:
void clear();
bool isEmpty();
- bool isAddedNodeInOrder(Node*);
+ bool areAddedNodesInOrder(const NodeVector&);
bool isRemovedNodeInOrder(Node*);
RefPtr<Node> m_target;
@@ -87,26 +87,27 @@
ASSERT(isEmpty());
}
-inline bool ChildListMutationScope::MutationAccumulator::isAddedNodeInOrder(Node* child)
+inline bool ChildListMutationScope::MutationAccumulator::areAddedNodesInOrder(const NodeVector& children)
{
- return isEmpty() || (m_lastAdded == child->previousSibling() && m_nextSibling == child->nextSibling());
+ return isEmpty() || (m_lastAdded == children.first()->previousSibling() && m_nextSibling == children.last()->nextSibling());
}
-void ChildListMutationScope::MutationAccumulator::childAdded(PassRefPtr<Node> prpChild)
+void ChildListMutationScope::MutationAccumulator::childrenAdded(const NodeVector& children)
{
- RefPtr<Node> child = prpChild;
+ ASSERT(!children.isEmpty());
- ASSERT(isAddedNodeInOrder(child.get()));
- if (!isAddedNodeInOrder(child.get()))
+ if (!areAddedNodesInOrder(children)) {
+ ASSERT_NOT_REACHED();
enqueueMutationRecord();
+ }
if (isEmpty()) {
- m_previousSibling = child->previousSibling();
- m_nextSibling = child->nextSibling();
+ m_previousSibling = children.first()->previousSibling();
+ m_nextSibling = children.last()->nextSibling();
}
- m_addedNodes.append(child);
- m_lastAdded = child;
+ m_addedNodes.append(children);
+ m_lastAdded = children.last();
}
inline bool ChildListMutationScope::MutationAccumulator::isRemovedNodeInOrder(Node* child)
@@ -189,14 +190,14 @@
return s_instance;
}
-void ChildListMutationScope::MutationAccumulationRouter::childAdded(Node* target, Node* child)
+void ChildListMutationScope::MutationAccumulationRouter::childrenAdded(Node* target, const NodeVector& children)
{
HashMap<Node*, OwnPtr<ChildListMutationScope::MutationAccumulator> >::iterator iter = m_accumulations.find(target);
ASSERT(iter != m_accumulations.end());
if (iter == m_accumulations.end() || !iter->second)
return;
- iter->second->childAdded(child);
+ iter->second->childrenAdded(children);
}
void ChildListMutationScope::MutationAccumulationRouter::willRemoveChild(Node* target, Node* child)
Modified: trunk/Source/WebCore/dom/ChildListMutationScope.h (113266 => 113267)
--- trunk/Source/WebCore/dom/ChildListMutationScope.h 2012-04-05 00:03:04 UTC (rev 113266)
+++ trunk/Source/WebCore/dom/ChildListMutationScope.h 2012-04-05 00:05:24 UTC (rev 113267)
@@ -33,6 +33,7 @@
#if ENABLE(MUTATION_OBSERVERS)
+#include "ContainerNode.h"
#include "Document.h"
#include "Node.h"
#include "WebKitMutationObserver.h"
@@ -58,10 +59,10 @@
MutationAccumulationRouter::instance()->decrementScopingLevel(m_target);
}
- void childAdded(Node* child)
+ void childrenAdded(const NodeVector& children)
{
if (m_target)
- MutationAccumulationRouter::instance()->childAdded(m_target, child);
+ MutationAccumulationRouter::instance()->childrenAdded(m_target, children);
}
void willRemoveChild(Node* child)
@@ -83,7 +84,7 @@
void incrementScopingLevel(Node*);
void decrementScopingLevel(Node*);
- void childAdded(Node* target, Node* child);
+ void childrenAdded(Node* target, const NodeVector& children);
void willRemoveChild(Node* target, Node* child);
private:
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (113266 => 113267)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2012-04-05 00:03:04 UTC (rev 113266)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2012-04-05 00:05:24 UTC (rev 113267)
@@ -50,7 +50,7 @@
static void notifyChildInserted(Node*);
static void dispatchChildInsertionEvents(Node*);
static void dispatchChildRemovalEvents(Node*);
-static void updateTreeAfterInsertion(ContainerNode*, Node*, bool shouldLazyAttach);
+static void notifyChildrenInserted(PassRefPtr<ContainerNode>, const NodeVector&, bool shouldLazyAttach);
typedef pair<RefPtr<Node>, unsigned> CallbackParameters;
typedef pair<NodeCallback, CallbackParameters> CallbackInfo;
@@ -121,7 +121,7 @@
// If it is, it can be deleted as a side effect of sending mutation events.
ASSERT(refCount() || parentOrHostNode());
- RefPtr<Node> protect(this);
+ RefPtr<ContainerNode> protect(this);
ec = 0;
@@ -152,34 +152,33 @@
if (targets.isEmpty())
return true;
+ // Due to arbitrary code running in response to a DOM mutation event it's
+ // possible that "next" is no longer a child of "this".
+ if (next->parentNode() != this)
+ return true;
+
#if ENABLE(INSPECTOR)
InspectorInstrumentation::willInsertDOMNode(document(), this);
#endif
-#if ENABLE(MUTATION_OBSERVERS)
- ChildListMutationScope mutation(this);
-#endif
+ forbidEventDispatch();
+ for (size_t i = 0; i < targets.size(); ++i) {
+ Node* child = targets[i].get();
- for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
- Node* child = it->get();
-
// Due to arbitrary code running in response to a DOM mutation event it's
- // possible that "next" is no longer a child of "this".
- // It's also possible that "child" has been inserted elsewhere.
- // In either of those cases, we'll just stop.
- if (next->parentNode() != this)
+ // possible that "child" has been inserted elsewhere.
+ if (child->parentNode()) {
+ targets.resize(i);
break;
- if (child->parentNode())
- break;
+ }
treeScope()->adoptIfNeeded(child);
insertBeforeCommon(next.get(), child);
-
- updateTreeAfterInsertion(this, child, shouldLazyAttach);
}
+ allowEventDispatch();
- dispatchSubtreeModifiedEvent();
+ notifyChildrenInserted(protect.release(), targets, shouldLazyAttach);
return true;
}
@@ -240,7 +239,7 @@
// If it is, it can be deleted as a side effect of sending mutation events.
ASSERT(refCount() || parentOrHostNode());
- RefPtr<Node> protect(this);
+ RefPtr<ContainerNode> protect(this);
ec = 0;
@@ -278,37 +277,36 @@
if (ec)
return false;
+ // Due to arbitrary code running in response to a DOM mutation event it's
+ // possible that "next" is no longer a child of "this".
+ if (next && next->parentNode() != this)
+ return true;
+
#if ENABLE(INSPECTOR)
InspectorInstrumentation::willInsertDOMNode(document(), this);
#endif
- // Add the new child(ren)
- for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
- Node* child = it->get();
+ forbidEventDispatch();
+ for (size_t i = 0; i < targets.size(); ++i) {
+ Node* child = targets[i].get();
// Due to arbitrary code running in response to a DOM mutation event it's
- // possible that "next" is no longer a child of "this".
- // It's also possible that "child" has been inserted elsewhere.
- // In either of those cases, we'll just stop.
- if (next && next->parentNode() != this)
+ // possible that "child" has been inserted elsewhere.
+ if (child->parentNode()) {
+ targets.resize(i);
break;
- if (child->parentNode())
- break;
+ }
treeScope()->adoptIfNeeded(child);
- // Add child before "next".
- forbidEventDispatch();
if (next)
insertBeforeCommon(next.get(), child);
else
appendChildToContainer(child, this);
- allowEventDispatch();
-
- updateTreeAfterInsertion(this, child, shouldLazyAttach);
}
+ allowEventDispatch();
- dispatchSubtreeModifiedEvent();
+ notifyChildrenInserted(protect.release(), targets, shouldLazyAttach);
return true;
}
@@ -562,31 +560,25 @@
InspectorInstrumentation::willInsertDOMNode(document(), this);
#endif
-#if ENABLE(MUTATION_OBSERVERS)
- ChildListMutationScope mutation(this);
-#endif
+ forbidEventDispatch();
+ for (size_t i = 0; i < targets.size(); ++i) {
+ Node* child = targets[i].get();
- // Now actually add the child(ren)
- for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
- Node* child = it->get();
-
// If the child has a parent again, just stop what we're doing, because
// that means someone is doing something with DOM mutation -- can't re-parent
// a child that already has a parent.
- if (child->parentNode())
+ if (child->parentNode()) {
+ targets.resize(i);
break;
+ }
treeScope()->adoptIfNeeded(child);
- // Append child to the end of the list
- forbidEventDispatch();
appendChildToContainer(child, this);
- allowEventDispatch();
-
- updateTreeAfterInsertion(this, child, shouldLazyAttach);
}
+ allowEventDispatch();
- dispatchSubtreeModifiedEvent();
+ notifyChildrenInserted(protect.release(), targets, shouldLazyAttach);
return true;
}
@@ -1037,13 +1029,6 @@
RefPtr<Node> c = child;
RefPtr<Document> document = child->document();
-#if ENABLE(MUTATION_OBSERVERS)
- if (c->parentNode()) {
- ChildListMutationScope mutation(c->parentNode());
- mutation.childAdded(c.get());
- }
-#endif
-
if (c->parentNode() && document->hasListenerType(Document::DOMNODEINSERTED_LISTENER))
c->dispatchScopedEvent(MutationEvent::create(eventNames().DOMNodeInsertedEvent, true, c->parentNode()));
@@ -1087,25 +1072,36 @@
}
}
-static void updateTreeAfterInsertion(ContainerNode* parent, Node* child, bool shouldLazyAttach)
+static void notifyChildrenInserted(PassRefPtr<ContainerNode> parent, const NodeVector& children, bool shouldLazyAttach)
{
- ASSERT(parent->refCount());
- ASSERT(child->refCount());
+ if (children.isEmpty())
+ return;
- parent->childrenChanged(false, child->previousSibling(), child->nextSibling(), 1);
+#if ENABLE(MUTATION_OBSERVERS)
+ ChildListMutationScope(parent.get()).childrenAdded(children);
+#endif
- notifyChildInserted(child);
+ parent->childrenChanged(false, children.first()->previousSibling(), children.last()->nextSibling(), children.size());
- // FIXME: Attachment should be the first operation in this function, but some code
- // (for example, HTMLFormControlElement's autofocus support) requires this ordering.
- if (parent->attached() && !child->attached() && child->parentNode() == parent) {
- if (shouldLazyAttach)
- child->lazyAttach();
- else
- child->attach();
+ for (size_t i = 0; i < children.size(); ++i) {
+ Node* child = children[i].get();
+ notifyChildInserted(child);
+
+ // FIXME: Attachment should be the first operation in this function, but some code
+ // (for example, HTMLFormControlElement's autofocus support) requires this ordering.
+ if (parent->attached() && !child->attached() && child->parentNode() == parent) {
+ if (shouldLazyAttach)
+ child->lazyAttach();
+ else
+ child->attach();
+ }
+
+ // Now that the child is attached to the render tree, dispatch
+ // the relevant mutation events.
+ dispatchChildInsertionEvents(child);
}
- dispatchChildInsertionEvents(child);
+ parent->dispatchSubtreeModifiedEvent();
}
} // namespace WebCore
