Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558643003:
[InputEvent] Move 'beforeinput' logic into |CompositeEditCommand::willApplyEditing()| (3/3) (Closed)
Description[InputEvent] Move 'beforeinput' logic into |CompositeEditCommand::willApplyEditing()| (3/3)
This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003.
Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in
1. 'CompositeEditCommand.cpp', and
2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()'
Note: 2) Should be removed after https://crbug.com/675820
This CL:
1. Moved/added InputEvent related attributes to |CompositeEditCommand|
* e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()|
2. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp'
3. Retains original event order WRT 'compositionupdate'.
To help reviewing, here is a simple doc describing the following plans:
https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd4ZkgJs5GPde2ZsDy0/edit?usp=sharing
This CL also fixed https://crbug.com/652436
BUG=670035
Patch Set 1 #
Total comments: 4
Patch Set 2 : yosin's review, fix nits #
Total comments: 2
Patch Set 3 : Rebase on EditCommandSource and willApply() CLs #
Total comments: 9
Patch Set 4 : Retain the order of firing 'beforeinput' before 'compositionupdate' #
Total comments: 9
Patch Set 5 : xiaocheng's review: Isolate |UndoStep| code; Move attribuates to private #
Total comments: 75
Patch Set 6 : xiaocheng's review 2: Remove unrelated changes and added more checks #
Total comments: 4
Patch Set 7 : xiaocheng's review 3: Rebase and remove updateStyleAndLayoutIgnorePendingStylesheets() #Messages
Total messages: 78 (64 generated)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [InputEvent] Add Editor::willApplyEditing() and unifies 'beforeinput' logic BUG=670035 ========== to ========== [InputEvent] Add Editor::willApplyEditing() and unifies 'beforeinput' logic Before CL we are firing 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| BUG=670035 ==========
Description was changed from ========== [InputEvent] Add Editor::willApplyEditing() and unifies 'beforeinput' logic Before CL we are firing 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| BUG=670035 ========== to ========== [InputEvent] Add Editor::willApplyEditing() and unifies 'beforeinput' logic Before CL we are firing 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed 652429 and 652429. BUG=670035 ==========
Description was changed from ========== [InputEvent] Add Editor::willApplyEditing() and unifies 'beforeinput' logic Before CL we are firing 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed 652429 and 652429. BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic Before CL we fire 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed 652429 and 652429. BUG=670035 ==========
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput'
logic
Before CL we fire 'beforeinput' in multiple places, which causes duplicated code
and it's difficult to maintain.
This CL introduced:
* |CompositeEditCommand::willApply()|
* |TypingCommand::willAddTypingToOpenCommand()|
* |Editor::willApplyEditing()|
and only fires 'beforeinput' in
* |Editor::willApplyEditing()|
* |Editor::redo()|
* |Editor::undo()|
InputEvent related attributes are now bound to |EditCommand|
* e.g. |inputType|, |data|, |dataTransfer| and |targetRanges|
This CL also fixed 652429 and 652429.
BUG=670035
==========
to
==========
[InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput'
logic
Before CL we fire 'beforeinput' in multiple places, which causes duplicated code
and it's difficult to maintain.
This CL introduced:
* |CompositeEditCommand::willApply()|
* |TypingCommand::willAddTypingToOpenCommand()|
* |Editor::willApplyEditing()|
and only fires 'beforeinput' in
* |Editor::willApplyEditing()|
* |Editor::redo()|
* |Editor::undo()|
InputEvent related attributes are now bound to |EditCommand|
* e.g. |inputType|, |data|, |dataTransfer| and |targetRanges|
This CL also
* Fixed 652429 and 652429
* Changed event order from |"beforeinput"->"compositionupdate"->"input"| to
|"compositionupdate"->"beforeinput"->"input"| to match Safari
* I recalled that Edge has fixed their bug so the new order would be
more
reasonable
BUG=670035
==========
Description was changed from
==========
[InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput'
logic
Before CL we fire 'beforeinput' in multiple places, which causes duplicated code
and it's difficult to maintain.
This CL introduced:
* |CompositeEditCommand::willApply()|
* |TypingCommand::willAddTypingToOpenCommand()|
* |Editor::willApplyEditing()|
and only fires 'beforeinput' in
* |Editor::willApplyEditing()|
* |Editor::redo()|
* |Editor::undo()|
InputEvent related attributes are now bound to |EditCommand|
* e.g. |inputType|, |data|, |dataTransfer| and |targetRanges|
This CL also
* Fixed 652429 and 652429
* Changed event order from |"beforeinput"->"compositionupdate"->"input"| to
|"compositionupdate"->"beforeinput"->"input"| to match Safari
* I recalled that Edge has fixed their bug so the new order would be
more
reasonable
BUG=670035
==========
to
==========
[InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput'
logic
Before CL we fire 'beforeinput' in multiple places, which causes duplicated code
and it's difficult to maintain.
This CL introduced:
* |CompositeEditCommand::willApply()|
* |TypingCommand::willAddTypingToOpenCommand()|
* |Editor::willApplyEditing()|
and only fires 'beforeinput' in
* |Editor::willApplyEditing()|
* |Editor::redo()|
* |Editor::undo()|
InputEvent related attributes are now bound to |EditCommand|
* e.g. |inputType|, |data|, |dataTransfer| and |targetRanges|
This CL also
* Fixed https://crbug.com/652429 and https://crbug.com/652429
* Changed event order from |"beforeinput"->"compositionupdate"->"input"| to
|"compositionupdate"->"beforeinput"->"input"| to match Safari
* I recalled that Edge has fixed their bug so the new order would be
more
reasonable
BUG=670035
==========
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic Before CL we fire 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also * Fixed https://crbug.com/652429 and https://crbug.com/652429 * Changed event order from |"beforeinput"->"compositionupdate"->"input"| to |"compositionupdate"->"beforeinput"->"input"| to match Safari * I recalled that Edge has fixed their bug so the new order would be more reasonable BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic Before CL we fire 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652429 BUG=670035 ==========
Patchset #1 (id:20001) has been deleted
chongz@chromium.org changed reviewers: + yosin@chromium.org
yosin@ Here is the improvement patch we have discussed, PTAL, thanks!
It is strange that we need to ask command to ask editor about canceling. EditCommand::willApply() should be self standing and we should not have contextual state |m_executingCommandFromDOM| in |Editor| which is error prone. We can represent it in |EditCommand| member variable to express it is for DOM or C++ code. https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:441: DCHECK(frame); No need to have |DCHECK(frame)| since next line uses it. https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:119: // Returns |false| to abort applying command. Is "cancel" better than "abort"? Should we mention about "beforeinput" event here? // Returns |false| to cancel applying command. Only "beforeinput" event handler asks to cancel.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> It is strange that we need to ask command to ask editor about canceling. Do you mean we should fire 'beforeinput' in |EditCommand::willApply()| instead? > EditCommand::willApply() should be self standing and we should not have > contextual state |m_executingCommandFromDOM| in |Editor| which is error prone. > We can represent it in |EditCommand| member variable to express it is for DOM or > C++ code. Do you mean we should pass |source| down to |EditCommand|s from |EditorCommand::executeBackColor/Copy/...|? I will create another patch for it since there are so many |execute*| functions. https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:441: DCHECK(frame); On 2016/12/07 07:51:27, Yosi_UTC9 wrote: > No need to have |DCHECK(frame)| since next line uses it. Done. https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2558643003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:119: // Returns |false| to abort applying command. On 2016/12/07 07:51:27, Yosi_UTC9 wrote: > Is "cancel" better than "abort"? > Should we mention about "beforeinput" event here? > > // Returns |false| to cancel applying command. Only "beforeinput" event handler > asks to cancel. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic Before CL we fire 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652429 BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic Before CL we fire 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
On 2016/12/07 at 15:22:21, chongz wrote: > > It is strange that we need to ask command to ask editor about canceling. > > Do you mean we should fire 'beforeinput' in |EditCommand::willApply()| instead? Yes. We also want to move |Editor::appliedEditing()| to |CompositeEditingCommand::apply()|. In this way, we can encapsulate event dispatching into |CompositeEditCommand::apply()|. BTW, I would like to rename |CompositeEditCommand| to |ComplexEditClosure|, to make name implies it hold state and avoid conflict with |Editor::Command|, and compositor. I think |Editor| class does too much thing. |Editor| just expose subcomponetns, e.g. undo stack, marker, last editing command, etc, and move implementations of |executeXXX()| of |EditorInternalCommand::execute()| to |executeXXX()|. > > EditCommand::willApply() should be self standing and we should not have > > contextual state |m_executingCommandFromDOM| in |Editor| which is error prone. > > > We can represent it in |EditCommand| member variable to express it is for DOM or > > C++ code. > > Do you mean we should pass |source| down to |EditCommand|s from |EditorCommand::executeBackColor/Copy/...|? > > I will create another patch for it since there are so many |execute*| functions. Thanks! Event if we have 140+ execCommands, it is almost mechanical.
xiaochengh@chromium.org changed reviewers: + xiaochengh@chromium.org
https://codereview.chromium.org/2558643003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2558643003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:74: // 1. Fire 'compositionupdate' event This patch switches the ordering of compositionupdate and beforeinput events. I haven't found any spec on the ordering of the two events. Could you help me double check that the ordering switch is fine?
https://codereview.chromium.org/2558643003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2558643003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:74: // 1. Fire 'compositionupdate' event On 2016/12/19 03:25:44, Xiaocheng wrote: > This patch switches the ordering of compositionupdate and beforeinput events. > > I haven't found any spec on the ordering of the two events. Could you help me > double check that the ordering switch is fine? Sorry for not pointing it out in advance. I've re-opened spec issue (https://github.com/w3c/uievents/issues/66) and I believe it's the correct thing to do. Also note that Safari's implementation is switched as well (i.e. Doesn't follow spec)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/19 at 20:53:44, chongz wrote: > https://codereview.chromium.org/2558643003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/2558643003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:74: // 1. Fire 'compositionupdate' event > On 2016/12/19 03:25:44, Xiaocheng wrote: > > This patch switches the ordering of compositionupdate and beforeinput events. > > > > I haven't found any spec on the ordering of the two events. Could you help me > > double check that the ordering switch is fine? > > Sorry for not pointing it out in advance. > > I've re-opened spec issue (https://github.com/w3c/uievents/issues/66) and I believe it's the correct thing to do. > Also note that Safari's implementation is switched as well (i.e. Doesn't follow spec) Thanks. Let's wait for a positive response before moving the firing of beforeinput into willApply.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic Before CL we fire 'beforeinput' in multiple places, which causes duplicated code and it's difficult to maintain. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. |EditCommandComposition::willUnapply()| 2. |EditCommandComposition::willReapply()| 3. |CompositeEditCommand::willApplyEditing()| 4. |InputMethodController.cpp::insertTextDuringCompositionWithEvents()| Note: 4) Should be removed after https://crbug.com/675820 This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. |EditCommandComposition::willUnapply()| 2. |EditCommandComposition::willReapply()| 3. |CompositeEditCommand::willApplyEditing()| 4. |InputMethodController.cpp::insertTextDuringCompositionWithEvents()| Note: 4) Should be removed after https://crbug.com/675820 This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. This CL introduced: * |CompositeEditCommand::willApply()| * |TypingCommand::willAddTypingToOpenCommand()| * |Editor::willApplyEditing()| and only fires 'beforeinput' in * |Editor::willApplyEditing()| * |Editor::redo()| * |Editor::undo()| InputEvent related attributes are now bound to |EditCommand| * e.g. |inputType|, |data|, |dataTransfer| and |targetRanges| This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into "CompositeEditCommand.cpp" 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into "CompositeEditCommand.cpp" 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into "CompositeEditCommand.cpp" 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. "CompositeEditCommand.cpp", and 2. "InputMethodController.cpp::insertTextDuringCompositionWithEvents()" Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into "CompositeEditCommand.cpp" 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. 'CompositeEditCommand.cpp', and 2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()' Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp' 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
> Thanks. Let's wait for a positive response before moving the firing of beforeinput into willApply. I'm not sure how long it will take to get a response, so I've added some extra code to retain the original order for now, and we can removed them in another CL later. I've rebased the CL and moved 'beforeinput' logic, PTAL, thanks! (And sorry for the big CL...) https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-execcommand.html (right): https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-execcommand.html:88: // testExecCommandInputType('paste', null, 'insertFromPaste'); |execCommand('paste')| will trigger a |TextEvent| to do the paste, will fix this in another patch. https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.h (right): https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.h:423: const CompositeEditCommand*); Currently used by "Editor.cpp" and "CompositeEditCommand.cpp", could be moved to "CompositeEditCommand.cpp" after we've moved 'input' event logic into "CompositeEditCommand.cpp" as well. https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:1040: // 'beforeinput' for JS triggered paste. See comments in "inputevent-execcommand.html". https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/Editor.h (left): https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.h:179: RangeVector* getTargetRanges() const; Moved to |CompositeEditCommand::targetRangesForInputEvent()|. https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (left): https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:57: InputEvent::InputType inputType() const override; We don't need |inputType()| for |EditCommandComposition| and |UndoStep| -- It could only be undo or redo. https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditCommand.h (left): https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditCommand.h:46: virtual InputEvent::InputType inputType() const; Moved all |InputEvent| related stuff to |CompositeEditCommand| since we can only "apply()" a |CompositeEditCommand|. https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (left): https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:114: InputEvent::InputType InputTypeFromCommandType( Distributed to the ctor of each |CompositeEditCommand|s. https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:194: RangeVector* RangesFromCurrentSelectionOrExtendCaret( Moved into "TypingCommand.cpp". https://codereview.chromium.org/2558643003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2720: RangeVector* Editor::Command::getTargetRanges() const { Moved to |TypingCommand::targetRangesForInputEvent()| as we only have special target ranges for deletions. (Otherwise we use current selection as target ranges) https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-compositionupdate.html (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-compositionupdate.html:34: ); Added this test to keep track of event order. https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2091: bool dispatchBeforeInputEvent(EditCommandSource source, Will move back to "CompositeEditCommand.cpp" after we have resolved the event order issue.
Description was changed from ========== [InputEvent] Introduce |Editor::willApplyEditing()| and unifies 'beforeinput' logic (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. 'CompositeEditCommand.cpp', and 2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()' Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp' 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Move 'beforeinput' logic into |CompositeEditCommand::willApplyEditing()| (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. 'CompositeEditCommand.cpp', and 2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()' Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp' 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for the hard work! Since the patch is big and there seems to be a lot of code moving, could you add notes to the patch to better illustrate how the code is moved? - For each chunk that's moved to elsewhere, comment with where it's moved to (may be multiple places) - For each chunk that's moved from elsewhere, comment with where it's moved from (may be multiple places) Sorry for lengthening the iteration... But I can't maintain the code-moving graph in my brain... > This CL: > 1. Removed |inputType()| from |UndoStep| This seems orthogonal to 'beforeinput'. Could you isolate it out into another patch? https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:235: if (!willApplyEditing(source)) Yeah, this should be the right place to fire the event. Thanks for catching it. https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:80: // Fires "beforeinput" and will returns |false| to cancel unapply / reapply if nit: s/returns/return/ https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.h (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.h:61: InputEvent::InputType inputType() const final; What's the reason of exposing these functions? They should be private if callers should only call them virtually via CompositeEditCommand's functions. https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.h:110: InputEvent::InputType inputType() const final; Same as for ReplaceSelectionCommand
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [InputEvent] Move 'beforeinput' logic into |CompositeEditCommand::willApplyEditing()| (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. 'CompositeEditCommand.cpp', and 2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()' Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Removed |inputType()| from |UndoStep| 2. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 3. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp' 4. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Move 'beforeinput' logic into |CompositeEditCommand::willApplyEditing()| (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. 'CompositeEditCommand.cpp', and 2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()' Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 2. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp' 3. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
xiaochengh@ Thanks for reviewing my code and sorry for the big patch... 1. I've added 2 tables to the doc to help getting an overview of the moving, and 2. I've also added inline comments as requested to help locating the trunk. Thanks for your time and PTAL again :) https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:80: // Fires "beforeinput" and will returns |false| to cancel unapply / reapply if On 2016/12/20 05:49:57, Xiaocheng wrote: > nit: s/returns/return/ Done. https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.h (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.h:61: InputEvent::InputType inputType() const final; On 2016/12/20 05:49:57, Xiaocheng wrote: > What's the reason of exposing these functions? > > They should be private if callers should only call them virtually via > CompositeEditCommand's functions. Done. https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right): https://codereview.chromium.org/2558643003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.h:110: InputEvent::InputType inputType() const final; On 2016/12/20 05:49:57, Xiaocheng wrote: > Same as for ReplaceSelectionCommand Done. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2050: DispatchEventResult dispatchBeforeInputInsertText(EventTarget* target, See header file. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2073: InputEvent::EventIsComposing isComposingFromCommand( See header file. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.h (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.h:431: const RangeVector*); Combined into a single |dispatchBeforeInputEvent()| below. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.h (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.h:426: InputEvent::EventIsComposing isComposingFromCommand( Moved from |Editor.cpp::isComposingFromCommand()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.h:428: InputEvent::EventCancelable isCancelableFromCommand( New method. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.h:434: bool dispatchBeforeInputEvent(EditCommandSource, Combined from 4x |dispatchBeforeInput*()| above. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/Editor.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:128: InputEvent::EventIsComposing isComposingFromCommand( Moved to |EditingUtilities.cpp::isComposingFromCommand()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:626: bool Editor::deleteSelectionAfterDraggingWithEvents( See header file. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:1073: if (dispatchBeforeInputDataTransfer(findEventTargetFromSelection(), Removed, cut was covered by |DeleteSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:1135: if (dispatchBeforeInputDataTransfer(findEventTargetFromSelection(), Removed, paste was covered by |ReplaceSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/Editor.h (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.h:179: RangeVector* getTargetRanges() const; Moved to |TypingCommand::targetRangesForInputEvent()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.h:289: bool deleteSelectionAfterDraggingWithEvents( Removed, covered by |DeleteSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.h:293: bool replaceSelectionAfterDraggingWithEvents(Element* dropTarget, Removed, covered by |ReplaceSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp:64: if (dispatchBeforeInputInsertText(evt->target(), keyEvent->text) != Removed, typing was covered by |TypingCommand => willApplyEditing() || willAddTypingToOpenCommand()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:122: DispatchEventResult result = dispatchBeforeInputFromComposition( Kept, still fire 'beforeinput' before 'compositionupdate'. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:355: if (dispatchBeforeInputInsertText(document().focusedElement(), text) != Removed, inserting text was covered by |TypingCommand => willApplyEditing() || willAddTypingToOpenCommand()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:391: dispatchBeforeInputFromComposition( Removed. This is a dummy 'beforeinput' with no actual editing behavior and doesn't follow spec, will do further work on IME later. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:780: dispatchBeforeInputEditorCommand( Removed, deleting text was covered by |TypingCommand => willApplyEditing() || willAddTypingToOpenCommand()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:154: return dispatchBeforeInputEvent(source, m_document->frame(), m_document, Added to cover undo command from "EditorCommand.cpp::Command::execute()". Also fixes https://crbug.com/652436 - We should fire undo/redo on |document|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:161: return dispatchBeforeInputEvent(source, m_document->frame(), m_document, Added to cover redo command from "EditorCommand.cpp::Command::execute()". Also fixes https://crbug.com/652436 - We should fire undo/redo on |document|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:281: return dispatchBeforeInputEvent( Covers all hot keys, context menu, cut&paste, drag&drop, spell checker and the first text editing action from typing or IME. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:303: return new RangeVector(1, document().frame()->selection().firstRange()); New implementation, also fixes https://crbug.com/652429 - We should always return current selection for ContentEditable if no better choice. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:120: virtual InputEvent::InputType inputType() const; Moved from |EditCommand::inputType()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:122: virtual String textDataForInputEvent() const; Moved from |EditCommand::textDataForInputEvent()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:123: virtual DataTransfer* dataTransferForInputEvent() const; New method. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:124: virtual RangeVector* targetRangesForInputEvent() const; New method. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditCommand.h (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditCommand.h:46: virtual InputEvent::InputType inputType() const; Moved to |CompositeEditCommand::inputType()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditCommand.h:61: virtual String textDataForInputEvent() const; Moved to |CompositeEditCommand::textDataForInputEvent()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:114: InputEvent::InputType InputTypeFromCommandType( Removed. Input types could be get from |CompositeEditCommand::inputType()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:194: RangeVector* RangesFromCurrentSelectionOrExtendCaret( Moved to |TypingCommand.cpp::targetRangesFromCurrentSelectionOrExtendCaret()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2655: if (dispatchBeforeInputEditorCommand( Removed, most |Editor::Command| are covered in |CompositeEditCommand::willApplyEditing()|, some continues deletion commands are covered by |TypingCommand::willAddTypingToOpenCommand()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2720: RangeVector* Editor::Command::getTargetRanges() const { See header file, moved to |TypingCommand::targetRangesForInputEvent()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:2130: return m_documentFragment->textContent(); New implementation. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:2139: textDataForInputEvent(), createMarkup(m_documentFragment)); New implementation. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:275: return CompositeEditCommand::textDataForInputEvent(); Moved down to group with other |InputEvent| methods. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:158: RangeVector* targetRangesFromCurrentSelectionOrExtendCaret( Moved from |EditorCommand.cpp::RangesFromCurrentSelectionOrExtendCaret()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:549: String TypingCommand::textDataForInputEvent() const { Moved down to group with other |InputEvent| methods. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:555: RangeVector* TypingCommand::targetRangesForInputEvent() const { Moved from |EditorCommand.cpp::Command::getTargetRanges()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:591: bool result = willApplyEditing(source); New implementation, covers all continues typing actions from 1. |EditorKeyBindings.cpp::handleEditingKeyboardEvent()::insertText()| 2. IME 3. Some deletion commands in |EditorCommand.cpp::Command::execute()| https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:844: dispatchBeforeInputDataTransfer( Removed, spell checker was covered by |ReplaceSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.cpp:160: inputEventInit.setRanges(*ranges); 'input' event always has null target ranges. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/DragController.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:604: if (!innerFrame->editor().deleteSelectionAfterDraggingWithEvents( Removed, drag deletion are covered by |DeleteSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:615: if (!innerFrame->editor().replaceSelectionAfterDraggingWithEvents( Removed, drop replacing are covered by |ReplaceSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:622: if (!innerFrame->editor().replaceSelectionAfterDraggingWithEvents( Removed, drop replacing are covered by |ReplaceSelectionCommand => willApplyEditing()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:637: if (!innerFrame->editor().replaceSelectionAfterDraggingWithEvents( Removed, drop replacing are covered by |ReplaceSelectionCommand => willApplyEditing()|.
Thanks for the doc and the inline comments, which help a lot (and offer me chance to further lengthening the iteration...) https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2128: if (frame->document()->frame() != frame) Is |frame->document()| still non-null after the event? Is it safer to compare |frame->document()| with the original document, like what SpellChecker currently does? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:89: #include "platform/clipboard/ClipboardMimeTypes.h" What needs it? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:108: if (!target || !target->isConnected()) This part doesn't seem relevant to beforeinput. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:1209: if (m_preventRevealSelection || !m_frame->selection().isAvailable()) This part seems to be a bug fix. Does any test case reach here? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:161: // TODO(chongz): Fire 'input' event. Is it a drive-by removal of a stale TODO? If so, could you isolate it out into another patch? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:87: // 1. Fire 'compositionupdate' event Please revise the comments here. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:133: if (!frame.document() || frame.document()->frame() != &frame) This seems irrelevant to beforeinput. If it's a bug fix, could you isolate it out into another patch? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:303: return new RangeVector(1, document().frame()->selection().firstRange()); On 2016/12/20 at 23:27:52, chongz wrote: > New implementation, also fixes https://crbug.com/652429 - We should always return current selection for ContentEditable if no better choice. What are the ranges used by the current implementation? Is it possible to somehow separate refactoring with behavior changes? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:2130: return m_documentFragment->textContent(); On 2016/12/20 at 23:27:53, chongz wrote: > New implementation. What text data does the current implementation use when firing bebeforeinput for ReplaceSelectionCommand? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:2139: textDataForInputEvent(), createMarkup(m_documentFragment)); On 2016/12/20 at 23:27:53, chongz wrote: > New implementation. Similar question as above. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:555: RangeVector* TypingCommand::targetRangesForInputEvent() const { On 2016/12/20 at 23:27:53, chongz wrote: > Moved from |EditorCommand.cpp::Command::getTargetRanges()|. This doesn't seem like code moving. What's the relationship between WebEditingCommandType and InputType? And the cases here are not 1:1 to |EditorCommand.cpp:Command::getTargetRanges()|. Could you clarify? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.h (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.h:95: const RangeVector*); It doesn't seem relevant to beforeinput. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/DragController.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:608: // 'beforeinput' event handler may destroy drop frame. Is it just 'beforeinput'? In fact is it a bug that the current drag&drop doesn't abort even if other events fired by DeleteSelectionCommand destroys the frame? https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:643: We should check whether the frame is destroyed here, and abort if so.
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [InputEvent] Move 'beforeinput' logic into |CompositeEditCommand::willApplyEditing()| (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. 'CompositeEditCommand.cpp', and 2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()' Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 2. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp' 3. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652429 and https://crbug.com/652436 BUG=670035 ========== to ========== [InputEvent] Move 'beforeinput' logic into |CompositeEditCommand::willApplyEditing()| (3/3) This CL is based on http://crrev.com/2583993002 and http://crrev.com/2581073003. Before CL we fire 'beforeinput' in multiple locations, after CL we fire 'beforeinput' in 1. 'CompositeEditCommand.cpp', and 2. 'InputMethodController.cpp::insertTextDuringCompositionWithEvents()' Note: 2) Should be removed after https://crbug.com/675820 This CL: 1. Moved/added InputEvent related attributes to |CompositeEditCommand| * e.g. |textDataForInputEvent()|, |targetRangesForInputEvent()| 2. Moved most 'beforeinput' logic into 'CompositeEditCommand.cpp' 3. Retains original event order WRT 'compositionupdate'. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... This CL also fixed https://crbug.com/652436 BUG=670035 ==========
xiaochengh@ Thanks for reviewing! I've removed unrelated changes, added some checks and added comments for clarifications. PTAL again, thanks! https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2114: if (hasRichlyEditableStyle(*(target->toNode())) || !dataTransfer) { This logic for data vs. dataTransfer are only used by paste, drop and spellchecker, so it makes more sense to be moved to |ReplaceSelectionCommand::textDataForInputEvent()| and |ReplaceSelectionCommand::dataTransferForInputEvent()|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2128: if (frame->document()->frame() != frame) On 2016/12/21 02:56:38, Xiaocheng wrote: > Is |frame->document()| still non-null after the event? Added check for |frame->document()|, but according to my tests it's always non-null. Also moved |isConnected()| check here. > > Is it safer to compare |frame->document()| with the original document, like what > SpellChecker currently does? Yeah yosin@ suggested this before, but according to my tests |frame->document()| won't change, see discussion: https://codereview.chromium.org/2258663003/diff/120001/third_party/WebKit/Sou... Also see "fast/events/inputevents/beforeinput-remove-iframe-crash.html" for test cases, where SpellChecker was not tested and I suspect it will fail. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:89: #include "platform/clipboard/ClipboardMimeTypes.h" On 2016/12/21 02:56:38, Xiaocheng wrote: > What needs it? My mistake, removed. It was added for |dispatchBeforeInput()| but then it was moved back to "EditingUtilities.cpp". https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:108: if (!target || !target->isConnected()) On 2016/12/21 02:56:38, Xiaocheng wrote: > This part doesn't seem relevant to beforeinput. Moved to the end of 'EditingUtilities.cpp::dispatchBeforeInputEvent()', so we don't apply the command if target was disconnected from the document. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:1209: if (m_preventRevealSelection || !m_frame->selection().isAvailable()) On 2016/12/21 02:56:38, Xiaocheng wrote: > This part seems to be a bug fix. Does any test case reach here? The test case is "fast/events/inputevents/beforeinput-remove-iframe-crash.html". Originally we have |m_frame->document()->frame() != m_frame| check after every 'beforeinput', this CL moved the check into |dispatchBeforeInput()| and returns false in this case. Most callers was able to catch return value from |dispatchBeforeInput()| and abort accordingly, but some callers such as |Editor::deleteWithDirection()| and Drag&Drop won't use/pass the return value, so we need to check here. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:161: // TODO(chongz): Fire 'input' event. On 2016/12/21 02:56:39, Xiaocheng wrote: > Is it a drive-by removal of a stale TODO? > > If so, could you isolate it out into another patch? Yes, will isolate into another patch. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:87: // 1. Fire 'compositionupdate' event On 2016/12/21 02:56:39, Xiaocheng wrote: > Please revise the comments here. Reverted comments since there is no behavior change. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:133: if (!frame.document() || frame.document()->frame() != &frame) On 2016/12/21 02:56:38, Xiaocheng wrote: > This seems irrelevant to beforeinput. > > If it's a bug fix, could you isolate it out into another patch? Reverted, will put in another patch. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:303: return new RangeVector(1, document().frame()->selection().firstRange()); On 2016/12/21 02:56:39, Xiaocheng wrote: > On 2016/12/20 at 23:27:52, chongz wrote: > > New implementation, also fixes https://crbug.com/652429 - We should always > return current selection for ContentEditable if no better choice. > > What are the ranges used by the current implementation? > > Is it possible to somehow separate refactoring with behavior changes? Reverted and will fix in another patch. We used to return |nullptr| for types other than |TypingCommand::delete*| and |InputType::InsertReplacementText|. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:2130: return m_documentFragment->textContent(); On 2016/12/21 02:56:39, Xiaocheng wrote: > On 2016/12/20 at 23:27:53, chongz wrote: > > New implementation. > > What text data does the current implementation use when firing bebeforeinput for > ReplaceSelectionCommand? Sorry for the confusion, this is actually the current behavior, but I just summarized the implementation from "EditingUtilities.cpp::dispatchBeforeInputDataTransfer()". Since the callers to |dispatchBeforeInputDataTransfer()| could only be paste, drop and spellchecker, and they both use |ReplaceSelectionCommand|, I think it makes more sense to have the logic here. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:2139: textDataForInputEvent(), createMarkup(m_documentFragment)); On 2016/12/21 02:56:39, Xiaocheng wrote: > On 2016/12/20 at 23:27:53, chongz wrote: > > New implementation. > > Similar question as above. Similar answer as above. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:555: RangeVector* TypingCommand::targetRangesForInputEvent() const { On 2016/12/21 02:56:39, Xiaocheng wrote: > On 2016/12/20 at 23:27:53, chongz wrote: > > Moved from |EditorCommand.cpp::Command::getTargetRanges()|. > > This doesn't seem like code moving. > > What's the relationship between WebEditingCommandType and InputType? Sorry for the confusion. *Inside Blink* |WebEditingCommandType| is *roughly* a *many-to-1-or-0* mapping to |InputType|: 1. Why *Inside Blink*: There are |InputType|s that only supported by other vendors; 2. *roughly*: SpellChecker doesn't go through "EditorCommand.cpp", so there isn't a |WebEditingCommandType| for it; 3. *many-to-1-or-0*: We have a |WebEditingCommandType => InputType| mapping if this command could be triggered from user action. > > And the cases here are not 1:1 to > |EditorCommand.cpp:Command::getTargetRanges()|. > > Could you clarify? For this case we have the mapping: 1. Both |WebEditingCommandType::Delete & DeleteBackward| are mapped to |InputType::DeleteContentBackward|; 2. Both |WebEditingCommandType::DeleteToBeginningOfParagraph & DeleteToEndOfParagraph| are mapped to |InputType::None|, so they don't fire 'beforeinput'. 3. Others are 1:1 See original mappings in |EditorCommand.cpp::InputTypeFromCommandType()|. The problem of the original implementation is * We use |InputTypeFromCommandType()| mapping for 'beforeinput', and * Wse "TypingCommand.cpp::inputTypeForTypingCommand()" for 'input'. (|commandType+granularity| -> |InputType|) This CL makes them both use |inputTypeForTypingCommand()|. There is no behavior change as we have the same |WebEditingCommandType => commandType+granularity| mapping. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.h (left): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.h:95: const RangeVector*); On 2016/12/21 02:56:39, Xiaocheng wrote: > It doesn't seem relevant to beforeinput. Reverted, will put in another patch. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/DragController.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:608: // 'beforeinput' event handler may destroy drop frame. On 2016/12/21 02:56:39, Xiaocheng wrote: > Is it just 'beforeinput'? > > In fact is it a bug that the current drag&drop doesn't abort even if other > events fired by DeleteSelectionCommand destroys the frame? I've tested 'input' and it won't crash, so I think it's only 'beforeinput'. Other events are dispatched through |dispatchScopedEvent()| and my debug log told me |ScopedEventQueue::instance()->shouldQueueEvents()| was true (however I'm not sure where the scope starts). We have to use |dispatchEvent()| for 'beforeinput' as JS might prevent default. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:643: On 2016/12/21 02:56:39, Xiaocheng wrote: > We should check whether the frame is destroyed here, and abort if so. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry I'm preempted by something else today and don't have enough time to go through the full patch. It seems that we can't land this patch in this year as holidays are coming... The patch is still pretty big, and it's still pretty hard to compare the behavior in every affected code path before and after the patch. To make the iteration more efficient, could you consider a further split? More specifically: 1. Make all code paths (those listed in your doc) pass an axtra parameter |hasFiredBeforeInput| to willApplyEditing and willUn/Reapply to indicate whether 'beforeinput' has been fired before reaching willXXX. The parameter value should be |true| for all code paths since they are all firing the event now. 2. Implement the willXXX functions and their helpers. willXXX functions should immediately return true if it's passed with |hasFiredBeforeInput| as true. 3. For each affected code path listed in your doc, stop firing 'beforeinput' and pass false to willXXX. This should also be split into several patches, each covering one or several related code paths. 4. Finish by cleaning up |hasFiredBeforeInput|. This pattern may also help resolving the ordering issue with 'compositionupdate', since we can just leave a true |hasFiredBeforeInput| in that code path. https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2558643003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2128: if (frame->document()->frame() != frame) On 2016/12/21 at 23:59:24, chongz wrote: > On 2016/12/21 02:56:38, Xiaocheng wrote: > > Is |frame->document()| still non-null after the event? > > Added check for |frame->document()|, but according to my tests it's always non-null. > Also moved |isConnected()| check here. > > > > > Is it safer to compare |frame->document()| with the original document, like what > > SpellChecker currently does? > > Yeah yosin@ suggested this before, but according to my tests |frame->document()| won't change, see discussion: > https://codereview.chromium.org/2258663003/diff/120001/third_party/WebKit/Sou... > > Also see "fast/events/inputevents/beforeinput-remove-iframe-crash.html" for test cases, where SpellChecker was not tested and I suspect it will fail. Thanks for the clarification. I thought |frame->document()->frame == frame| should be always true... I may need to look into that later. https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right): https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:121: InputEvent, DataTransferReadable, Hmm... This is a behavior change. We were creating DataTransfers with DragAndDrop and InsertReplacementText for beforeinput, but now they both become InputEvent. The working draft doesn't say anything about InputEvent's DataTransfer, though... Anyway, could you clarify? https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:595: document().updateStyleAndLayoutIgnorePendingStylesheets(); This layout update is not very good, as we can't easily find the consumers of the clean layout. It may also update layout unnecessarily (when |result == false|). I uploaded another patch to put it at the correct places: https://codereview.chromium.org/2594313002. Please rebase after it lands.
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Sorry I'm preempted by something else today and don't have enough time to go > through the full patch. It seems that we can't land this patch in this year as > holidays are coming... No problem at all! Thank you for reviewing my code for the whole week, have a wonderful holiday! :) > > The patch is still pretty big, and it's still pretty hard to compare the > behavior in every affected code path before and after the patch. To make the > iteration more efficient, could you consider a further split? More specifically: > > 1. Make all code paths (those listed in your doc) pass an axtra parameter > |hasFiredBeforeInput| to willApplyEditing and willUn/Reapply to indicate whether > 'beforeinput' has been fired before reaching willXXX. The parameter value should > be |true| for all code paths since they are all firing the event now. > > 2. Implement the willXXX functions and their helpers. willXXX functions should > immediately return true if it's passed with |hasFiredBeforeInput| as true. > > 3. For each affected code path listed in your doc, stop firing 'beforeinput' and > pass false to willXXX. This should also be split into several patches, each > covering one or several related code paths. > > 4. Finish by cleaning up |hasFiredBeforeInput|. > > This pattern may also help resolving the ordering issue with > 'compositionupdate', since we can just leave a true |hasFiredBeforeInput| in > that code path. Thanks for the detailed plan! Will continue working on it after the holiday. https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/clipboard/DataTransfer.cpp (right): https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/clipboard/DataTransfer.cpp:121: InputEvent, DataTransferReadable, On 2016/12/22 06:26:20, Xiaocheng wrote: > Hmm... This is a behavior change. > > We were creating DataTransfers with DragAndDrop and InsertReplacementText for > beforeinput, but now they both become InputEvent. > > The working draft doesn't say anything about InputEvent's DataTransfer, > though... > > Anyway, could you clarify? Sorry for the confusion, there is actually no behavior change. Originally |DataTransfer| was used by 2 events: 1. ClipboardEvent (https://w3c.github.io/clipboard-apis/#clipboard-event-interfaces) * e.g. 'copy', 'paste' 2. DragEvent (https://html.spec.whatwg.org/multipage/interaction.html#dragevent) * 'e.g. 'dragstart', 'drop', etc. Now we are adding |InputEvent| which would also use |DataTransfer|. There is no conflicts because |InputEvent::'deleteByDrag'| etc. are new events and they are not going to replace drag&drop events. To help reducing future confusion I think we should change |DataTransferType| to: ``` enum DataTransferType { ClipboardEvent, DragEvent, InputEvent, }; ``` Or even just change it to `bool m_isDragEvent;` as we only using it internally for activating drop effect, etc. https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2558643003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:595: document().updateStyleAndLayoutIgnorePendingStylesheets(); On 2016/12/22 06:26:20, Xiaocheng wrote: > This layout update is not very good, as we can't easily find the consumers of > the clean layout. It may also update layout unnecessarily (when |result == > false|). > > I uploaded another patch to put it at the correct places: > https://codereview.chromium.org/2594313002. Please rebase after it lands. Rebased, thanks for the patch! |
