Fix undo stack ordering with TypingCommand

Fix undo stack ordering with TypingCommand

This patch guards certain parts in TypingCommand by EventQueueScope,
so that when adding to an open typing command, if MutationEvent is
triggered, the undo stack ordering is still maintained.

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

Review-Url: https://codereview.chromium.org/2700913002
Cr-Commit-Position: refs/heads/master@{#452373}
Committed: https://chromium.googlesource.com/chromium/src/+/7a5e5d369147c5ce7977ebf1d5ae97d4c572abb4

[Xiaocheng, 2017-02-17 02:23:15]

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

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

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 04:27:29]

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

[commit-bot: I haz the power, 2017-02-17 04:27:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_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)

[Xiaocheng, 2017-02-18 04:26:28]

Description was changed from

==========
Fix undo stack ordering with TypingCommand


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

to

==========
Fix undo stack ordering with TypingCommand

This patch guards certain parts in TypingCommand by EventQueueScope,
so that when adding to an open typing command, if MutationEvent is
triggered, the undo stack ordering is still maintained.

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

[Xiaocheng, 2017-02-18 04:26:34]

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

[Xiaocheng, 2017-02-18 04:26:35]

PTAL.

[yosin_UTC9, 2017-02-20 06:22:52]

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

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/type_with_mutation_event_undo_order.html:28:
document.execCommand('undo');
These tests and similar test are useful since these tests verifies "undo"
command rather than internal state of undo stack.
So, we don't need to expose undo stack to layout test.

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right):

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:381: // No
need for EventQueueScope because this function is only reachable
Nested |EventQueueScope| is not harmful.
It seems just using |EventQueueScope| is simpler than
|DCHECK(document.isRunningExecCommand())| with comment.
So, I prefer using |EventQueueScope| here.

[Xiaocheng, 2017-02-22 02:50:30]

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right):

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:381: // No
need for EventQueueScope because this function is only reachable
On 2017/02/20 at 06:22:51, yosin_UTC9 wrote:
> Nested |EventQueueScope| is not harmful.
> It seems just using |EventQueueScope| is simpler than
|DCHECK(document.isRunningExecCommand())| with comment.
> So, I prefer using |EventQueueScope| here.

Since this part is not reachable from user actions, it's impossible to have a
reasonable test case covering it.

And it's going to look weird to change code without test coverage...

That's why I ended up with adding a DCHECK here.

[yosin_UTC9, 2017-02-22 04:18:42]

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right):

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:381: // No
need for EventQueueScope because this function is only reachable
On 2017/02/22 at 02:50:30, Xiaocheng wrote:
> On 2017/02/20 at 06:22:51, yosin_UTC9 wrote:
> > Nested |EventQueueScope| is not harmful.
> > It seems just using |EventQueueScope| is simpler than
|DCHECK(document.isRunningExecCommand())| with comment.
> > So, I prefer using |EventQueueScope| here.
> 
> Since this part is not reachable from user actions, it's impossible to have a
reasonable test case covering it.
> 
> And it's going to look weird to change code without test coverage...
> 
> That's why I ended up with adding a DCHECK here.

I'm confusing. I think tests in the patch cover this path.
However, I don't see document.execCommand('insertParagraphSeparator') in
attached tests.

Is using gTest make writing test easier?

[yosin_UTC9, 2017-02-22 04:42:25]

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right):

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:381: // No
need for EventQueueScope because this function is only reachable
On 2017/02/22 at 04:18:42, yosin_UTC9 wrote:
> On 2017/02/22 at 02:50:30, Xiaocheng wrote:
> > On 2017/02/20 at 06:22:51, yosin_UTC9 wrote:
> > > Nested |EventQueueScope| is not harmful.
> > > It seems just using |EventQueueScope| is simpler than
|DCHECK(document.isRunningExecCommand())| with comment.
> > > So, I prefer using |EventQueueScope| here.
> > 
> > Since this part is not reachable from user actions, it's impossible to have
a reasonable test case covering it.
> > 
> > And it's going to look weird to change code without test coverage...
> > 
> > That's why I ended up with adding a DCHECK here.
> 
> I'm confusing. I think tests in the patch cover this path.
> However, I don't see document.execCommand('insertParagraphSeparator') in
attached tests.
> 
> Is using gTest make writing test easier?

It seems TypingCommand::insertParagraphSeparatorInQuotedContent(Document&
document) is called from executeInsertNewlineInQuotedContent() in
"EditorCommand.cpp".
It seems it isn't called via Document::execCommand().
Did I look wrong thing?

