[Editing] Store |CommandSource| in |CompositeEditCommand|

[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL was split into 3 sub-patches:
1. https://crrev.com/2578753002
2. https://crrev.com/2576903002
3. https://crrev.com/2579253002

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This CL shouldn't cause any behavior change, and is a pre-patch
for https://crrev.com/2558643003/.

BUG=673789

[chongz, 2016-12-13 17:01:59]

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

[commit-bot: I haz the power, 2016-12-13 17:02:59]

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-13 18:38:06]

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

[commit-bot: I haz the power, 2016-12-13 18:38:07]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-12-13 19:15:58]

Description was changed from

==========
[Editing] Store CommandSource in CompositEditCommand

BUG=673789
==========

to

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This is a pre-patch for https://crrev.com/2558643003/.

BUG=673789
==========

[chongz, 2016-12-13 19:16:28]

Description was changed from

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This is a pre-patch for https://crrev.com/2558643003/.

BUG=673789
==========

to

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This is a pre-patch for https://crrev.com/2558643003/.

BUG=673789
==========

[chongz, 2016-12-13 20:03:31]

Description was changed from

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This is a pre-patch for https://crrev.com/2558643003/.

BUG=673789
==========

to

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This is shouldn't cause any behavior change, and is a pre-patch for 
https://crrev.com/2558643003/.

BUG=673789
==========

[chongz, 2016-12-13 20:03:49]

Description was changed from

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This is shouldn't cause any behavior change, and is a pre-patch for 
https://crrev.com/2558643003/.

BUG=673789
==========

to

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This CL shouldn't cause any behavior change, and is a
pre-patch for https://crrev.com/2558643003/.

BUG=673789
==========

[chongz, 2016-12-13 20:04:05]

Description was changed from

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This CL shouldn't cause any behavior change, and is a
pre-patch for https://crrev.com/2558643003/.

BUG=673789
==========

to

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This CL shouldn't cause any behavior change, and is a pre-patch
for https://crrev.com/2558643003/.

BUG=673789
==========

[chongz, 2016-12-13 20:08:11]

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

[chongz, 2016-12-13 20:08:12]

xiaochengh@ PTAL, thanks! (Seems that yosin@ and yutak@ are both OOO)

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right):

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:827:
DCHECK_EQ(source, CommandSource::MenuOrKeyBinding);
|executeInsertBacktab()| is only supported from menu or key binding, similar for
other |execute*| that fires an |TextEvent|.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2562: //
Specially handling commands that Editor::execCommand does not directly
Note: This is for |WebFrame::executeCommand()| rather than
|Document::execCommand(||.

[Xiaocheng, 2016-12-14 04:19:50]

I tried to understand the patch but didn't succeed due to my limited experience
with event lifecycle...

Anyway, could you make the patch easier for me by splitting it into simpler
patches?

I notice that a lot of changes in editing/command are kind of
mechanical/straightforward. It would be easier to review if mechanical parts can
be separated out.

Besides, in the current code base, there are only two initialization sites of
EditorCommandSource (in Document and EditorCommand resp.), which is easy to
track and reason about. This patch makes some functions taking an extra
CommandSource parameter, and hence, introduces a lot more initialization sites.
It is hard figure out whether the value is correct, and whether the relevant
functions really need the extra parameter. Is it possible to somehow limit and
unify the init sites of CommandSource?

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

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:992: if
(!focusedOrMainFrame->selection().isAvailable())
Seems irrelevant to this patch.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:1291: if
(m_preventRevealSelection || !m_frame->selection().isAvailable())
Seems irrelevant to this patch.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:1445: if
(frame().selection().isAvailable())
Seems irrelevant to this patch.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right):

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:693: if
(frame.selection().isAvailable())
Seems irrelevant to this patch.

[Xiaocheng, 2016-12-14 05:13:15]

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:115: #include
"core/editing/commands/CompositeEditCommand.h"
Besides, ideally we would like not to make anything outside editing depend on
editing/commands.

[chongz, 2016-12-15 00:53:33]

> I tried to understand the patch but didn't succeed due to my limited
experience
> with event lifecycle...
> 
> Anyway, could you make the patch easier for me by splitting it into simpler
> patches?

Sorry for the confusion. I will split it into 3 patches:
1. Rename |CommandSource|
2. Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...|
3. Pass source from |Editor::apply*()/insert*()/...| to |CompositeEditCommand|
   and |TypingCommand|

And the first 2 patches are here:
1. https://crrev.com/2578753002
2. https://crrev.com/2576903002

> 
> I notice that a lot of changes in editing/command are kind of
> mechanical/straightforward. It would be easier to review if mechanical parts
can
> be separated out.
> 
> Besides, in the current code base, there are only two initialization sites of
> EditorCommandSource (in Document and EditorCommand resp.), which is easy to
> track and reason about. This patch makes some functions taking an extra
> CommandSource parameter, and hence, introduces a lot more initialization
sites.
> It is hard figure out whether the value is correct, and whether the relevant
> functions really need the extra parameter. Is it possible to somehow limit and
> unify the init sites of CommandSource?

The original plan was to make |CommandSource| an attribute of
|CompositeEditCommand|,
which means we'd have to modify lots of constructors.
To make our life easier, do you think it would be better to pass |CommandSource|
to |CompositeEditCommand::apply(CommandSource =
CommandSource::MenuOrKeyBinding)| instead?
The benefit is we don't have to touch constructors.

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

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:992: if
(!focusedOrMainFrame->selection().isAvailable())
On 2016/12/14 04:19:50, Xiaocheng wrote:
> Seems irrelevant to this patch.

Will remove.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:1291: if
(m_preventRevealSelection || !m_frame->selection().isAvailable())
On 2016/12/14 04:19:50, Xiaocheng wrote:
> Seems irrelevant to this patch.

Will remove.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:1445: if
(frame().selection().isAvailable())
On 2016/12/14 04:19:50, Xiaocheng wrote:
> Seems irrelevant to this patch.

Will remove.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right):

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:693: if
(frame.selection().isAvailable())
On 2016/12/14 04:19:50, Xiaocheng wrote:
> Seems irrelevant to this patch.

Will remove.

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2574793002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:115: #include
"core/editing/commands/CompositeEditCommand.h"
On 2016/12/14 05:13:15, Xiaocheng wrote:
> Besides, ideally we would like not to make anything outside editing depend on
> editing/commands.

Changed |CommandSource| to be a default parameter in the second sub-patch:
"https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/core/editing/Editor.h#newcode277"

[chongz, 2016-12-16 18:22:41]

Description was changed from

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This CL shouldn't cause any behavior change, and is a pre-patch
for https://crrev.com/2558643003/.

BUG=673789
==========

to

==========
[Editing] Store |CommandSource| in |CompositeEditCommand|

This CL was split into 3 sub-patches:
1. https://crrev.com/2578753002
2. https://crrev.com/2576903002
3. https://crrev.com/2579253002

This CL added a new attribute |CommandSource m_source| to 
|CompositeEditCommand| and it should be set for all of it's
sub-classes.

|m_source| could wither be
  * |CommandSource::MenuOrKeyBinding|, e.g. User actions, or
  * |CommandSource::Dom|, e.g. 'document.execCommand()'.

This CL shouldn't cause any behavior change, and is a pre-patch
for https://crrev.com/2558643003/.

BUG=673789
==========