Remove the wrong optimization in replacing a text node with innerHTML

Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to this wrong behavior, this optimization also:
- Prevents a slotchange event from being fired because it skips the check.
- Prevents a mutation observer from being notified.

BUG=658605
Committed: https://crrev.com/946f30141fad7e002a44d3545a7d9f45e3cdcbf3
Cr-Commit-Position: refs/heads/master@{#435908}

[hayato, 2016-11-30 03:30:33]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[hayato, 2016-11-30 03:31:07]

Description was changed from

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its bahavior; it does not
use a normal DOM mutation function. We need to check slotchange in this
optimized path too.

BUG=658605
==========

to

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its bahavior; it does not
use a normal DOM mutation function. We need to check slotchange in this
optimized code path too.

BUG=658605
==========

[commit-bot: I haz the power, 2016-11-30 03:31:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-11-30 03:31:33]

hayato@chromium.org changed reviewers:
+ kochi@chromium.org, tkent@chromium.org

[hayato, 2016-11-30 03:31:34]

PTAL

[tkent, 2016-11-30 03:50:25]

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/serializers/Serialization.cpp
(right):

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/serializers/Serialization.cpp:717: if
(containerNode->hasOneTextChild() && fragment->hasOneTextChild()) {
We should remove this block.  This is a wrong optimization.

e.g.

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "udpated".

This is a regression by Oilpan, I think.

[kochi, 2016-11-30 04:20:37]

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/shadow-dom/slotchange.html (right):

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/shadow-dom/slotchange.html:314: <div id="host1">
Not critial, but all these subtrees have duplicate
#host1's in document tree.

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/shadow-dom/slotchange.html:319: </div>
This #test8 subtree isn't used?

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/shadow-dom/slotchange.html:323: const n =
createTestTree(window.test1);
nit: other tests use just 'test1', and no local 'test1' in this function, so
'window.' is unnecessary? It's fine if this is to avoid confusion with the next
line ('n.test1').

Or should this be 'test8'?

[commit-bot: I haz the power, 2016-11-30 05:06:31]

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

[commit-bot: I haz the power, 2016-11-30 05:06:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hayato, 2016-11-30 05:11:08]

removed

[hayato, 2016-11-30 05:11:08]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 05:11:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-11-30 05:17:39]

Description was changed from

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its bahavior; it does not
use a normal DOM mutation function. We need to check slotchange in this
optimized code path too.

BUG=658605
==========

to

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function.

See `replaceChildrenWithFragment()` in Serialization.cpp for details.

This optimization is wrong;

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".


In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

[hayato, 2016-11-30 05:18:04]

Description was changed from

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function.

See `replaceChildrenWithFragment()` in Serialization.cpp for details.

This optimization is wrong;

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".


In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

to

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong;

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".


In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

[hayato, 2016-11-30 05:18:35]

Description was changed from

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong;

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".


In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

to

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

[hayato, 2016-11-30 05:22:56]

Thanks you for the reviews.

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/shadow-dom/slotchange.html (right):

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/shadow-dom/slotchange.html:319: </div>
Done.

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/shadow-dom/slotchange.html:323: const n =
createTestTree(window.test1);
On 2016/11/30 at 04:20:37, kochi wrote:
> nit: other tests use just 'test1', and no local 'test1' in this function, so
'window.' is unnecessary? It's fine if this is to avoid confusion with the next
line ('n.test1').
> 
> Or should this be 'test8'?

Recently, I am using `window.` prefix explicitly for new code to suppress
eslint's warnings.
Yeah, it is inconsistent within this file, but that might be okay.

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/serializers/Serialization.cpp
(right):

https://codereview.chromium.org/2532393003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/serializers/Serialization.cpp:717: if
(containerNode->hasOneTextChild() && fragment->hasOneTextChild()) {
Agreed. I removed this optimization.

[tkent, 2016-11-30 06:04:01]

PS2 fixes imported/wpt/dom/nodes/MutationObserver-inner-outer.html.  It's a good
sign.

[hayato, 2016-11-30 06:23:31]

Description was changed from

==========
Fire a slotchange event when a text node is replaced via innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

to

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

[hayato, 2016-11-30 06:24:25]

Description was changed from

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that, this optimization prevents from a slotchange event being
fired
because it does not check it.

This CL removes this wrong optimization.

BUG=658605
==========

to

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that, this optimization prevents from a slotchange event being
fired
because it does skip the check.

This CL removes this wrong optimization.

BUG=658605
==========

[commit-bot: I haz the power, 2016-11-30 06:43:50]

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

[commit-bot: I haz the power, 2016-11-30 06:43:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kochi, 2016-11-30 07:02:49]

lgtm

[hayato, 2016-11-30 07:32:51]

remove an unnecessary expected.txt because the test becomes to pass.

[hayato, 2016-11-30 07:32:51]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 07:33:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 09:03:10]

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

[commit-bot: I haz the power, 2016-11-30 09:03:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hayato, 2016-11-30 09:27:23]

The CQ bit was checked by hayato@chromium.org

[hayato, 2016-11-30 09:27:24]

The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2532393003/#ps40001
(title: "remove an unnecessary expected.txt because the test becomes to pass.")

[commit-bot: I haz the power, 2016-11-30 09:27:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 11:06:32]

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

[commit-bot: I haz the power, 2016-11-30 11:06:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hayato, 2016-12-01 02:44:34]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 02:44:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 04:18:54]

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