If this function requires called in Document::execCommand(), we should rename
this function to indicate this function is Document::execCommand only, e.g.
insertParagraphSeparatorInQuotedContentForExecCommand(), otherwise in the
future, we will call in another path.

[Xiaocheng, 2017-02-22 06:02:44]

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right):

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:381: // No
need for EventQueueScope because this function is only reachable
On 2017/02/22 at 04:42:25, yosin_UTC9 wrote:
> On 2017/02/22 at 04:18:42, yosin_UTC9 wrote:
> > On 2017/02/22 at 02:50:30, Xiaocheng wrote:
> > > On 2017/02/20 at 06:22:51, yosin_UTC9 wrote:
> > > > Nested |EventQueueScope| is not harmful.
> > > > It seems just using |EventQueueScope| is simpler than
|DCHECK(document.isRunningExecCommand())| with comment.
> > > > So, I prefer using |EventQueueScope| here.
> > > 
> > > Since this part is not reachable from user actions, it's impossible to
have a reasonable test case covering it.
> > > 
> > > And it's going to look weird to change code without test coverage...
> > > 
> > > That's why I ended up with adding a DCHECK here.
> > 
> > I'm confusing. I think tests in the patch cover this path.
> > However, I don't see document.execCommand('insertParagraphSeparator') in
attached tests.
> > 
> > Is using gTest make writing test easier?
> 
> It seems TypingCommand::insertParagraphSeparatorInQuotedContent(Document&
document) is called from executeInsertNewlineInQuotedContent() in
"EditorCommand.cpp".
> It seems it isn't called via Document::execCommand().
> Did I look wrong thing?

Document::execCommand() runs this function via Editor::Command::execute

> 
> If this function requires called in Document::execCommand(), we should rename
this function to indicate this function is Document::execCommand only, e.g.
> insertParagraphSeparatorInQuotedContentForExecCommand(), otherwise in the
future, we will call in another path.

I'm not sure if there's a requirement, but currently the only code path entering
this command is via Document::execCommand. (TestRunner::execCommand doesn't
count). Anyway, it seems to be some very old code that nobody uses.

How about that: if we observe any DCHECK hit, we can remove the DCHECK, add an
EventQueueScope and also a test case accordingly; if we don't observe any DCHECK
hit, we change the function name as you suggested.

[yosin_UTC9, 2017-02-22 06:18:44]

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right):

https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:381: // No
need for EventQueueScope because this function is only reachable
On 2017/02/22 at 06:02:44, Xiaocheng wrote:
> On 2017/02/22 at 04:42:25, yosin_UTC9 wrote:
> > On 2017/02/22 at 04:18:42, yosin_UTC9 wrote:
> > > On 2017/02/22 at 02:50:30, Xiaocheng wrote:
> > > > On 2017/02/20 at 06:22:51, yosin_UTC9 wrote:
> > > > > Nested |EventQueueScope| is not harmful.
> > > > > It seems just using |EventQueueScope| is simpler than
|DCHECK(document.isRunningExecCommand())| with comment.
> > > > > So, I prefer using |EventQueueScope| here.
> > > > 
> > > > Since this part is not reachable from user actions, it's impossible to
have a reasonable test case covering it.
> > > > 
> > > > And it's going to look weird to change code without test coverage...
> > > > 
> > > > That's why I ended up with adding a DCHECK here.
> > > 
> > > I'm confusing. I think tests in the patch cover this path.
> > > However, I don't see document.execCommand('insertParagraphSeparator') in
attached tests.
> > > 
> > > Is using gTest make writing test easier?
> > 
> > It seems TypingCommand::insertParagraphSeparatorInQuotedContent(Document&
document) is called from executeInsertNewlineInQuotedContent() in
"EditorCommand.cpp".
> > It seems it isn't called via Document::execCommand().
> > Did I look wrong thing?
> 
> Document::execCommand() runs this function via Editor::Command::execute
> 
> > 
> > If this function requires called in Document::execCommand(), we should
rename this function to indicate this function is Document::execCommand only,
e.g.
> > insertParagraphSeparatorInQuotedContentForExecCommand(), otherwise in the
future, we will call in another path.
> 
> I'm not sure if there's a requirement, but currently the only code path
entering this command is via Document::execCommand. (TestRunner::execCommand
doesn't count). Anyway, it seems to be some very old code that nobody uses.
> 
> How about that: if we observe any DCHECK hit, we can remove the DCHECK, add an
EventQueueScope and also a test case accordingly; if we don't observe any DCHECK
hit, we change the function name as you suggested.
I'm not a fan of this approach. This opens the door to ClusterFuzz.

ClusterFuzz may generate testRunner.execCommand('insertParagraphSeparator') then
we need to address it event if it only for testing[1][2].


[1] http://crrev.com/2706913003 FrameView can be null in addTextMatchMaker()
after appendChild(iframe)
[2] http://crrev.com/2065513002 Make textInputController.selectedRange() not to
crash when there is no selection

[Xiaocheng, 2017-02-23 00:30:43]

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

[commit-bot: I haz the power, 2017-02-23 00:31:43]

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-23 00:32:06]

