Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583993002:
[Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) (Closed)
Description[Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3)
This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL
https://crrev.com/2558643003)
This CL:
1. Added attribute |m_inputType| to |TypingCommand|
2. Added calling path |CompositeEditCommand::apply()| =>
*|CompositeEditCommand::willApply()| =>
*|CompositeEditCommand::willApplyEditing()|
3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| =>
*|TypingCommand::willAddTypingToOpenCommand()| =>
*|CompositeEditCommand::willApplyEditing()|
* Marks new methods.
The next step is to move 'beforeinput' logic to
|CompositeEditCommand::willApplyEditing()|.
This CL shouldn't have any behavior change.
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
BUG=670035
Committed: https://crrev.com/4d0f52c1162dae46a7902527775f2a95fc972423
Cr-Commit-Position: refs/heads/master@{#439633}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add TODO, remove |willApply()| #
Dependent Patchsets: Messages
Total messages: 36 (24 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 ========== Added method |CompositeEditCommand::willApply()| and |TypingCommand::willAddTypingToOpenCommand()| BUG=670035 ========== to ========== [InputEvent] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => |CompositeEditCommand::willApply()| => |CompositeEditCommand::willApplyEditing()| BUG=670035 ==========
Description was changed from ========== [InputEvent] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => |CompositeEditCommand::willApply()| => |CompositeEditCommand::willApplyEditing()| BUG=670035 ========== to ========== [InputEvent] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => |CompositeEditCommand::willApply()| => |CompositeEditCommand::willApplyEditing()| 2. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => |TypingCommand::willAddTypingToOpenCommand()| => |CompositeEditCommand::willApplyEditing()| The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ==========
Description was changed from ========== [InputEvent] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => |CompositeEditCommand::willApply()| => |CompositeEditCommand::willApplyEditing()| 2. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => |TypingCommand::willAddTypingToOpenCommand()| => |CompositeEditCommand::willApplyEditing()| The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ========== to ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => |CompositeEditCommand::willApply()| => |CompositeEditCommand::willApplyEditing()| 2. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => |TypingCommand::willAddTypingToOpenCommand()| => |CompositeEditCommand::willApplyEditing()| The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ==========
Description was changed from ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => |CompositeEditCommand::willApply()| => |CompositeEditCommand::willApplyEditing()| 2. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => |TypingCommand::willAddTypingToOpenCommand()| => |CompositeEditCommand::willApplyEditing()| The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ========== to ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 2. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ==========
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #1 (id:1) 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 ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 2. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ========== to ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added attribute |m_inputType| to |TypingCommand| 2. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2574793002) This CL: 1. Added attribute |m_inputType| to |TypingCommand| 2. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ========== to ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2558643003) This CL: 1. Added attribute |m_inputType| to |TypingCommand| 2. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ==========
chongz@chromium.org changed reviewers: + xiaochengh@chromium.org
xiaochengh@ PTAL, thanks!
xiaochengh@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent
https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:127: bool willApplyEditing(EditCommandSource); Please add a TODO: implementation note here since this function is not really firing beforeinput for now.
https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:256: return willApplyEditing(source); What's the difference between willApply() and willApplyEditing()?
https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:256: return willApplyEditing(source); On 2016/12/19 03:43:24, tkent wrote: > What's the difference between willApply() and willApplyEditing()? The reason is both |TypingCommand::willAddTypingToOpenCommand()| and |CompositeEditCommand::willApply()| should fire 'beforeinput', and these code could be shared. Having |CompositeEditCommand::willApplyEditing()| and put 'beforeinput' logic here could make the logic cleaner. Also later we could move |Editor::appliedEditing()| to |CompositeEditCommand::appliedEditing()|, so it would be consistent with |willApplyEditing()|. Also see comments in "CompositeEditCommand.h". https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:127: bool willApplyEditing(EditCommandSource); On 2016/12/19 03:33:57, Xiaocheng wrote: > Please add a TODO: implementation note here since this function is not really > firing beforeinput for now. Will do.
https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:256: return willApplyEditing(source); On 2016/12/19 at 06:48:57, chongz wrote: > On 2016/12/19 03:43:24, tkent wrote: > > What's the difference between willApply() and willApplyEditing()? > > The reason is both |TypingCommand::willAddTypingToOpenCommand()| and |CompositeEditCommand::willApply()| should fire 'beforeinput', and these code could be shared. > > Having |CompositeEditCommand::willApplyEditing()| and put 'beforeinput' logic here could make the logic cleaner. Also later we could move |Editor::appliedEditing()| to |CompositeEditCommand::appliedEditing()|, so it would be consistent with |willApplyEditing()|. > > Also see comments in "CompositeEditCommand.h". It didn't convince me. Why CompositeEditCommand::apply() doesn't call willApplyEditing() directly? Will you add more code to willApply()?
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...
tkent@ I've added TODO and removed |willApply()|, PTAL again, thanks! https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:256: return willApplyEditing(source); On 2016/12/19 08:22:18, tkent wrote: > On 2016/12/19 at 06:48:57, chongz wrote: > > On 2016/12/19 03:43:24, tkent wrote: > > > What's the difference between willApply() and willApplyEditing()? > > > > The reason is both |TypingCommand::willAddTypingToOpenCommand()| and > |CompositeEditCommand::willApply()| should fire 'beforeinput', and these code > could be shared. > > > > Having |CompositeEditCommand::willApplyEditing()| and put 'beforeinput' logic > here could make the logic cleaner. Also later we could move > |Editor::appliedEditing()| to |CompositeEditCommand::appliedEditing()|, so it > would be consistent with |willApplyEditing()|. > > > > Also see comments in "CompositeEditCommand.h". > > It didn't convince me. > > Why CompositeEditCommand::apply() doesn't call willApplyEditing() directly? > Will you add more code to willApply()? Oh I got what you mean. Yeah we don't need |willApply()| for now and we should only add it later if we really need it. Removed |willApply()| and uses |willApplyEditing()| directly. https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h (right): https://codereview.chromium.org/2583993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.h:127: bool willApplyEditing(EditCommandSource); On 2016/12/19 06:48:57, chongz wrote: > On 2016/12/19 03:33:57, Xiaocheng wrote: > > Please add a TODO: implementation note here since this function is not really > > firing beforeinput for now. > > Will do. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2583993002/#ps40001 (title: "Add TODO, remove |willApply()|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482193179143730,
"parent_rev": "a84ac00504a0427be608c384e213c4a98d5c21f0", "commit_rev":
"34510ea535983771bbb906d3cb46e79d67ce2ad3"}
Message was sent while issue was closed.
Description was changed from ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2558643003) This CL: 1. Added attribute |m_inputType| to |TypingCommand| 2. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 ========== to ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2558643003) This CL: 1. Added attribute |m_inputType| to |TypingCommand| 2. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 Review-Url: https://codereview.chromium.org/2583993002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2558643003) This CL: 1. Added attribute |m_inputType| to |TypingCommand| 2. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 Review-Url: https://codereview.chromium.org/2583993002 ========== to ========== [Editing] Introduce |CompositeEditCommand::willApplyEditing()| in prepare for 'beforeinput' (1/3) This is the first sub-patch (1/3) of unifying 'beforeinput' logic. (Original CL https://crrev.com/2558643003) This CL: 1. Added attribute |m_inputType| to |TypingCommand| 2. Added calling path |CompositeEditCommand::apply()| => *|CompositeEditCommand::willApply()| => *|CompositeEditCommand::willApplyEditing()| 3. Added calling path |static TypingCommand::deleteSelection()/insertText()/...| => *|TypingCommand::willAddTypingToOpenCommand()| => *|CompositeEditCommand::willApplyEditing()| * Marks new methods. The next step is to move 'beforeinput' logic to |CompositeEditCommand::willApplyEditing()|. This CL shouldn't have any behavior change. To help reviewing, here is a simple doc describing the following plans: https://docs.google.com/a/chromium.org/document/d/1M9bQaLKdRpR1qJ9i7zZKN-hKwd... BUG=670035 Committed: https://crrev.com/4d0f52c1162dae46a7902527775f2a95fc972423 Cr-Commit-Position: refs/heads/master@{#439633} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4d0f52c1162dae46a7902527775f2a95fc972423 Cr-Commit-Position: refs/heads/master@{#439633}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/2634633002/ by chongz@chromium.org. The reason for reverting is: We want to revert all CLs and start over the refactor process, see: https://crbug.com/673789.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