[commit-bot: I haz the power, 2016-12-01 04:18:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hayato, 2016-12-01 06:32:26]

It looks the patch would cause a regression on some tests. Let me take a look.

[hayato, 2016-12-02 05:05:36]

Some of failing tests are mutating a text node in a mutation observer handler,
and starting to emit "Maximum call stack size exceeded" with this patch.

This sounds an expected behavior. Blink has already the same behavior if
`xx.innerHTML = '<div></div>'` is used in a mutation handler without this CL.
Firefox has the same behavior, saying "too much recursion", in this case.

Thus, we have to rebase our expectations somehow, and update some tests if
necessary.

[tkent, 2016-12-02 05:08:41]

On 2016/12/02 at 05:05:36, hayato wrote:
> Some of failing tests are mutating a text node in a mutation observer handler,
and starting to emit "Maximum call stack size exceeded" with this patch.
> 
> This sounds an expected behavior. Blink has already the same behavior if
`xx.innerHTML = '<div></div>'` is used in a mutation handler without this CL.
> Firefox has the same behavior, saying "too much recursion", in this case.
> 
> Thus, we have to rebase our expectations somehow, and update some tests if
necessary.

I agree.

[hayato, 2016-12-02 06:03:20]

rebased

[hayato, 2016-12-02 06:03:21]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 06:03:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-12-02 07:27:31]

Description was changed from

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that, this optimization prevents from a slotchange event being
fired
because it does skip the check.

This CL removes this wrong optimization.

BUG=658605
==========

to

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that:
- This optimization prevents from a slotchange event being fired because it does
skip the check.
- This optimization prevents mutation observers from be notified.

This CL removes this wrong optimization.

BUG=658605
==========

[hayato, 2016-12-02 08:02:36]

use NeedsRebaseline

[hayato, 2016-12-02 08:02:37]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 08:02:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-12-02 08:51:28]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-12-02 08:51:31]

The CQ bit was checked by hayato@chromium.org

[hayato, 2016-12-02 08:51:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2532393003/#ps80001
(title: "use NeedsRebaseline")

[hayato, 2016-12-02 08:51:46]

The CQ bit was unchecked by hayato@chromium.org

[commit-bot: I haz the power, 2016-12-02 08:51:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-12-02 08:53:09]

Description was changed from

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to that:
- This optimization prevents from a slotchange event being fired because it does
skip the check.
- This optimization prevents mutation observers from be notified.

This CL removes this wrong optimization.

BUG=658605
==========

to

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to this wrong behavior, this optimization also:
- Prevents a slotchange event from being fired because it skips the check.
- Prevents a mutation observer from being notified.

BUG=658605
==========

[hayato, 2016-12-02 08:53:19]

The CQ bit was checked by hayato@chromium.org

[commit-bot: I haz the power, 2016-12-02 08:53:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 10:43:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480668799847280,
"parent_rev": "d1884f3137bfe8558184719a623be4371b4b029f", "commit_rev":
"258fa2c7fafe842a5cf53789c7a026b519988afb"}

[commit-bot: I haz the power, 2016-12-02 10:44:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-02 10:46:36]

Description was changed from

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to this wrong behavior, this optimization also:
- Prevents a slotchange event from being fired because it skips the check.
- Prevents a mutation observer from being notified.

BUG=658605
==========

to

==========
Remove the wrong optimization in replacing a text node with innerHTML

Replacing a single text node with innerHTML optimizes its behavior; it does not
use a normal DOM mutation function. See `replaceChildrenWithFragment()` in
Serialization.cpp for details.

This optimization is wrong:

<div>original</div>

var originalText = div.firstChild;
div.innerHTML = 'updated';
console.log(originalText.data);
// It should show "original", but shows "updated".

In addition to this wrong behavior, this optimization also:
- Prevents a slotchange event from being fired because it skips the check.
- Prevents a mutation observer from being notified.

BUG=658605

Committed: https://crrev.com/946f30141fad7e002a44d3545a7d9f45e3cdcbf3
Cr-Commit-Position: refs/heads/master@{#435908}
==========

[commit-bot: I haz the power, 2016-12-02 10:46:37]

Patchset 5 (id:??) landed as
https://crrev.com/946f30141fad7e002a44d3545a7d9f45e3cdcbf3
Cr-Commit-Position: refs/heads/master@{#435908}