On 2017/02/22 at 06:18:44, yosin wrote:
>
https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp
(right):
> 
>
https://codereview.chromium.org/2700913002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:381: // No
need for EventQueueScope because this function is only reachable
> On 2017/02/22 at 06:02:44, Xiaocheng wrote:
> > On 2017/02/22 at 04:42:25, yosin_UTC9 wrote:
> > > On 2017/02/22 at 04:18:42, yosin_UTC9 wrote:
> > > > On 2017/02/22 at 02:50:30, Xiaocheng wrote:
> > > > > On 2017/02/20 at 06:22:51, yosin_UTC9 wrote:
> > > > > > Nested |EventQueueScope| is not harmful.
> > > > > > It seems just using |EventQueueScope| is simpler than
|DCHECK(document.isRunningExecCommand())| with comment.
> > > > > > So, I prefer using |EventQueueScope| here.
> > > > > 
> > > > > Since this part is not reachable from user actions, it's impossible to
have a reasonable test case covering it.
> > > > > 
> > > > > And it's going to look weird to change code without test coverage...
> > > > > 
> > > > > That's why I ended up with adding a DCHECK here.
> > > > 
> > > > I'm confusing. I think tests in the patch cover this path.
> > > > However, I don't see document.execCommand('insertParagraphSeparator') in
attached tests.
> > > > 
> > > > Is using gTest make writing test easier?
> > > 
> > > It seems TypingCommand::insertParagraphSeparatorInQuotedContent(Document&
document) is called from executeInsertNewlineInQuotedContent() in
"EditorCommand.cpp".
> > > It seems it isn't called via Document::execCommand().
> > > Did I look wrong thing?
> > 
> > Document::execCommand() runs this function via Editor::Command::execute
> > 
> > > 
> > > If this function requires called in Document::execCommand(), we should
rename this function to indicate this function is Document::execCommand only,
e.g.
> > > insertParagraphSeparatorInQuotedContentForExecCommand(), otherwise in the
future, we will call in another path.
> > 
> > I'm not sure if there's a requirement, but currently the only code path
entering this command is via Document::execCommand. (TestRunner::execCommand
doesn't count). Anyway, it seems to be some very old code that nobody uses.
> > 
> > How about that: if we observe any DCHECK hit, we can remove the DCHECK, add
an EventQueueScope and also a test case accordingly; if we don't observe any
DCHECK hit, we change the function name as you suggested.
> I'm not a fan of this approach. This opens the door to ClusterFuzz.
> 
> ClusterFuzz may generate testRunner.execCommand('insertParagraphSeparator')
then we need to address it event if it only for testing[1][2].
> 
> 
> [1] http://crrev.com/2706913003 FrameView can be null in addTextMatchMaker()
after appendChild(iframe)
> [2] http://crrev.com/2065513002 Make textInputController.selectedRange() not
to crash when there is no selection

In this case, we have to also protect insert...InQuotedContent with an
EventQueueScope.

Done in Patch 3.

[yosin_UTC9, 2017-02-23 02:59:52]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-23 03:00:00]

lgtm

Thanks!

[yosin_UTC9, 2017-02-23 03:00:02]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-02-23 03:00:28]

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-23 03:26:10]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487818802687120,
"parent_rev": "76d5eefa3e349bb0d925e64d24d087d64f0ade50", "commit_rev":
"7a5e5d369147c5ce7977ebf1d5ae97d4c572abb4"}

[commit-bot: I haz the power, 2017-02-23 03:26:45]

Description was changed from

==========
Fix undo stack ordering with TypingCommand

This patch guards certain parts in TypingCommand by EventQueueScope,
so that when adding to an open typing command, if MutationEvent is
triggered, the undo stack ordering is still maintained.

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

to

==========
Fix undo stack ordering with TypingCommand

This patch guards certain parts in TypingCommand by EventQueueScope,
so that when adding to an open typing command, if MutationEvent is
triggered, the undo stack ordering is still maintained.

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

Review-Url: https://codereview.chromium.org/2700913002
Cr-Commit-Position: refs/heads/master@{#452373}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a5e5d369147c5ce7977ebf1d5ae...
==========

[commit-bot: I haz the power, 2017-02-23 03:26:46]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7a5e5d369147c5ce7977ebf1d5ae...