[InputEvent] Introduce temporary flag |BeforeInputTiming| (1/11)

[InputEvent] Introduce temporary flag |BeforeInputTiming| (1/11)

This CL introduced temporary flag |BeforeInputTiming| to |willApplyEditing|, 
|willUn/Reapply| and related code path, indicating whether ‘beforeinput’ has already
been fired.

This CL doesn't have behavior change.

Please see bug and the design doc below for the big picture:
https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd4ZkgJs5GPde2ZsDy0/edit?usp=sharing

BUG=678795

[chongz, 2017-01-05 20:29:31]

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

[commit-bot: I haz the power, 2017-01-05 20:30:01]

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

[chongz, 2017-01-05 22:52:52]

Description was changed from

==========
Added flag |BeforeInputTiming|

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

to

==========
[InputEvent] Introduce temporary flag |BeforeInputTiming| (1/11)

This CL introduced temporary flag |BeforeInputTiming| to |willApplyEditing|, 
|willUn/Reapply| and related code path, indicating whether ‘beforeinput’ has
already
been fired.

This CL doesn't have behavior change.

Please see bug and the design doc below for the big picture:
https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd...

BUG=678795
==========

[chongz, 2017-01-05 23:39:35]

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

[chongz, 2017-01-05 23:39:35]

xiaochengh@ I've split the original CL into 11 small CLs as you've suggested,
PTAL, thanks! (And sorry for the coming spam :) )

[commit-bot: I haz the power, 2017-01-05 23:46:21]

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

[commit-bot: I haz the power, 2017-01-05 23:46:22]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-01-06 02:03:45]

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

[Xiaocheng, 2017-01-06 02:03:45]

Thanks for the split, lgtm

+yosin

[chongz, 2017-01-09 23:40:53]

yosin@ PTAL, thanks!

[yosin_UTC9, 2017-01-10 04:15:44]

On 2017/01/09 at 23:40:53, chongz wrote:
> yosin@ PTAL, thanks!

Back from long vacation. I'm still in catch up mode.
I'm confused that our original plan to have command source in
CompositeEditCommand[1].
But, now it is passed to apply() and unapply()[2].

Could you explain why do we need to pass command source to apply()/unapply()?


[1] https://codereview.chromium.org/2574793002 [Editing] Store |CommandSource|
in |CompositeEditCommand|
[2] https://codereview.chromium.org/2581073003 [Editing] Introduce
|EditCommandComposition::willUn/Reapply()| in prepare for 'beforeinput' (2/3)

[chongz, 2017-01-10 04:57:42]

On 2017/01/10 04:15:44, Yosi_UTC9 wrote:
> On 2017/01/09 at 23:40:53, chongz wrote:
> > yosin@ PTAL, thanks!
> 
> Back from long vacation. I'm still in catch up mode.
> I'm confused that our original plan to have command source in
> CompositeEditCommand[1].
> But, now it is passed to apply() and unapply()[2].
> 
> Could you explain why do we need to pass command source to apply()/unapply()?
> 
> 
> [1] https://codereview.chromium.org/2574793002 [Editing] Store |CommandSource|
> in |CompositeEditCommand|
> [2] https://codereview.chromium.org/2581073003 [Editing] Introduce
> |EditCommandComposition::willUn/Reapply()| in prepare for 'beforeinput' (2/3)

Sure.

