Skip to content

GitLab

T
tangled
Commit f0b795d9 authored by eseidel@chromium.org's avatar eseidel@chromium.org
Browse files
Options

Node::setTextContent should avoid work when nothing changes 

We already had code to handle this case, which is shared
by editing as well as HTMLElement::setInnerText.
However that code (replaceChildrenWithText) appears to
be both wrong and unsafe (modifies existing text nodes
without checking if they're shared or not), so I chose
not to re-use it for now.

This was originally committed as:
https://src.chromium.org/viewvc/blink?view=rev&revision=169394
using replaceChildrenWithText, but was rolled out for
breaking a zillion tests. This version does not use
replaceChildrenWithText.
BUG=352836
R=jchaffraix@chromium.org

Review URL: https://codereview.chromium.org/200763006

git-svn-id: svn://svn.chromium.org/blink/trunk@169805 bbb929c8-8fbe-4397-9dbb-9b2b20218538
parent 10ca134a
Hide whitespace changes
Inline Side-by-side
Showing with 35 additions and 1 deletion
third_party/WebKit/LayoutTests/fast/dom/HTMLObjectElement/beforeload-set-text-crash.xhtml
View file @ f0b795d9
...@@ -14,7 +14,8 @@ function setText() { ...@@ -14,7 +14,8 @@ function setText() {
finishJSTest(); finishJSTest();
} }
gc(); // Because we are recursively entering into setText, can't gc() after this command. gc(); // Because we are recursively entering into setText, can't gc() after this command.
document.getElementById("object").textContent = "A"; // textContent will ignore setting identical text, so use count.
document.getElementById("object").textContent = count;
} }
document.execCommand("SelectAll"); document.execCommand("SelectAll");
document.getElementById("object").textContent = "A"; document.getElementById("object").textContent = "A";
......
third_party/WebKit/LayoutTests/fast/repaint/set-text-content-same.html 0 → 100644
View file @ f0b795d9
<!DOCTYPE html>
<body onload="runRepaintTest();">
<pre id="target">PASS if does not repaint.</pre>
<script src="resources/text-based-repaint.js" type="text/javascript"></script>
<script type="text/javascript">
function repaintTest()
{
target.textContent = "PASS if does not repaint.";
}
</script>
third_party/WebKit/Source/core/dom/Node.cpp
View file @ f0b795d9
...@@ -59,6 +59,7 @@ ...@@ -59,6 +59,7 @@
#include "core/dom/shadow/InsertionPoint.h" #include "core/dom/shadow/InsertionPoint.h"
#include "core/dom/shadow/ShadowRoot.h" #include "core/dom/shadow/ShadowRoot.h"
#include "core/editing/htmlediting.h" #include "core/editing/htmlediting.h"
#include "core/editing/markup.h"
#include "core/events/BeforeLoadEvent.h" #include "core/events/BeforeLoadEvent.h"
#include "core/events/Event.h" #include "core/events/Event.h"
#include "core/events/EventDispatchMediator.h" #include "core/events/EventDispatchMediator.h"
...@@ -1496,9 +1497,15 @@ void Node::setTextContent(const String& text) ...@@ -1496,9 +1497,15 @@ void Node::setTextContent(const String& text)
case ELEMENT_NODE: case ELEMENT_NODE:
case ATTRIBUTE_NODE: case ATTRIBUTE_NODE:
case DOCUMENT_FRAGMENT_NODE: { case DOCUMENT_FRAGMENT_NODE: {
// FIXME: Merge this logic into replaceChildrenWithText.
RefPtr<ContainerNode> container = toContainerNode(this); RefPtr<ContainerNode> container = toContainerNode(this);
// No need to do anything if the text is identical.
if (container->hasOneTextChild() && toText(container->firstChild())->data() == text)
return;
ChildListMutationScope mutation(*this); ChildListMutationScope mutation(*this);
container->removeChildren(); container->removeChildren();
// Note: This API will not insert empty text nodes:
// http://dom.spec.whatwg.org/#dom-node-textcontent
if (!text.isEmpty()) if (!text.isEmpty())
container->appendChild(document().createTextNode(text), ASSERT_NO_EXCEPTION); container->appendChild(document().createTextNode(text), ASSERT_NO_EXCEPTION);
return; return;
......
third_party/WebKit/Source/core/editing/markup.cpp
View file @ f0b795d9
...@@ -1006,11 +1006,13 @@ void replaceChildrenWithFragment(ContainerNode* container, PassRefPtr<DocumentFr ...@@ -1006,11 +1006,13 @@ void replaceChildrenWithFragment(ContainerNode* container, PassRefPtr<DocumentFr
return; return;
} }
// FIXME: This is wrong if containerNode->firstChild() has more than one ref!
if (containerNode->hasOneTextChild() && fragment->hasOneTextChild()) { if (containerNode->hasOneTextChild() && fragment->hasOneTextChild()) {
toText(containerNode->firstChild())->setData(toText(fragment->firstChild())->data()); toText(containerNode->firstChild())->setData(toText(fragment->firstChild())->data());
return; return;
} }
// FIXME: No need to replace the child it is a text node and its contents are already == text.
if (containerNode->hasOneChild()) { if (containerNode->hasOneChild()) {
containerNode->replaceChild(fragment, containerNode->firstChild(), exceptionState); containerNode->replaceChild(fragment, containerNode->firstChild(), exceptionState);
return; return;
...@@ -1027,13 +1029,25 @@ void replaceChildrenWithText(ContainerNode* container, const String& text, Excep ...@@ -1027,13 +1029,25 @@ void replaceChildrenWithText(ContainerNode* container, const String& text, Excep
ChildListMutationScope mutation(*containerNode); ChildListMutationScope mutation(*containerNode);
// FIXME: This is wrong if containerNode->firstChild() has more than one ref! Example:
// <div>foo</div>
// <script>
// var oldText = div.firstChild;
// console.log(oldText.data); // foo
// div.innerText = "bar";
// console.log(oldText.data); // bar!?!
// </script>
// I believe this is an intentional benchmark cheat from years ago.
// We should re-visit if we actually want this still.
if (containerNode->hasOneTextChild()) { if (containerNode->hasOneTextChild()) {
toText(containerNode->firstChild())->setData(text); toText(containerNode->firstChild())->setData(text);
return; return;
} }
// NOTE: This method currently always creates a text node, even if that text node will be empty.
RefPtr<Text> textNode = Text::create(containerNode->document(), text); RefPtr<Text> textNode = Text::create(containerNode->document(), text);
// FIXME: No need to replace the child it is a text node and its contents are already == text.
if (containerNode->hasOneChild()) { if (containerNode->hasOneChild()) {
containerNode->replaceChild(textNode.release(), containerNode->firstChild(), exceptionState); containerNode->replaceChild(textNode.release(), containerNode->firstChild(), exceptionState);
return; return;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment