Change the timing of event dispatching and <script> execution in Node::appendChild, insertBefore,...

Change the timing of event dispatching and <script> execution in Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment children was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to improve interoperability
and avoid additional hierarchy check for crbug.com/523157.

Implementation:
Fold updateTreeAfterInsertion() and notifyNodeInserted() into insertNodeVector(),
and move childrenChange(), didNotifySubtreeInsertionsToDocument(), and
dispatchChildInsertionEvents() out from the loop.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157
Committed: https://crrev.com/e15391d9a56a210cf5faee97c24bfad856225ea9
Cr-Commit-Position: refs/heads/master@{#417209}

[tkent, 2016-09-05 05:18:02]

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

[commit-bot: I haz the power, 2016-09-05 05:18:14]

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-09-05 05:36:44]

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

[commit-bot: I haz the power, 2016-09-05 05:36:44]

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

[tkent, 2016-09-08 03:05:43]

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

[commit-bot: I haz the power, 2016-09-08 03:05:58]

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

[tkent, 2016-09-08 03:11:37]

Description was changed from

==========
Delay DOM mutation events and iframe load events in Node::appendChild,
inesrtBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment childlen was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

This CL makes Blink same as Firefox and Edge in order to avoid additional
hierarchy check for crbug.com/523157.

This CL also changes the timing of <iframe> load event because of
EventQueueScope.

Test changes:
* fast/dom/Document/readystate.html
  Iframe 'load' event timing issue. With this CL,
  dynamicIframe.contentDocument.close() dispatches another 'load' event.

* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

* fast/dom/Window/open-after-frame-detached.html
  Iframe 'load' event timing issue.

* fast/dom/custom/created-callback.html
  Iframe 'load' event timing issue.  Variables 't' was global, and this CL
  exposed a race in the test.

*
imported/wpt/dom/nodes/Document-contentType/contentType/contenttype_javascripturi.html
  Iframe 'load' event timing issue.  The event was dispatched before DOM node
  insertion was completed.

BUG=523157
==========

to

==========
Change the timing of event dispatching and <script> execution in
Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment childlen was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to avoid additional
hierarchy check for crbug.com/523157.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157
==========

[tkent, 2016-09-08 03:35:58]

Patchset #1 (id:1) has been deleted

[tkent, 2016-09-08 03:45:33]

Description was changed from

==========
Change the timing of event dispatching and <script> execution in
Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment childlen was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to avoid additional
hierarchy check for crbug.com/523157.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157
==========

to

==========
Change the timing of event dispatching and <script> execution in
Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment children was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to improve
interoperability
and avoid additional hierarchy check for crbug.com/523157.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157
==========

[tkent, 2016-09-08 04:49:15]

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

[commit-bot: I haz the power, 2016-09-08 04:49:34]

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

[tkent, 2016-09-08 04:52:02]

Description was changed from

==========
Change the timing of event dispatching and <script> execution in
Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment children was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to improve
interoperability
and avoid additional hierarchy check for crbug.com/523157.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157
==========

to

==========
Change the timing of event dispatching and <script> execution in
Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment children was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to improve
interoperability
and avoid additional hierarchy check for crbug.com/523157.

Implementation:
Fold updateTreeAfterInsertion() and notifyNodeInserted() into
insertNodeVector(),
and move childrenChange(), didNotifySubtreeInsertionsToDocument(), and
dispatchChildInsertionEvents() out from the loop.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157
==========

[tkent, 2016-09-08 04:52:44]

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

[tkent, 2016-09-08 04:52:45]

hayato@, would you review this please?

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:164: // TODO(tkent):
mutator never returns false because scripts don't run in the loop.
Will remove unnecessary code in a follow-up CL.

[hayato, 2016-09-08 05:32:21]

Is this diff all? 
Didn't notifyNodeInserted(child) change?

[tkent, 2016-09-08 05:45:07]

On 2016/09/08 at 05:32:21, hayato wrote:
> Is this diff all? 

Yes.

> Didn't notifyNodeInserted(child) change?

No.  notifyNodeInserted() is still called from other places for single node
insertion.

[hayato, 2016-09-08 06:18:55]

I see. I have one more question:

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:181: if
(targetNode->parentNode() == this)
Should we check this condition?
It looks the original code is always calling dispatchChildInsertionEvents.

[commit-bot: I haz the power, 2016-09-08 06:29:32]

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

[commit-bot: I haz the power, 2016-09-08 06:29:33]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-09-08 06:31:09]

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:181: if
(targetNode->parentNode() == this)
On 2016/09/08 at 06:18:55, hayato wrote:
> Should we check this condition?
> It looks the original code is always calling dispatchChildInsertionEvents.

The DOM specification is unclear for this and we may dispatch unconditionally,
but this check is compatible with Firefox and Edge.

With the original code, JavaScript execution for an earlier child by <script> or
DOM mutation event can't remove a later child from the parent because the later
child isn't appended to the parent yet.

[hayato, 2016-09-08 06:48:11]

lgtm

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2306323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:181: if
(targetNode->parentNode() == this)
We chatted offline. 

We have decided to make this align to Firefox and Edge, though the spec is
unclear.

[tkent, 2016-09-08 06:48:54]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2016-09-08 06:49:14]

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-09-08 06:52:58]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-08 06:55:40]

Description was changed from

==========
Change the timing of event dispatching and <script> execution in
Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment children was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to improve
interoperability
and avoid additional hierarchy check for crbug.com/523157.

Implementation:
Fold updateTreeAfterInsertion() and notifyNodeInserted() into
insertNodeVector(),
and move childrenChange(), didNotifySubtreeInsertionsToDocument(), and
dispatchChildInsertionEvents() out from the loop.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157
==========

to

==========
Change the timing of event dispatching and <script> execution in
Node::appendChild, insertBefore, and replaceChild.

When we insert a DocumentFragment including multiple children into another node,

* WebKit and Blink: DOMNodeInserted is dispatched just after each of
  DocumentFragment children was inserted.

* Firefox and Edge: DOMNodeInserted is dispatched after all of DocumentFragment
  children were inserted.

<script> execution and <iframe> load event have the same incompatibility.

This CL makes Blink same as Firefox and Edge in order to improve
interoperability
and avoid additional hierarchy check for crbug.com/523157.

Implementation:
Fold updateTreeAfterInsertion() and notifyNodeInserted() into
insertNodeVector(),
and move childrenChange(), didNotifySubtreeInsertionsToDocument(), and
dispatchChildInsertionEvents() out from the loop.

Test changes:
* fast/dom/MutationObserver/removed-out-of-order.html
  DOMNodeInserted timing change.  New result matches to Firefox and Edge.

BUG=523157

Committed: https://crrev.com/e15391d9a56a210cf5faee97c24bfad856225ea9
Cr-Commit-Position: refs/heads/master@{#417209}
==========

[commit-bot: I haz the power, 2016-09-08 06:55:41]

Patchset 2 (id:??) landed as
https://crrev.com/e15391d9a56a210cf5faee97c24bfad856225ea9
Cr-Commit-Position: refs/heads/master@{#417209}