- Revision
- 113230
- Author
- ad...@chromium.org
- Date
- 2012-04-04 13:10:34 -0700 (Wed, 04 Apr 2012)
Log Message
Web Inspector: break on DOM node insertion only once per operation, not once per inserted node
https://bugs.webkit.org/show_bug.cgi?id=82967
Reviewed by Ojan Vafai.
Source/WebCore:
This change affects the case where a DocumentFragment is inserted,
rather than a single node. This is most common when using innerHTML:
the effect of the change is that inserting, e.g., '<input><input>',
the SubtreeModified breakpoint will be hit once, rather than twice
(once for each input element). Given that the particular node being
inserted wasn't exposed as part of the breakpoint, this seems strictly
better.
Now that the call to willInsertDOMNode is outside the loop, there's
not an obvious node to pass in as the new child. Luckily, InspectorDOMDebuggerAgent
already ignored that argument, so it's simply been removed from the signature.
This changes paves the way to do only tree-modification work, and no
external notifications, inside the loops in appendChild/insertBefore/replaceChild.
* dom/ContainerNode.cpp:
(WebCore::ContainerNode::insertBefore): Hoisted call to willInsertDOMNode out of loop.
(WebCore::ContainerNode::replaceChild): ditto.
(WebCore::ContainerNode::appendChild): ditto.
* inspector/InspectorDOMDebuggerAgent.cpp:
(WebCore::InspectorDOMDebuggerAgent::willInsertDOMNode): Removed first argument (now takes only the parent).
* inspector/InspectorDOMDebuggerAgent.h:
(InspectorDOMDebuggerAgent):
* inspector/InspectorInstrumentation.cpp:
(WebCore::InspectorInstrumentation::willInsertDOMNodeImpl): Removed second argument.
* inspector/InspectorInstrumentation.h:
(InspectorInstrumentation):
(WebCore::InspectorInstrumentation::willInsertDOMNode): Removed second argument.
LayoutTests:
Added test for setting inner HTML and ensuring that only a single
breakpoint is hit.
* inspector/debugger/dom-breakpoints.html:
* platform/chromium/inspector/debugger/dom-breakpoints-expected.txt:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (113229 => 113230)
--- trunk/LayoutTests/ChangeLog 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/LayoutTests/ChangeLog 2012-04-04 20:10:34 UTC (rev 113230)
@@ -1,3 +1,16 @@
+2012-04-04 Adam Klein <ad...@chromium.org>
+
+ Web Inspector: break on DOM node insertion only once per operation, not once per inserted node
+ https://bugs.webkit.org/show_bug.cgi?id=82967
+
+ Reviewed by Ojan Vafai.
+
+ Added test for setting inner HTML and ensuring that only a single
+ breakpoint is hit.
+
+ * inspector/debugger/dom-breakpoints.html:
+ * platform/chromium/inspector/debugger/dom-breakpoints-expected.txt:
+
2012-04-04 Jeffrey Pfau <jp...@apple.com>
Move pending sheet removal from ~HTMLLinkElement to removal from document.
Modified: trunk/LayoutTests/inspector/debugger/dom-breakpoints.html (113229 => 113230)
--- trunk/LayoutTests/inspector/debugger/dom-breakpoints.html 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/LayoutTests/inspector/debugger/dom-breakpoints.html 2012-04-04 20:10:34 UTC (rev 113230)
@@ -44,6 +44,17 @@
element.parentNode.removeChild(element);
}
+function setInnerHTML(elementId, html)
+{
+ var element = document.getElementById(elementId);
+ element.innerHTML = html;
+}
+
+function breakDebugger()
+{
+ debugger;
+}
+
function test()
{
WebInspector.inspectorView.setCurrentPanel(WebInspector.panels.elements);
@@ -80,12 +91,28 @@
InspectorTest.addResult("Test that 'Subtree Modified' breakpoint is hit when removing a child.");
InspectorTest.evaluateInPageWithTimeout("removeElement('grandchildElement')");
InspectorTest.addResult("Remove grandchildElement.");
+ waitUntilPausedAndDumpStack(next);
+ },
+
+ function testInnerHTML(next)
+ {
+ InspectorTest.addResult("Test that 'Subtree Modified' breakpoint is hit exactly once when setting innerHTML.");
+ InspectorTest.evaluateInPageWithTimeout("setInnerHTML('childElement', '<br><br>')");
+ InspectorTest.addResult("Set childElement.innerHTML.");
waitUntilPausedAndDumpStack(step2);
function step2()
{
+ InspectorTest.waitUntilPaused(step3);
+ InspectorTest.evaluateInPageWithTimeout("breakDebugger()");
+ InspectorTest.addResult("Call breakDebugger, expect it to show up in next stack trace.");
+ }
+
+ function step3(frames)
+ {
+ InspectorTest.captureStackTrace(frames);
pane._removeBreakpoint(rootElement, pane._breakpointTypes.SubtreeModified);
- next();
+ InspectorTest.resumeExecution(next);
}
},
Modified: trunk/LayoutTests/platform/chromium/inspector/debugger/dom-breakpoints-expected.txt (113229 => 113230)
--- trunk/LayoutTests/platform/chromium/inspector/debugger/dom-breakpoints-expected.txt 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/LayoutTests/platform/chromium/inspector/debugger/dom-breakpoints-expected.txt 2012-04-04 20:10:34 UTC (rev 113230)
@@ -33,6 +33,22 @@
Paused on a "Subtree Modified" breakpoint set on div#rootElement, because its descendant div#grandchildElement was removed.
Script execution resumed.
+Running: testInnerHTML
+Test that 'Subtree Modified' breakpoint is hit exactly once when setting innerHTML.
+Set childElement.innerHTML.
+Script execution paused.
+Call stack:
+ 0) setInnerHTML (dom-breakpoints.html:50)
+ 1) (:1)
+Paused on a "Subtree Modified" breakpoint set on div#rootElement, because a new child was added to its descendant div#childElement.
+Script execution resumed.
+Call breakDebugger, expect it to show up in next stack trace.
+Script execution paused.
+Call stack:
+ 0) breakDebugger (dom-breakpoints.html:55)
+ 1) (:1)
+Script execution resumed.
+
Running: testModifyAttribute
Test that 'Attribute Modified' breakpoint is hit when modifying attribute.
Set 'Attribute Modified' DOM breakpoint on rootElement.
Modified: trunk/Source/WebCore/ChangeLog (113229 => 113230)
--- trunk/Source/WebCore/ChangeLog 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/Source/WebCore/ChangeLog 2012-04-04 20:10:34 UTC (rev 113230)
@@ -1,3 +1,39 @@
+2012-04-04 Adam Klein <ad...@chromium.org>
+
+ Web Inspector: break on DOM node insertion only once per operation, not once per inserted node
+ https://bugs.webkit.org/show_bug.cgi?id=82967
+
+ Reviewed by Ojan Vafai.
+
+ This change affects the case where a DocumentFragment is inserted,
+ rather than a single node. This is most common when using innerHTML:
+ the effect of the change is that inserting, e.g., '<input><input>',
+ the SubtreeModified breakpoint will be hit once, rather than twice
+ (once for each input element). Given that the particular node being
+ inserted wasn't exposed as part of the breakpoint, this seems strictly
+ better.
+
+ Now that the call to willInsertDOMNode is outside the loop, there's
+ not an obvious node to pass in as the new child. Luckily, InspectorDOMDebuggerAgent
+ already ignored that argument, so it's simply been removed from the signature.
+
+ This changes paves the way to do only tree-modification work, and no
+ external notifications, inside the loops in appendChild/insertBefore/replaceChild.
+
+ * dom/ContainerNode.cpp:
+ (WebCore::ContainerNode::insertBefore): Hoisted call to willInsertDOMNode out of loop.
+ (WebCore::ContainerNode::replaceChild): ditto.
+ (WebCore::ContainerNode::appendChild): ditto.
+ * inspector/InspectorDOMDebuggerAgent.cpp:
+ (WebCore::InspectorDOMDebuggerAgent::willInsertDOMNode): Removed first argument (now takes only the parent).
+ * inspector/InspectorDOMDebuggerAgent.h:
+ (InspectorDOMDebuggerAgent):
+ * inspector/InspectorInstrumentation.cpp:
+ (WebCore::InspectorInstrumentation::willInsertDOMNodeImpl): Removed second argument.
+ * inspector/InspectorInstrumentation.h:
+ (InspectorInstrumentation):
+ (WebCore::InspectorInstrumentation::willInsertDOMNode): Removed second argument.
+
2012-04-04 Jeffrey Pfau <jp...@apple.com>
Move pending sheet removal from ~HTMLLinkElement to removal from document.
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (113229 => 113230)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2012-04-04 20:10:34 UTC (rev 113230)
@@ -152,6 +152,10 @@
if (targets.isEmpty())
return true;
+#if ENABLE(INSPECTOR)
+ InspectorInstrumentation::willInsertDOMNode(document(), this);
+#endif
+
#if ENABLE(MUTATION_OBSERVERS)
ChildListMutationScope mutation(this);
#endif
@@ -168,10 +172,6 @@
if (child->parentNode())
break;
-#if ENABLE(INSPECTOR)
- InspectorInstrumentation::willInsertDOMNode(document(), child, this);
-#endif
-
treeScope()->adoptIfNeeded(child);
insertBeforeCommon(next.get(), child);
@@ -278,6 +278,10 @@
if (ec)
return false;
+#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();
@@ -291,10 +295,6 @@
if (child->parentNode())
break;
-#if ENABLE(INSPECTOR)
- InspectorInstrumentation::willInsertDOMNode(document(), child, this);
-#endif
-
treeScope()->adoptIfNeeded(child);
// Add child before "next".
@@ -558,6 +558,10 @@
if (targets.isEmpty())
return true;
+#if ENABLE(INSPECTOR)
+ InspectorInstrumentation::willInsertDOMNode(document(), this);
+#endif
+
#if ENABLE(MUTATION_OBSERVERS)
ChildListMutationScope mutation(this);
#endif
@@ -572,10 +576,6 @@
if (child->parentNode())
break;
-#if ENABLE(INSPECTOR)
- InspectorInstrumentation::willInsertDOMNode(document(), child, this);
-#endif
-
treeScope()->adoptIfNeeded(child);
// Append child to the end of the list
Modified: trunk/Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp (113229 => 113230)
--- trunk/Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/Source/WebCore/inspector/InspectorDOMDebuggerAgent.cpp 2012-04-04 20:10:34 UTC (rev 113230)
@@ -267,7 +267,7 @@
}
}
-void InspectorDOMDebuggerAgent::willInsertDOMNode(Node*, Node* parent)
+void InspectorDOMDebuggerAgent::willInsertDOMNode(Node* parent)
{
if (hasBreakpoint(parent, SubtreeModified)) {
RefPtr<InspectorObject> eventData = InspectorObject::create();
Modified: trunk/Source/WebCore/inspector/InspectorDOMDebuggerAgent.h (113229 => 113230)
--- trunk/Source/WebCore/inspector/InspectorDOMDebuggerAgent.h 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/Source/WebCore/inspector/InspectorDOMDebuggerAgent.h 2012-04-04 20:10:34 UTC (rev 113230)
@@ -72,7 +72,7 @@
virtual void removeDOMBreakpoint(ErrorString*, int nodeId, const String& type);
// InspectorInstrumentation API
- void willInsertDOMNode(Node*, Node* parent);
+ void willInsertDOMNode(Node* parent);
void didInvalidateStyleAttr(Node*);
void didInsertDOMNode(Node*);
void willRemoveDOMNode(Node*);
Modified: trunk/Source/WebCore/inspector/InspectorInstrumentation.cpp (113229 => 113230)
--- trunk/Source/WebCore/inspector/InspectorInstrumentation.cpp 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/Source/WebCore/inspector/InspectorInstrumentation.cpp 2012-04-04 20:10:34 UTC (rev 113230)
@@ -123,11 +123,11 @@
return false;
}
-void InspectorInstrumentation::willInsertDOMNodeImpl(InstrumentingAgents* instrumentingAgents, Node* node, Node* parent)
+void InspectorInstrumentation::willInsertDOMNodeImpl(InstrumentingAgents* instrumentingAgents, Node* parent)
{
#if ENABLE(_javascript__DEBUGGER)
if (InspectorDOMDebuggerAgent* domDebuggerAgent = instrumentingAgents->inspectorDOMDebuggerAgent())
- domDebuggerAgent->willInsertDOMNode(node, parent);
+ domDebuggerAgent->willInsertDOMNode(parent);
#endif
}
Modified: trunk/Source/WebCore/inspector/InspectorInstrumentation.h (113229 => 113230)
--- trunk/Source/WebCore/inspector/InspectorInstrumentation.h 2012-04-04 20:06:35 UTC (rev 113229)
+++ trunk/Source/WebCore/inspector/InspectorInstrumentation.h 2012-04-04 20:10:34 UTC (rev 113230)
@@ -84,7 +84,7 @@
static void didClearWindowObjectInWorld(Frame*, DOMWrapperWorld*);
static bool isDebuggerPaused(Frame*);
- static void willInsertDOMNode(Document*, Node*, Node* parent);
+ static void willInsertDOMNode(Document*, Node* parent);
static void didInsertDOMNode(Document*, Node*);
static void willRemoveDOMNode(Document*, Node*);
static void willModifyDOMAttr(Document*, Element*, const AtomicString& oldValue, const AtomicString& newValue);
@@ -237,7 +237,7 @@
static void didClearWindowObjectInWorldImpl(InstrumentingAgents*, Frame*, DOMWrapperWorld*);
static bool isDebuggerPausedImpl(InstrumentingAgents*);
- static void willInsertDOMNodeImpl(InstrumentingAgents*, Node*, Node* parent);
+ static void willInsertDOMNodeImpl(InstrumentingAgents*, Node* parent);
static void didInsertDOMNodeImpl(InstrumentingAgents*, Node*);
static void willRemoveDOMNodeImpl(InstrumentingAgents*, Node*);
static void didRemoveDOMNodeImpl(InstrumentingAgents*, Node*);
@@ -406,12 +406,12 @@
return false;
}
-inline void InspectorInstrumentation::willInsertDOMNode(Document* document, Node* node, Node* parent)
+inline void InspectorInstrumentation::willInsertDOMNode(Document* document, Node* parent)
{
#if ENABLE(INSPECTOR)
FAST_RETURN_IF_NO_FRONTENDS(void());
if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForDocument(document))
- willInsertDOMNodeImpl(instrumentingAgents, node, parent);
+ willInsertDOMNodeImpl(instrumentingAgents, parent);
#endif
}
