Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576903002:
[EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) (Closed)
Description[EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3)
This is the sub-patch (2/3) of supporting |EditCommandSource| in
|CompositeEditCommand| (https://crrev.com/2574793002).
This CL:
1. Added method |Editor::replaceSelectionForSpellChecker()| for
|SpellChecker::replaceMisspelledRange()| to hide editor details;
2. Passed command source from |Editor::Command| to various methods in |Editor|,
such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|,
|replaceSelectionWithText()| etc.
Note:
It's OK to add extra |EditCommandSource| parameter to those methods because
they are going to be moved to "EditorCommand.cpp" (according to the editing
refactor doc), which means they should share the similar pattern as other
|static execute*()| methods.
This CL shouldn't have any behavior change.
Next CL will pass command source down to |CompositeEditCommand| and
|TypingCommand|.
Editing Refactor doc:
https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14BNMFTB8ms9uedeArbw/edit?usp=sharing
BUG=673789
Committed: https://crrev.com/f6fddd7fab5cf706023fc4f2edbdfa4fdd93d56f
Cr-Commit-Position: refs/heads/master@{#439010}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Xiaocheng's review: Add TODOs and DCHECK comments #
Total comments: 3
Depends on Patchset: Messages
Total messages: 40 (29 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 ========== Pass |CommandSource| from |Editor::Command| to |Editor::apply*/insert*/delete*/...| BUG=673789 ========== to ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. It's OK to add extra |CommandSource| parameter to those methods because as per the editing refactor doc they are going to be moved to "EditorCommand.cpp" thus should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
Description was changed from ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. It's OK to add extra |CommandSource| parameter to those methods because as per the editing refactor doc they are going to be moved to "EditorCommand.cpp" thus should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because as per the editing refactor doc they are going to be moved to "EditorCommand.cpp" thus should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
Description was changed from ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because as per the editing refactor doc they are going to be moved to "EditorCommand.cpp" thus should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because according to the editing refactor doc they are going to be moved to "EditorCommand.cpp" thus should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
Description was changed from ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because according to the editing refactor doc they are going to be moved to "EditorCommand.cpp" thus should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
Description was changed from ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
chongz@chromium.org changed reviewers: + xiaochengh@chromium.org
xiaochengh@ This is the second sub-patch of the |CommandSource| CL (depends on the first one). PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:379: CommandSource, Please add a TODO about future plan so that other people don't get surprised by this unused parameter. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:572: void Editor::replaceSelectionWithFragment(CommandSource, Please add a TODO about future plan so that other people don't get surprised by this unused parameter. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:710: void Editor::removeFormattingAndStyle(CommandSource) { Please add a TODO about future plan so that other people don't get surprised by this unused parameter. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:757: void Editor::applyParagraphStyle(CommandSource, Please add a TODO about future plan so that other people don't get surprised by this unused parameter. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1252: void Editor::undo(CommandSource) { How are we going to use CommandSource here? https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1260: void Editor::redo(CommandSource) { Same question as undo. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1414: void Editor::computeAndSetTypingStyle(CommandSource, Please add a TODO about future plan so that other people don't get surprised by this unused parameter. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.h:277: CommandSource = CommandSource::MenuOrKeyBinding); I'm not a fan of this... Since the troublemaker is WebLocalFrameImpl::replaceSelection, I think we should move its implementation to Editor. The current implementation exposes too many details of Editor to web/. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:824: DCHECK_EQ(source, CommandSource::MenuOrKeyBinding); Please add some explanation to this DCHECK. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:886: DCHECK_EQ(source, CommandSource::MenuOrKeyBinding); Please add some explanation to this DCHECK. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:925: DCHECK_EQ(source, CommandSource::MenuOrKeyBinding); Please add some explanation to this DCHECK.
https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.h:277: CommandSource = CommandSource::MenuOrKeyBinding); On 2016/12/15 at 03:27:50, Xiaocheng wrote: > I'm not a fan of this... > > Since the troublemaker is WebLocalFrameImpl::replaceSelection, I think we should move its implementation to Editor. The current implementation exposes too many details of Editor to web/. Implementation of WebLocalFrameImpl::replaceSelection is moved to Editor by https://codereview.chromium.org/2575343002/. Please rebase accordingly.
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #2 (id:20001) 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 ========== [CommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |CommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |CommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
Xiaocheng@ I've rebased and updated as per your comments, PTAL again, thanks! https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:379: CommandSource, On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add a TODO about future plan so that other people don't get surprised by > this unused parameter. Done. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:572: void Editor::replaceSelectionWithFragment(CommandSource, On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add a TODO about future plan so that other people don't get surprised by > this unused parameter. Done. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:710: void Editor::removeFormattingAndStyle(CommandSource) { On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add a TODO about future plan so that other people don't get surprised by > this unused parameter. Done. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:757: void Editor::applyParagraphStyle(CommandSource, On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add a TODO about future plan so that other people don't get surprised by > this unused parameter. Done. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1252: void Editor::undo(CommandSource) { On 2016/12/15 03:27:50, Xiaocheng wrote: > How are we going to use CommandSource here? We are going to fire a 'beforeinput' for user triggered undo & redo, so JS can cancel the action. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1260: void Editor::redo(CommandSource) { On 2016/12/15 03:27:50, Xiaocheng wrote: > Same question as undo. Same as above. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1414: void Editor::computeAndSetTypingStyle(CommandSource, On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add a TODO about future plan so that other people don't get surprised by > this unused parameter. Done. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.h:277: CommandSource = CommandSource::MenuOrKeyBinding); On 2016/12/15 05:38:21, Xiaocheng wrote: > On 2016/12/15 at 03:27:50, Xiaocheng wrote: > > I'm not a fan of this... > > > > Since the troublemaker is WebLocalFrameImpl::replaceSelection, I think we > should move its implementation to Editor. The current implementation exposes too > many details of Editor to web/. > > Implementation of WebLocalFrameImpl::replaceSelection is moved to Editor by > https://codereview.chromium.org/2575343002/. Please rebase accordingly. Rebased and removed default parameter. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:824: DCHECK_EQ(source, CommandSource::MenuOrKeyBinding); On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add some explanation to this DCHECK. Done. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:886: DCHECK_EQ(source, CommandSource::MenuOrKeyBinding); On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add some explanation to this DCHECK. Done. https://codereview.chromium.org/2576903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:925: DCHECK_EQ(source, CommandSource::MenuOrKeyBinding); On 2016/12/15 03:27:50, Xiaocheng wrote: > Please add some explanation to this DCHECK. Done. https://codereview.chromium.org/2576903002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2576903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:281: void replaceSelectionForSpellChecker(const String&); Added |replaceSelectionForSpellChecker()| similar to |replaceSelection()| above.
xiaochengh@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent https://codereview.chromium.org/2576903002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2576903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:281: void replaceSelectionForSpellChecker(const String&); On 2016/12/16 at 00:45:22, chongz wrote: > Added |replaceSelectionForSpellChecker()| similar to |replaceSelection()| above. I like this change. Could you update patch description accordingly?
Description was changed from ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL passed command source down to various editing methods inside Editor. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added |Editor::replaceSelectionForSpellChecker()| to be used in |SpellChecker::replaceMisspelledRange()| 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
Description was changed from ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added |Editor::replaceSelectionForSpellChecker()| to be used in |SpellChecker::replaceMisspelledRange()| 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added |Editor::replaceSelectionForSpellChecker()| to be used in |SpellChecker::replaceMisspelledRange()| 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added |Editor::replaceSelectionForSpellChecker()| to be used in |SpellChecker::replaceMisspelledRange()| 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added method |Editor::replaceSelectionForSpellChecker()| for |SpellChecker::replaceMisspelledRange()| to hide editor details; 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
Description was changed from ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added method |Editor::replaceSelectionForSpellChecker()| for |SpellChecker::replaceMisspelledRange()| to hide editor details; 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added method |Editor::replaceSelectionForSpellChecker()| for |SpellChecker::replaceMisspelledRange()| to hide editor details; 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText()| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ==========
lgtm
https://codereview.chromium.org/2576903002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/2576903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:281: void replaceSelectionForSpellChecker(const String&); On 2016/12/16 01:30:43, Xiaocheng wrote: > On 2016/12/16 at 00:45:22, chongz wrote: > > Added |replaceSelectionForSpellChecker()| similar to |replaceSelection()| > above. > > I like this change. Could you update patch description accordingly? Done.
The CQ bit was checked by chongz@chromium.org
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": 1481858333753640,
"parent_rev": "d397227468774f3a4362f4f1fb97225fc28e302a", "commit_rev":
"b0f6da5572f6fd5a57a90e18f9ab6fdf5a586de7"}
Message was sent while issue was closed.
Description was changed from ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added method |Editor::replaceSelectionForSpellChecker()| for |SpellChecker::replaceMisspelledRange()| to hide editor details; 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText()| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 ========== to ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added method |Editor::replaceSelectionForSpellChecker()| for |SpellChecker::replaceMisspelledRange()| to hide editor details; 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText()| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 Review-Url: https://codereview.chromium.org/2576903002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added method |Editor::replaceSelectionForSpellChecker()| for |SpellChecker::replaceMisspelledRange()| to hide editor details; 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText()| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 Review-Url: https://codereview.chromium.org/2576903002 ========== to ========== [EditCommandSource] Pass source from |Editor::Command| to |Editor::apply*()/insert*()/...| (2/3) This is the sub-patch (2/3) of supporting |EditCommandSource| in |CompositeEditCommand| (https://crrev.com/2574793002). This CL: 1. Added method |Editor::replaceSelectionForSpellChecker()| for |SpellChecker::replaceMisspelledRange()| to hide editor details; 2. Passed command source from |Editor::Command| to various methods in |Editor|, such as |performDelete()|, |removeFormattingAndStyle()|, |applyStyle()|, |replaceSelectionWithText()| etc. Note: It's OK to add extra |EditCommandSource| parameter to those methods because they are going to be moved to "EditorCommand.cpp" (according to the editing refactor doc), which means they should share the similar pattern as other |static execute*()| methods. This CL shouldn't have any behavior change. Next CL will pass command source down to |CompositeEditCommand| and |TypingCommand|. Editing Refactor doc: https://docs.google.com/a/chromium.org/document/d/1i-f5ByGHIvTcvy2UEAvOtBK14B... BUG=673789 Committed: https://crrev.com/f6fddd7fab5cf706023fc4f2edbdfa4fdd93d56f Cr-Commit-Position: refs/heads/master@{#439010} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f6fddd7fab5cf706023fc4f2edbdfa4fdd93d56f Cr-Commit-Position: refs/heads/master@{#439010}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/2635963004/ 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.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
