Node::setTextContent should avoid work when nothing changes

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

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169805

[eseidel, 2014-03-15 00:00:02]

I verified that the test does print repaint rects before my change.

[Julien - ping for review, 2014-03-15 01:12:50]

lgtm!

https://codereview.chromium.org/200763006/diff/1/LayoutTests/fast/repaint/set...
File LayoutTests/fast/repaint/set-text-content-same.html (right):

https://codereview.chromium.org/200763006/diff/1/LayoutTests/fast/repaint/set...
LayoutTests/fast/repaint/set-text-content-same.html:5: <script
type="text/javascript">
I usually tell people to remove these attribute as they don't give much.

[eseidel, 2014-03-15 01:13:53]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-15 01:13:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/200763006/1

[commit-bot: I haz the power, 2014-03-15 02:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-15 02:00:43]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[eseidel, 2014-03-15 07:15:41]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-15 07:15:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/200763006/1

[commit-bot: I haz the power, 2014-03-15 08:02:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-15 08:02:51]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[eseidel, 2014-03-17 21:54:10]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-17 21:54:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/200763006/1

[eseidel, 2014-03-17 21:54:50]

Committed patchset #1 manually as r169394 (presubmit successful).

[Julien - ping for review, 2014-03-18 22:55:54]

lgtm

https://codereview.chromium.org/200763006/diff/20001/Source/core/editing/mark...
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/200763006/diff/20001/Source/core/editing/mark...
Source/core/editing/markup.cpp:1009: // FIXME: This is wrong if
containerNode->firstChild() has more than one ref!
I am not sure I understand this comment. All references should point to the same
Node or is it something else?

[eseidel, 2014-03-18 23:10:11]

Changing the data on the text-node is visible from JS.

<div>foo</div>
<script>
var oldText = div.firstChild;
console.log(oldText.data); // foo
div.innerText = "bar";
console.log(oldText.data); // bar!?!
</script>

I remember this came up long ago.  This was a benchmark optimization which was
made to avoid needing to always alloc a new Text node every time we call
setInnerText... but I chose not to match that optimization with
Node::setTextContenet, 1. because it's not in the spec, and 2. it's confusing.
:)

[Julien - ping for review, 2014-03-20 17:12:00]

On 2014/03/18 23:10:11, eseidel wrote:
> Changing the data on the text-node is visible from JS.
> 
> <div>foo</div>
> <script>
> var oldText = div.firstChild;
> console.log(oldText.data); // foo
> div.innerText = "bar";
> console.log(oldText.data); // bar!?!
> </script>
> 
> I remember this came up long ago.  This was a benchmark optimization which was
> made to avoid needing to always alloc a new Text node every time we call
> setInnerText... but I chose not to match that optimization with
> Node::setTextContenet, 1. because it's not in the spec, and 2. it's confusing.
> :)

I think you should reference this optimization (and why we shouldn't do it),
which would make the comment more understandable for people who were not
involved in it :-)

[eseidel, 2014-03-20 20:45:59]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:46:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/200763006/40001

[commit-bot: I haz the power, 2014-03-20 20:47:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:47:49]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[eseidel, 2014-03-22 19:56:55]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-22 19:56:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/200763006/60001

[commit-bot: I haz the power, 2014-03-22 19:58:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-22 19:58:28]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[eseidel, 2014-03-22 20:00:40]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-22 20:00:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/200763006/60001

[commit-bot: I haz the power, 2014-03-22 20:54:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-22 20:54:33]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[eseidel, 2014-03-22 22:38:10]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-22 22:38:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/200763006/60001

[commit-bot: I haz the power, 2014-03-22 23:08:25]

Change committed as 169805