Fix undo stack ordering for all-but-typing commands

Fix undo stack ordering for all-but-typing commands

When one top-level editing command triggers another command via
event handlers, Blink used to have their undo ordering reversed.
The reason is that the enstacking of commands is not protected
by EventQueueScope; consequently, the commands are executed in
a nested manner, making the first command end later than the
second command.

This patch protects everything in CompositeEditCommand::apply
with EventQueueScope, so that the nesting behavior for all
commands (except TypingCommand) is eliminated.

BUG=685975
TEST=editing/undo/drag_with_mutation_event_undo_order.html

Review-Url: https://codereview.chromium.org/2674173002
Cr-Commit-Position: refs/heads/master@{#452095}
Committed: https://chromium.googlesource.com/chromium/src/+/91ce4c5c6b3c38b5291c94d897f691b30282cd79

[Xiaocheng, 2017-02-16 21:18:32]

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

[commit-bot: I haz the power, 2017-02-16 21:19:51]

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

[Xiaocheng, 2017-02-16 21:22:35]

Description was changed from

==========
Fix undo stack ordering for all-but-typing commands

BUG=
==========

to

==========
Fix undo stack ordering for all-but-typing commands

When one top-level editing command triggers another command via
event handlers, Blink used to have their undo ordering reversed.
The reason is that the enstacking of commands is not protected
by EventQueueScope; consequently, the commands are executed in
a nested manner, making the first command end later than the
second command.

This patch protects everything in CompositeEditCommand::apply
with EventQueueScope, so that the nesting behavior for all
commands (except TypingCommand) is eliminated.

BUG=685975
TEST=editing/undo/drag_with_mutation_event_undo_order.html
==========

[Xiaocheng, 2017-02-16 21:22:43]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2017-02-16 21:22:43]

PTAL.

[Xiaocheng, 2017-02-16 21:23:45]

https://codereview.chromium.org/2674173002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/undo/drag_with_mutation_event_undo_order.html
(right):

https://codereview.chromium.org/2674173002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/drag_with_mutation_event_undo_order.html:51:
dragAndDrop(selection, 'source', 'div1');
Note: the first command must be initiated by user action instead of
document.execCommand, because nested execution of execCommand is prohibited.

[commit-bot: I haz the power, 2017-02-16 23:35:19]

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

[commit-bot: I haz the power, 2017-02-16 23:35:20]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xiaocheng, 2017-02-17 00:20:38]

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

[commit-bot: I haz the power, 2017-02-17 00:21: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, 2017-02-17 02:24:15]

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

[commit-bot: I haz the power, 2017-02-17 02:24:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[yosin_UTC9, 2017-02-17 13:05:34]

https://codereview.chromium.org/2674173002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/undo/drag_with_mutation_event_undo_order.html
(right):

https://codereview.chromium.org/2674173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/drag_with_mutation_event_undo_order.html:72:
'undo',
We don't need to check result of "undo" command, since functionality of "undo"
is checked in other tests.

The thing we want to verify this test is L80. So, verifying result of
drag-and-drop and undo is "pre-condition" check.

See comment in PS#4 of https://codereview.chromium.org/2685723005

[Xiaocheng, 2017-02-17 18:46:30]

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

[commit-bot: I haz the power, 2017-02-17 18:47:17]

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

[Xiaocheng, 2017-02-17 18:47:23]

Patch updated with test case minimized. PTAL.

https://codereview.chromium.org/2674173002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/undo/drag_with_mutation_event_undo_order.html
(right):

https://codereview.chromium.org/2674173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/drag_with_mutation_event_undo_order.html:72:
'undo',
On 2017/02/17 at 13:05:34, yosin_UTC9 wrote:
> We don't need to check result of "undo" command, since functionality of "undo"
is checked in other tests.
> 
> The thing we want to verify this test is L80. So, verifying result of
drag-and-drop and undo is "pre-condition" check.
> 
> See comment in PS#4 of https://codereview.chromium.org/2685723005

This test check undo ordering.

To make tests minimal and eliminate dependency, I'm going to check only the
result of the first undo.

[commit-bot: I haz the power, 2017-02-17 20:34:28]

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

[commit-bot: I haz the power, 2017-02-17 20:34:29]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-02-20 06:12:02]

lgtm

https://codereview.chromium.org/2674173002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/undo/paste_with_mutation_event_undo_order.html
(right):

https://codereview.chromium.org/2674173002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/paste_with_mutation_event_undo_order.html:30:
document.execCommand('undo');
Do we have other test scripts want to check undo stack ordering?
Do we want to expose "undo stack" via window.internals?

[Xiaocheng, 2017-02-22 02:36:20]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-02-22 02:36:20]

Thanks for the review.

https://codereview.chromium.org/2674173002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/undo/paste_with_mutation_event_undo_order.html
(right):

https://codereview.chromium.org/2674173002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/paste_with_mutation_event_undo_order.html:30:
document.execCommand('undo');
On 2017/02/20 at 06:12:02, yosin_UTC9 wrote:
> Do we have other test scripts want to check undo stack ordering?
> Do we want to expose "undo stack" via window.internals?

Guess your comment in the other patch means you are no longer a fan of this
idea.

So I'm committing.

[commit-bot: I haz the power, 2017-02-22 02:37:21]

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, 2017-02-22 04:40:45]

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

[commit-bot: I haz the power, 2017-02-22 04:40:46]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Xiaocheng, 2017-02-22 05:39:29]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:40:22]

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, 2017-02-22 07:34:54]

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

[commit-bot: I haz the power, 2017-02-22 07:34:55]

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

[Xiaocheng, 2017-02-22 16:09:23]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2017-02-22 16:09:45]

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, 2017-02-22 17:01:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487779763507190,
"parent_rev": "7cec6d255cab4a76bcee44508c4d29622f5c933e", "commit_rev":
"91ce4c5c6b3c38b5291c94d897f691b30282cd79"}

[commit-bot: I haz the power, 2017-02-22 17:03:27]

Description was changed from

==========
Fix undo stack ordering for all-but-typing commands

When one top-level editing command triggers another command via
event handlers, Blink used to have their undo ordering reversed.
The reason is that the enstacking of commands is not protected
by EventQueueScope; consequently, the commands are executed in
a nested manner, making the first command end later than the
second command.

This patch protects everything in CompositeEditCommand::apply
with EventQueueScope, so that the nesting behavior for all
commands (except TypingCommand) is eliminated.

BUG=685975
TEST=editing/undo/drag_with_mutation_event_undo_order.html
==========

to

==========
Fix undo stack ordering for all-but-typing commands

When one top-level editing command triggers another command via
event handlers, Blink used to have their undo ordering reversed.
The reason is that the enstacking of commands is not protected
by EventQueueScope; consequently, the commands are executed in
a nested manner, making the first command end later than the
second command.

This patch protects everything in CompositeEditCommand::apply
with EventQueueScope, so that the nesting behavior for all
commands (except TypingCommand) is eliminated.

BUG=685975
TEST=editing/undo/drag_with_mutation_event_undo_order.html

Review-Url: https://codereview.chromium.org/2674173002
Cr-Commit-Position: refs/heads/master@{#452095}
Committed:
https://chromium.googlesource.com/chromium/src/+/91ce4c5c6b3c38b5291c94d897f6...
==========

[commit-bot: I haz the power, 2017-02-22 17:03:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/91ce4c5c6b3c38b5291c94d897f6...