Yes, the original plan was to have command source in |CompositeEditCommand|
through ctors, but according to comments in [1] I believe xiaocheng@ is
concerned about having the extra parameter in too many irrelevant places (to
which I agree but don't have a better solution).

The new solution is to pass command source in |apply()|, |unapply()| and
|reapply|, where the affected files could be reduced dramatically [3], and it
would be easier for future maintenance (e.g. Easier to add new commands without
knowledge about command source).

Another support reason for passing through |apply()|:
* Constructing a |CompositeEditCommand| is not a 1:1 relationship to
'beforeinput'
    * e.g. |CompositeEditCommand| could be created and executed through
|applyCommandToComposite()|, so it would be confusing to have command source in
both parent command and children commands.
* Calling |apply()| is a 1:1 relationship to 'beforeinput'
    * We should always fire a 'beforeinput' if there is a corresponding
|inputType| and the source is user action

Or do you prefer the original solution or have other suggestions?

[3] https://codereview.chromium.org/2579253002 [EditCommandSource] Pass source
to |CompositEditCommand| and |TypingCommand| (3/3)

[yosin_UTC9, 2017-01-11 02:20:53]

On 2017/01/10 at 04:57:42, chongz wrote:
> On 2017/01/10 04:15:44, Yosi_UTC9 wrote:
> > On 2017/01/09 at 23:40:53, chongz wrote:
> > > yosin@ PTAL, thanks!
> > 
> > Back from long vacation. I'm still in catch up mode.
> > I'm confused that our original plan to have command source in
> > CompositeEditCommand[1].
> > But, now it is passed to apply() and unapply()[2].
> > 
> > Could you explain why do we need to pass command source to
apply()/unapply()?
> > 
> > 
> > [1] https://codereview.chromium.org/2574793002 [Editing] Store
|CommandSource|
> > in |CompositeEditCommand|
> > [2] https://codereview.chromium.org/2581073003 [Editing] Introduce
> > |EditCommandComposition::willUn/Reapply()| in prepare for 'beforeinput'
(2/3)
> 
> Sure.
> 
> Yes, the original plan was to have command source in |CompositeEditCommand|
through ctors, but according to comments in [1] I believe xiaocheng@ is
concerned about having the extra parameter in too many irrelevant places (to
which I agree but don't have a better solution).
> 
First of all thanks for explanation. I understand discussion so far.

> The new solution is to pass command source in |apply()|, |unapply()| and
|reapply|, where the affected files could be reduced dramatically [3], and it
would be easier for future maintenance (e.g. Easier to add new commands without
knowledge about command source).

I still fan of having CommandSource is a parameter of CompositeEditCommand ctor,
because
 - CommandSource is intrinsic property of CompositeEditCommand, it is lasting
and never changed == const member variable. Making it is parameter of apply
implies CommandSource depends on execution flow.
 - Amount of code changes is not good metrics. It can make future debt.
  To split CL, we can make CommandSource as optional parameter then change call
sites incrementally. Finally, we can make CommandSouce to required parameter. We
may want to defer decision whether required or not.
 - New commands should know care about CommandSouce. We don't need to dispatch
"beforeinput" for commands not created by user action.

> 
> Another support reason for passing through |apply()|:
> * Constructing a |CompositeEditCommand| is not a 1:1 relationship to
'beforeinput'
>     * e.g. |CompositeEditCommand| could be created and executed through
|applyCommandToComposite()|, so it would be confusing to have command source in
both parent command and children commands.
Creating |CompoisteEditCommand| other than UI event is implementation details.
CommandSource for them is |kDOM|, or kInternal.
Given that CommandSource can be:

enum class EditCommandSouce {
  kUserAction,  // from menu, or keyboard shortcut
  kExecCommand, // document.execCommand()
  kInternal, // Blink creates a command for implementation.
}

> * Calling |apply()| is a 1:1 relationship to 'beforeinput'
>     * We should always fire a 'beforeinput' if there is a corresponding
|inputType| and the source is user action

Dispatching "beforeinput" should be implementation details of
CompositeEditCommand. Callers don't want to know it.
Also, this makes sure we don't dispatch "beforeinput" for
CompositeEditCommand::apply() created for non-user-action, e.g.

void dispatchBeforeInput(const CompositEditCommand& command, Element* target) {
  DCHECK_EQ(command.command_source(), EditCommandSource::kUserAction);
  ... dispatch "beforeinput" to |target|.
}

> Or do you prefer the original solution or have other suggestions?
Yes, I prefer original solution.

> [3] https://codereview.chromium.org/2579253002 [EditCommandSource] Pass source
to |CompositEditCommand| and |TypingCommand| (3/3)

[chongz, 2017-01-11 05:04:14]

On 2017/01/11 02:20:53, Yosi_UTC9 wrote:
> On 2017/01/10 at 04:57:42, chongz wrote:
> > On 2017/01/10 04:15:44, Yosi_UTC9 wrote:
> > > On 2017/01/09 at 23:40:53, chongz wrote:
> > > > yosin@ PTAL, thanks!
> > > 
> > > Back from long vacation. I'm still in catch up mode.
> > > I'm confused that our original plan to have command source in
> > > CompositeEditCommand[1].
> > > But, now it is passed to apply() and unapply()[2].
> > > 
> > > Could you explain why do we need to pass command source to
> apply()/unapply()?
> > > 
> > > 
> > > [1] https://codereview.chromium.org/2574793002 [Editing] Store
> |CommandSource|
> > > in |CompositeEditCommand|
> > > [2] https://codereview.chromium.org/2581073003 [Editing] Introduce
> > > |EditCommandComposition::willUn/Reapply()| in prepare for 'beforeinput'
> (2/3)
> > 
> > Sure.
> > 
> > Yes, the original plan was to have command source in |CompositeEditCommand|
> through ctors, but according to comments in [1] I believe xiaocheng@ is
> concerned about having the extra parameter in too many irrelevant places (to
> which I agree but don't have a better solution).
> > 
> First of all thanks for explanation. I understand discussion so far.
> 
> > The new solution is to pass command source in |apply()|, |unapply()| and
> |reapply|, where the affected files could be reduced dramatically [3], and it
> would be easier for future maintenance (e.g. Easier to add new commands
without
> knowledge about command source).
> 
> I still fan of having CommandSource is a parameter of CompositeEditCommand
ctor,
> because
>  - CommandSource is intrinsic property of CompositeEditCommand, it is lasting
> and never changed == const member variable. Making it is parameter of apply
> implies CommandSource depends on execution flow.
>  - Amount of code changes is not good metrics. It can make future debt.
>   To split CL, we can make CommandSource as optional parameter then change
call
> sites incrementally. Finally, we can make CommandSouce to required parameter.
We
> may want to defer decision whether required or not.
>  - New commands should know care about CommandSouce. We don't need to dispatch
> "beforeinput" for commands not created by user action.
> 
> > 
> > Another support reason for passing through |apply()|:
> > * Constructing a |CompositeEditCommand| is not a 1:1 relationship to
> 'beforeinput'
> >     * e.g. |CompositeEditCommand| could be created and executed through
> |applyCommandToComposite()|, so it would be confusing to have command source
in
> both parent command and children commands.
> Creating |CompoisteEditCommand| other than UI event is implementation details.
> CommandSource for them is |kDOM|, or kInternal.
> Given that CommandSource can be:
> 
> enum class EditCommandSouce {
>   kUserAction,  // from menu, or keyboard shortcut
>   kExecCommand, // document.execCommand()
>   kInternal, // Blink creates a command for implementation.
> }
> 
> > * Calling |apply()| is a 1:1 relationship to 'beforeinput'
> >     * We should always fire a 'beforeinput' if there is a corresponding
> |inputType| and the source is user action
> 
> Dispatching "beforeinput" should be implementation details of
> CompositeEditCommand. Callers don't want to know it.
> Also, this makes sure we don't dispatch "beforeinput" for
> CompositeEditCommand::apply() created for non-user-action, e.g.
> 
> void dispatchBeforeInput(const CompositEditCommand& command, Element* target)
{
>   DCHECK_EQ(command.command_source(), EditCommandSource::kUserAction);
>   ... dispatch "beforeinput" to |target|.
> }
> 
> > Or do you prefer the original solution or have other suggestions?
> Yes, I prefer original solution.
> 
> > [3] https://codereview.chromium.org/2579253002 [EditCommandSource] Pass
source
> to |CompositEditCommand| and |TypingCommand| (3/3)

OK I see, thanks for the detailed explanation!

However given that the |apply(source)| code is already in and these 11 CLs for
moving 'beforeinput' depend on that, are you OK with the following plan:
1. Keep |apply(source)| code for now
2. Land these 11 CLs as they don't conflict with where we store CommandSource
3. Add CommandSource to ctor gradually according your plan (e.g. as optional
parameter first => required parameter)
4. Remove |apply(source)| and use source from ctor instead

Or do you prefer reverting |apply(source)| and implement CommandSource in ctor
first?

[yosin_UTC9, 2017-01-11 05:50:02]

On 2017/01/11 at 05:04:14, chongz wrote:
> On 2017/01/11 02:20:53, Yosi_UTC9 wrote:
> > On 2017/01/10 at 04:57:42, chongz wrote:
> > > On 2017/01/10 04:15:44, Yosi_UTC9 wrote:
> > > > On 2017/01/09 at 23:40:53, chongz wrote:
> > > > > yosin@ PTAL, thanks!
> > > > 
> > > > Back from long vacation. I'm still in catch up mode.
> > > > I'm confused that our original plan to have command source in
> > > > CompositeEditCommand[1].
> > > > But, now it is passed to apply() and unapply()[2].
> > > > 
> > > > Could you explain why do we need to pass command source to
> > apply()/unapply()?
> > > > 
> > > > 
> > > > [1] https://codereview.chromium.org/2574793002 [Editing] Store
> > |CommandSource|
> > > > in |CompositeEditCommand|
> > > > [2] https://codereview.chromium.org/2581073003 [Editing] Introduce
> > > > |EditCommandComposition::willUn/Reapply()| in prepare for 'beforeinput'
> > (2/3)
> > > 
> > > Sure.
> > > 
> > > Yes, the original plan was to have command source in
|CompositeEditCommand|
> > through ctors, but according to comments in [1] I believe xiaocheng@ is
> > concerned about having the extra parameter in too many irrelevant places (to
> > which I agree but don't have a better solution).
> > > 
> > First of all thanks for explanation. I understand discussion so far.
> > 
> > > The new solution is to pass command source in |apply()|, |unapply()| and
> > |reapply|, where the affected files could be reduced dramatically [3], and
it
> > would be easier for future maintenance (e.g. Easier to add new commands
without
> > knowledge about command source).
> > 
> > I still fan of having CommandSource is a parameter of CompositeEditCommand
ctor,
> > because
> >  - CommandSource is intrinsic property of CompositeEditCommand, it is
lasting
> > and never changed == const member variable. Making it is parameter of apply
> > implies CommandSource depends on execution flow.
> >  - Amount of code changes is not good metrics. It can make future debt.
> >   To split CL, we can make CommandSource as optional parameter then change
call
> > sites incrementally. Finally, we can make CommandSouce to required
parameter. We
> > may want to defer decision whether required or not.
> >  - New commands should know care about CommandSouce. We don't need to
dispatch
> > "beforeinput" for commands not created by user action.
> > 
> > > 
> > > Another support reason for passing through |apply()|:
> > > * Constructing a |CompositeEditCommand| is not a 1:1 relationship to
> > 'beforeinput'
> > >     * e.g. |CompositeEditCommand| could be created and executed through
> > |applyCommandToComposite()|, so it would be confusing to have command source
in
> > both parent command and children commands.
> > Creating |CompoisteEditCommand| other than UI event is implementation
details.
> > CommandSource for them is |kDOM|, or kInternal.
> > Given that CommandSource can be:
> > 
> > enum class EditCommandSouce {
> >   kUserAction,  // from menu, or keyboard shortcut
> >   kExecCommand, // document.execCommand()
> >   kInternal, // Blink creates a command for implementation.
> > }
> > 
> > > * Calling |apply()| is a 1:1 relationship to 'beforeinput'
> > >     * We should always fire a 'beforeinput' if there is a corresponding
> > |inputType| and the source is user action
> > 
> > Dispatching "beforeinput" should be implementation details of
> > CompositeEditCommand. Callers don't want to know it.
> > Also, this makes sure we don't dispatch "beforeinput" for
> > CompositeEditCommand::apply() created for non-user-action, e.g.
> > 
> > void dispatchBeforeInput(const CompositEditCommand& command, Element*
target) {
> >   DCHECK_EQ(command.command_source(), EditCommandSource::kUserAction);
> >   ... dispatch "beforeinput" to |target|.
> > }
> > 
> > > Or do you prefer the original solution or have other suggestions?
> > Yes, I prefer original solution.
> > 
> > > [3] https://codereview.chromium.org/2579253002 [EditCommandSource] Pass
source
> > to |CompositEditCommand| and |TypingCommand| (3/3)
> 
> OK I see, thanks for the detailed explanation!
> 
> However given that the |apply(source)| code is already in and these 11 CLs for
moving 'beforeinput' depend on that, are you OK with the following plan:
> 1. Keep |apply(source)| code for now
> 2. Land these 11 CLs as they don't conflict with where we store CommandSource
> 3. Add CommandSource to ctor gradually according your plan (e.g. as optional
parameter first => required parameter)
> 4. Remove |apply(source)| and use source from ctor instead
> 
> Or do you prefer reverting |apply(source)| and implement CommandSource in ctor
first?

For ease of blaming and keep git log clean, I prefer reverting |apply(source)|
and implement CommandSource in ctor first.

[chongz, 2017-01-11 23:38:42]

On 2017/01/11 05:50:02, Yosi_UTC9 wrote:
> On 2017/01/11 at 05:04:14, chongz wrote:
> > On 2017/01/11 02:20:53, Yosi_UTC9 wrote:
> > > On 2017/01/10 at 04:57:42, chongz wrote:
> > > > On 2017/01/10 04:15:44, Yosi_UTC9 wrote:
> > > > > On 2017/01/09 at 23:40:53, chongz wrote:
> > > > > > yosin@ PTAL, thanks!
> > > > > 
> > > > > Back from long vacation. I'm still in catch up mode.
> > > > > I'm confused that our original plan to have command source in
> > > > > CompositeEditCommand[1].
> > > > > But, now it is passed to apply() and unapply()[2].
> > > > > 
> > > > > Could you explain why do we need to pass command source to
> > > apply()/unapply()?
> > > > > 
> > > > > 
> > > > > [1] https://codereview.chromium.org/2574793002 [Editing] Store
> > > |CommandSource|
> > > > > in |CompositeEditCommand|
> > > > > [2] https://codereview.chromium.org/2581073003 [Editing] Introduce
> > > > > |EditCommandComposition::willUn/Reapply()| in prepare for
'beforeinput'
> > > (2/3)
> > > > 
> > > > Sure.
> > > > 
> > > > Yes, the original plan was to have command source in
> |CompositeEditCommand|
> > > through ctors, but according to comments in [1] I believe xiaocheng@ is
> > > concerned about having the extra parameter in too many irrelevant places
(to
> > > which I agree but don't have a better solution).
> > > > 
> > > First of all thanks for explanation. I understand discussion so far.
> > > 
> > > > The new solution is to pass command source in |apply()|, |unapply()| and
> > > |reapply|, where the affected files could be reduced dramatically [3], and
> it
> > > would be easier for future maintenance (e.g. Easier to add new commands
> without
> > > knowledge about command source).
> > > 
> > > I still fan of having CommandSource is a parameter of CompositeEditCommand
> ctor,
> > > because
> > >  - CommandSource is intrinsic property of CompositeEditCommand, it is
> lasting
> > > and never changed == const member variable. Making it is parameter of
apply
> > > implies CommandSource depends on execution flow.
> > >  - Amount of code changes is not good metrics. It can make future debt.
> > >   To split CL, we can make CommandSource as optional parameter then change
> call
> > > sites incrementally. Finally, we can make CommandSouce to required
> parameter. We
> > > may want to defer decision whether required or not.
> > >  - New commands should know care about CommandSouce. We don't need to
> dispatch
> > > "beforeinput" for commands not created by user action.
> > > 
> > > > 
> > > > Another support reason for passing through |apply()|:
> > > > * Constructing a |CompositeEditCommand| is not a 1:1 relationship to
> > > 'beforeinput'
> > > >     * e.g. |CompositeEditCommand| could be created and executed through
> > > |applyCommandToComposite()|, so it would be confusing to have command
source
> in
> > > both parent command and children commands.
> > > Creating |CompoisteEditCommand| other than UI event is implementation
> details.
> > > CommandSource for them is |kDOM|, or kInternal.
> > > Given that CommandSource can be:
> > > 
> > > enum class EditCommandSouce {
> > >   kUserAction,  // from menu, or keyboard shortcut
> > >   kExecCommand, // document.execCommand()
> > >   kInternal, // Blink creates a command for implementation.
> > > }
> > > 
> > > > * Calling |apply()| is a 1:1 relationship to 'beforeinput'
> > > >     * We should always fire a 'beforeinput' if there is a corresponding
> > > |inputType| and the source is user action
> > > 
> > > Dispatching "beforeinput" should be implementation details of
> > > CompositeEditCommand. Callers don't want to know it.
> > > Also, this makes sure we don't dispatch "beforeinput" for
> > > CompositeEditCommand::apply() created for non-user-action, e.g.
> > > 
> > > void dispatchBeforeInput(const CompositEditCommand& command, Element*
> target) {
> > >   DCHECK_EQ(command.command_source(), EditCommandSource::kUserAction);
> > >   ... dispatch "beforeinput" to |target|.
> > > }
> > > 
> > > > Or do you prefer the original solution or have other suggestions?
> > > Yes, I prefer original solution.
> > > 
> > > > [3] https://codereview.chromium.org/2579253002 [EditCommandSource] Pass
> source
> > > to |CompositEditCommand| and |TypingCommand| (3/3)
> > 
> > OK I see, thanks for the detailed explanation!
> > 
> > However given that the |apply(source)| code is already in and these 11 CLs
for
> moving 'beforeinput' depend on that, are you OK with the following plan:
> > 1. Keep |apply(source)| code for now
> > 2. Land these 11 CLs as they don't conflict with where we store
CommandSource
> > 3. Add CommandSource to ctor gradually according your plan (e.g. as optional
> parameter first => required parameter)
> > 4. Remove |apply(source)| and use source from ctor instead
> > 
> > Or do you prefer reverting |apply(source)| and implement CommandSource in
ctor
> first?
> 
> For ease of blaming and keep git log clean, I prefer reverting |apply(source)|
> and implement CommandSource in ctor first.

To revert [4] I will have to revert [5] as well because [5] is using |source|.
Instead I uploaded a CL to move command source from |apply(source)| to ctor,
PTAL!
https://codereview.chromium.org/2627103003  [EditCommandSource] Pass source
through |CompositeEditCommand| ctor instead of |apply(source)|

Also I'm not sure why reverting would keep blaming & git log clean as it will
still create entries in git log, no?

[4] https://crrev.com/2579253002 [EditCommandSource] Pass source to
|CompositEditCommand| and |TypingCommand| (3/3)
[5] https://crrev.com/2583993002 [Editing] Introduce
|CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3)