Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530843003:
Introduce InsertIncrementalTextCommand to respect existing style for composition (Closed)
DescriptionIntroduce InsertIncrementalTextCommand to respect existing style for composition
In order to keep the previous text formatting when continuing typing on the word,
confirming the ongoing composition, or inserting new text, this CL introduces
InsertIncrementalTextCommand for insertion. The command detects what was
there already, and restricts the write to the character(s) that actually changed.
Note that this CL doesn't change the behavior of events, i.e., it still sends
'beforeinput', 'input', and 'textInput' events with the entire text.
BUG=620549
Committed: https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044
Cr-Commit-Position: refs/heads/master@{#438490}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Add a test for commitText() #
Total comments: 7
Patch Set 3 : Experiment: for textInput event #Patch Set 4 : Introduce InsertIncrementalTextCommand #
Total comments: 11
Patch Set 5 : Address chongz@'s review #
Total comments: 5
Patch Set 6 : A little change #
Total comments: 40
Patch Set 7 : Address yosin@'s and xiaochengh@'s review #Patch Set 8 : A little change #
Total comments: 15
Patch Set 9 : Address xiaochengh@'s review #Depends on Patchset: Messages
Total messages: 95 (62 generated)
The CQ bit was checked by yabinh@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...
PTAL~
yabinh@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
https://codereview.chromium.org/2530843003/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2530843003/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1277: + "compositionupdate(a),input,compositionend(a),selectionchange,selectionchange," Selection change events are more than expected already. I think it's not too bad to add one more. https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html:26: Replacing textInput event with beforeinput and input events because: " The textInput event, originally proposed as a replacement for keypress, was removed in favor of the current beforeinput and input events. " (https://www.w3.org/TR/uievents/#event-flow) https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1003: It's better to have a test for inserting empty text. (BTW, this test can pass without applying this patch). https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:252: The followings functions (till Line 335) are moved from InputMethodController.cpp. https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:335: The above functions are moved from InputMethodController.cpp. https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebViewTest.cpp:1282: frame->frame()->inputMethodController().setCompositionFromExistingText( Note that the parameter is the composition range. So [3,3] doesn't make any sense.
Description was changed from ========== Make setComposition and finishComposingText respect existing style(2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text instead of the incremental text. BUG=620549 ========== to ========== Make setComposition and finishComposingText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text instead of the incremental text. BUG=620549 ==========
https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (left): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:687: Node* extentNode = extent.anchorNode(); baseNode and extentNode could be different anchor nodes. e.g, InputMethodControllerTest#SetCompositionKeepingStyle https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:692: unsigned baseOffset = base.computeOffsetInContainerNode(); Note that the offset is relative to its anchor node.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make setComposition and finishComposingText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text instead of the incremental text. BUG=620549 ========== to ========== Make setComposition and finishComposingText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (v.s the incremental text). BUG=620549 ==========
Description was changed from ========== Make setComposition and finishComposingText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (v.s the incremental text). BUG=620549 ========== to ========== Make setComposition and finishComposingText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
The CQ bit was checked by yabinh@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 ========== Make setComposition and finishComposingText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
Description was changed from ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
changwan@chromium.org changed reviewers: + yosin@chromium.org
This is mostly Blink change, adding yosin@ for Blink side changes. ImeTest.java lgtm
Description was changed from ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word or confirming the ongoing composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or replacing the composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
Description was changed from ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or replacing the composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or replacing the composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
Description was changed from ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or replacing the composition, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
Description was changed from ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
Description was changed from ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chongz@chromium.org changed reviewers: + chongz@chromium.org
Non-owner comments. https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html:26: On 2016/11/25 04:36:27, yabinh wrote: > Replacing textInput event with beforeinput and input events because: > " > The textInput event, originally proposed as a replacement for keypress, was > removed in favor of the current beforeinput and input events. > " > (https://www.w3.org/TR/uievents/#event-flow) I think we cannot simply remove 'textInput' event as it will break the web, please see comments in |InputMethodController|. https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:252: On 2016/11/25 04:36:27, yabinh wrote: > The followings functions (till Line 335) are moved from > InputMethodController.cpp. I recalled that yosin@ doesn't want extra logic in |TypingCommand| (https://crbug.com/620549#c28), do you think it would be better to move them into a new CompositeEditCommand (as suggested in https://crbug.com/620549#c30)? https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:129: TypingCommand::insertIncrementalText(*frame.document(), oldText, newText, Is it possible to utilize |FrameSelection::selectedText()| in |TypingCommand::insertIncrementalText()|, such that we don't need to pass in |oldText|? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/F... https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: break; We cannot simply remove 'textInput' event due to it's high usage (0.063%), IIRC some apps such as Google Docs on Windows is still relying on it. To remove it we will have to follow the "Launch Process: Deprecation" (https://www.chromium.org/blink#TOC-Launch-Process:-Deprecation) as it's a web exposed change. For now I would fire a special 'textInput' event for confirm composition and do the actual work here (similar to Drop), and plan for the deprecation&removal after web had adopted to InputEvent (2~3 years maybe). See Chromestatus: https://www.chromestatus.com/metrics/feature/timeline/popularity/831
On 2016/11/28 16:01:04, chongz wrote: > Non-owner comments. > > https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html > (right): > > https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html:26: > On 2016/11/25 04:36:27, yabinh wrote: > > Replacing textInput event with beforeinput and input events because: > > " > > The textInput event, originally proposed as a replacement for keypress, was > > removed in favor of the current beforeinput and input events. > > " > > (https://www.w3.org/TR/uievents/#event-flow) > > I think we cannot simply remove 'textInput' event as it will break the web, > please see comments in |InputMethodController|. > > https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): > > https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:252: > On 2016/11/25 04:36:27, yabinh wrote: > > The followings functions (till Line 335) are moved from > > InputMethodController.cpp. > > I recalled that yosin@ doesn't want extra logic in |TypingCommand| > (https://crbug.com/620549#c28), do you think it would be better to move them > into a new CompositeEditCommand (as suggested in https://crbug.com/620549#c30)? > > https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:129: > TypingCommand::insertIncrementalText(*frame.document(), oldText, newText, > Is it possible to utilize |FrameSelection::selectedText()| in > |TypingCommand::insertIncrementalText()|, such that we don't need to pass in > |oldText|? > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/F... > > https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: break; > We cannot simply remove 'textInput' event due to it's high usage (0.063%), IIRC > some apps such as Google Docs on Windows is still relying on it. > > To remove it we will have to follow the "Launch Process: Deprecation" > (https://www.chromium.org/blink#TOC-Launch-Process:-Deprecation) as it's a web > exposed change. > > For now I would fire a special 'textInput' event for confirm composition and do > the actual work here (similar to Drop), and plan for the deprecation&removal > after web had adopted to InputEvent (2~3 years maybe). > > See Chromestatus: > https://www.chromestatus.com/metrics/feature/timeline/popularity/831 not lgtm as we cannot remove 'textInput' event.
https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: break; On 2016/11/28 at 16:01:04, chongz wrote: > We cannot simply remove 'textInput' event due to it's high usage (0.063%), IIRC some apps such as Google Docs on Windows is still relying on it. > > To remove it we will have to follow the "Launch Process: Deprecation" (https://www.chromium.org/blink#TOC-Launch-Process:-Deprecation) as it's a web exposed change. > > For now I would fire a special 'textInput' event for confirm composition and do the actual work here (similar to Drop), and plan for the deprecation&removal after web had adopted to InputEvent (2~3 years maybe). > > See Chromestatus: > https://www.chromestatus.com/metrics/feature/timeline/popularity/831 Could you split use counter for 'textInput' into non-IME and IME? I hope IME-textInput is less used.
https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:252: On 2016/11/28 16:01:04, chongz wrote: > On 2016/11/25 04:36:27, yabinh wrote: > > The followings functions (till Line 335) are moved from > > InputMethodController.cpp. > > I recalled that yosin@ doesn't want extra logic in |TypingCommand| > (https://crbug.com/620549#c28), do you think it would be better to move them > into a new CompositeEditCommand (as suggested in https://crbug.com/620549#c30)? I think what yosin@ doesn't want is an extra option flag. And a separate function is acceptable. yosin@, can you clarify that? https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:129: TypingCommand::insertIncrementalText(*frame.document(), oldText, newText, On 2016/11/28 16:01:04, chongz wrote: > Is it possible to utilize |FrameSelection::selectedText()| in > |TypingCommand::insertIncrementalText()|, such that we don't need to pass in > |oldText|? > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/F... Here, oldText refers to the previous composing text, which could be different from the selected text.
https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:252: On 2016/11/29 at 02:15:21, yabinh wrote: > On 2016/11/28 16:01:04, chongz wrote: > > On 2016/11/25 04:36:27, yabinh wrote: > > > The followings functions (till Line 335) are moved from > > > InputMethodController.cpp. > > > > I recalled that yosin@ doesn't want extra logic in |TypingCommand| > > (https://crbug.com/620549#c28), do you think it would be better to move them > > into a new CompositeEditCommand (as suggested in https://crbug.com/620549#c30)? > > I think what yosin@ doesn't want is an extra option flag. And a separate function is acceptable. > yosin@, can you clarify that? chognze@ is right. Our conclusion is introducing TypingForCompositionCommand rather than make TypingCommand more complex.
chongz@chromium.org changed reviewers: + foolip@chromium.org
foolip@ can we have some help on the status of |TextEvent| please? Thanks! https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: break; On 2016/11/29 02:10:16, Yosi_UTC9 wrote: > On 2016/11/28 at 16:01:04, chongz wrote: > > We cannot simply remove 'textInput' event due to it's high usage (0.063%), > IIRC some apps such as Google Docs on Windows is still relying on it. > > > > To remove it we will have to follow the "Launch Process: Deprecation" > (https://www.chromium.org/blink#TOC-Launch-Process:-Deprecation) as it's a web > exposed change. > > > > For now I would fire a special 'textInput' event for confirm composition and > do the actual work here (similar to Drop), and plan for the deprecation&removal > after web had adopted to InputEvent (2~3 years maybe). > > > > See Chromestatus: > > https://www.chromestatus.com/metrics/feature/timeline/popularity/831 > > Could you split use counter for 'textInput' into non-IME and IME? I hope > IME-textInput is less used. Adding foolip@ for more input as he is the owner of |TextEvent|. I'm not sure if we can split the use counter, but I'd assume the ratio to be similar as JS has no way to distinguish IME/non-IME 'textInput' (e.g. no |isComposing| field). I checked Google Docs on Windows again and it seems to work as usual (they've probably changed IME model), but I cannot figure out how 'textInput' was used in the wild. e.g. See HTTP Archive query for 'textInput': https://docs.google.com/a/chromium.org/spreadsheets/d/1zFbRFKtQ-RKF3z5mLVfzs7...
The CQ bit was checked by yabinh@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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: break; On 2016/11/29 22:20:58, chongz wrote: > On 2016/11/29 02:10:16, Yosi_UTC9 wrote: > > On 2016/11/28 at 16:01:04, chongz wrote: > > > We cannot simply remove 'textInput' event due to it's high usage (0.063%), > > IIRC some apps such as Google Docs on Windows is still relying on it. > > > > > > To remove it we will have to follow the "Launch Process: Deprecation" > > (https://www.chromium.org/blink#TOC-Launch-Process:-Deprecation) as it's a web > > exposed change. > > > > > > For now I would fire a special 'textInput' event for confirm composition and > > do the actual work here (similar to Drop), and plan for the > deprecation&removal > > after web had adopted to InputEvent (2~3 years maybe). > > > > > > See Chromestatus: > > > https://www.chromestatus.com/metrics/feature/timeline/popularity/831 > > > > Could you split use counter for 'textInput' into non-IME and IME? I hope > > IME-textInput is less used. > > Adding foolip@ for more input as he is the owner of |TextEvent|. > > I'm not sure if we can split the use counter, but I'd assume the ratio to be > similar as JS has no way to distinguish IME/non-IME 'textInput' (e.g. no > |isComposing| field). > > I checked Google Docs on Windows again and it seems to work as usual (they've > probably changed IME model), but I cannot figure out how 'textInput' was used in > the wild. > > e.g. See HTTP Archive query for 'textInput': > https://docs.google.com/a/chromium.org/spreadsheets/d/1zFbRFKtQ-RKF3z5mLVfzs7... I added a TODO in TextEvent.idl and a use counter, but I think it has to be the input team who owns the fate of TextEvent. If there is any way to get rid of it, that'd be great. I can't quite see how TextEvent is affected in this CL, can someone spell it out for me? In any case, risky changes are best left done separately from the non-risky ones, and outright removals of features do need deprecation and removal. (Trivial things can bypass all process, of course.)
> I can't quite see how TextEvent is affected in this CL, can someone spell it out > for me? Patch 1 and 2 don't fire textInput event. So there was a discussion about whether we should keep that event. But Patch 3 (experimental path) fires as it did before. BTW, I'll upload a new patch later.
The CQ bit was checked by yabinh@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 ========== Make setComposition, finishComposingText and commitText respect existing style (2/2) In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL keeps sending 'beforeinput' and 'input' events with the whole text (vs the incremental text). BUG=620549 ========== to ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behaviour of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the whole text. BUG=620549 ==========
Description was changed from ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behaviour of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the whole text. BUG=620549 ========== to ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behaviour of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the whole text. BUG=620549 ==========
Description was changed from ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behaviour of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the whole text. BUG=620549 ========== to ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ==========
PTAL~ Note that this CL depends on another CL: https://codereview.chromium.org/2143923002/ https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001.html:26: On 2016/11/28 16:01:04, chongz wrote: > On 2016/11/25 04:36:27, yabinh wrote: > > Replacing textInput event with beforeinput and input events because: > > " > > The textInput event, originally proposed as a replacement for keypress, was > > removed in favor of the current beforeinput and input events. > > " > > (https://www.w3.org/TR/uievents/#event-flow) > > I think we cannot simply remove 'textInput' event as it will break the web, > please see comments in |InputMethodController|. 'textInput' event is kept in patch set 3 and 4. https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:252: On 2016/11/29 03:31:33, Yosi_UTC9 wrote: > On 2016/11/29 at 02:15:21, yabinh wrote: > > On 2016/11/28 16:01:04, chongz wrote: > > > On 2016/11/25 04:36:27, yabinh wrote: > > > > The followings functions (till Line 335) are moved from > > > > InputMethodController.cpp. > > > > > > I recalled that yosin@ doesn't want extra logic in |TypingCommand| > > > (https://crbug.com/620549#c28), do you think it would be better to move them > > > into a new CompositeEditCommand (as suggested in > https://crbug.com/620549#c30)? > > > > I think what yosin@ doesn't want is an extra option flag. And a separate > function is acceptable. > > yosin@, can you clarify that? > > chognze@ is right. Our conclusion is introducing TypingForCompositionCommand > rather than make TypingCommand more complex. Done. Introduced a new command in patch set 4. https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:129: TypingCommand::insertIncrementalText(*frame.document(), oldText, newText, On 2016/11/29 02:15:21, yabinh wrote: > On 2016/11/28 16:01:04, chongz wrote: > > Is it possible to utilize |FrameSelection::selectedText()| in > > |TypingCommand::insertIncrementalText()|, such that we don't need to pass in > > |oldText|? > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/F... > > Here, oldText refers to the previous composing text, which could be different > from the selected text. It turns out to be possible after a workaround. See patch set 4.
Description was changed from ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ========== to ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ==========
Description was changed from ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition or inserting new text, this CL introduce InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ========== to ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition, or inserting new text, this CL introduces InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the update and sorry for the delay. Here are my comments: https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:71: bool static needsIncrementalInsertion(const LocalFrame& frame, I was under the impression that the style guide prefers anonymous namespace than static functions, yosin@ can you confirm please? e.g. Remove |static| since it's already inside anonymous namespace. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:77: if (frame.document()->focusedElement()->tagName() == String("INPUT")) Maybe |!frame.editor().canEditRichly()|? e.g. We don't need it for "TEXTAREA" as well. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:150: case TypingCommand::TextCompositionType::TextCompositionConfirm: Sorry for the confusion from https://crbug.com/620549#c30 to https://crbug.com/620549#c34, I wasn't suggesting calling |TypingCommand| for |TextCompositionConfirm|. My suggestions are: 1. For |TextCompositionUpdate|: Keep using |TypingCommand| (or |TypingForCompositionCommand| as suggested by yosion@), which should then call |InsertIncrementalTextCommand| (Already done) 2. For |TextCompositionConfirm/Cancel|: Don't switch to |TypingCommand| until |TextEvent| has been fully removed (Should at least be in another patch) For now I'd suggest something like: ``` case |TextCompositionConfirm/Cancel|: const bool textEventCanceled = frame.eventHandler().handleTextInputEvent(..., TextEventInputComposition) // Check if |frame| was destroyed maybe If (!textEventCanceled) InsertIncrementalTextCommand::create(...)->apply() ``` Also in |Editor::handleTextEvent()| we should return false if |event->isComposition()| Does that make sense? Also see previous comment: https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:302: // Delete the current selection. I'm not sure if I fully understand the difficulties and workaround, but why can't we just use |InsertTextCommand| here to replace the calculated selection? e.g. Similar to the usage in |TypingCommand::insertTextRunWithoutNewlines()|, where |InsertIncrementalTextCommand| acts like a wrapper of |InsertTextCommand| and adjusted selection and text to insert. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:191: newText = event->data(); We don't need to retrieve |data| again as |TextEvent| doesn't have the API to modify it. (Different from the |BeforeTextInsertedEvent| above).
The CQ bit was checked by yabinh@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...
PTAL https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:71: bool static needsIncrementalInsertion(const LocalFrame& frame, On 2016/12/02 20:34:44, chongz wrote: > I was under the impression that the style guide prefers anonymous namespace than > static functions, yosin@ can you confirm please? > e.g. Remove |static| since it's already inside anonymous namespace. Done. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:77: if (frame.document()->focusedElement()->tagName() == String("INPUT")) On 2016/12/02 20:34:44, chongz wrote: > Maybe |!frame.editor().canEditRichly()|? e.g. We don't need it for "TEXTAREA" as > well. Done. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:150: case TypingCommand::TextCompositionType::TextCompositionConfirm: On 2016/12/02 20:34:44, chongz wrote: > Sorry for the confusion from https://crbug.com/620549#c30 to > https://crbug.com/620549#c34, I wasn't suggesting calling |TypingCommand| for > |TextCompositionConfirm|. > > My suggestions are: > 1. For |TextCompositionUpdate|: Keep using |TypingCommand| (or > |TypingForCompositionCommand| as suggested by yosion@), which should then call > |InsertIncrementalTextCommand| (Already done) > 2. For |TextCompositionConfirm/Cancel|: Don't switch to |TypingCommand| until > |TextEvent| has been fully removed (Should at least be in another patch) > For now I'd suggest something like: > ``` > case |TextCompositionConfirm/Cancel|: > const bool textEventCanceled = > frame.eventHandler().handleTextInputEvent(..., TextEventInputComposition) > // Check if |frame| was destroyed maybe > If (!textEventCanceled) > InsertIncrementalTextCommand::create(...)->apply() > ``` > > Also in |Editor::handleTextEvent()| we should return false if > |event->isComposition()| > > Does that make sense? > > Also see previous comment: > https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... Sorry I still don't understand why we can't switch to TypingCommand for TextCompositionConfirm since TextInputEvent is kept in this patch. If we decide to remove TextInputEvent in the future, we can simply stop calling dispatchTextInputEvent(). I'm not sure if my understanding is correct, but frame.eventHandler().handleTextInputEvent() deletes the entire composing text and inserts new text. So we will lose the style before calling InsertIncrementalTextCommand::create(...)->apply(). Also, it will fire duplicate 'input' event if we call both. So we need stop calling frame.eventHandler().handleTextInputEvent() for TextCompositionConfirm case. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:302: // Delete the current selection. On 2016/12/02 20:34:44, chongz wrote: > I'm not sure if I fully understand the difficulties and workaround, but why > can't we just use |InsertTextCommand| here to replace the calculated selection? > e.g. Similar to the usage in |TypingCommand::insertTextRunWithoutNewlines()|, > where |InsertIncrementalTextCommand| acts like a wrapper of |InsertTextCommand| > and adjusted selection and text to insert. > > https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... Done. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:191: newText = event->data(); On 2016/12/02 20:34:44, chongz wrote: > We don't need to retrieve |data| again as |TextEvent| doesn't have the API to > modify it. (Different from the |BeforeTextInsertedEvent| above). Done.
Description was changed from ========== Introduce InsertIncrementalTextCommand for composition to respect existing style In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition, or inserting new text, this CL introduces InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ========== to ========== Introduce InsertIncrementalTextCommand to respect existing style for composition In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition, or inserting new text, this CL introduces InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ==========
Thanks for the update! |TextEvent| lgtm with nits, will defer other reviews to yosin@. https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:150: case TypingCommand::TextCompositionType::TextCompositionConfirm: On 2016/12/05 08:09:39, yabinh wrote: > On 2016/12/02 20:34:44, chongz wrote: > > Sorry for the confusion from https://crbug.com/620549#c30 to > > https://crbug.com/620549#c34, I wasn't suggesting calling |TypingCommand| for > > |TextCompositionConfirm|. > > > > My suggestions are: > > 1. For |TextCompositionUpdate|: Keep using |TypingCommand| (or > > |TypingForCompositionCommand| as suggested by yosion@), which should then call > > |InsertIncrementalTextCommand| (Already done) > > 2. For |TextCompositionConfirm/Cancel|: Don't switch to |TypingCommand| until > > |TextEvent| has been fully removed (Should at least be in another patch) > > For now I'd suggest something like: > > ``` > > case |TextCompositionConfirm/Cancel|: > > const bool textEventCanceled = > > frame.eventHandler().handleTextInputEvent(..., TextEventInputComposition) > > // Check if |frame| was destroyed maybe > > If (!textEventCanceled) > > InsertIncrementalTextCommand::create(...)->apply() > > ``` > > > > Also in |Editor::handleTextEvent()| we should return false if > > |event->isComposition()| > > > > Does that make sense? > > > > Also see previous comment: > > > https://codereview.chromium.org/2530843003/diff/20001/third_party/WebKit/Sour... > > Sorry I still don't understand why we can't switch to TypingCommand for > TextCompositionConfirm since TextInputEvent is kept in this patch. If we decide > to remove TextInputEvent in the future, we can simply stop calling > dispatchTextInputEvent(). > > I'm not sure if my understanding is correct, but > frame.eventHandler().handleTextInputEvent() deletes the entire composing text > and inserts new text. So we will lose the style before calling > InsertIncrementalTextCommand::create(...)->apply(). > Also, it will fire duplicate 'input' event if we call both. > So we need stop calling frame.eventHandler().handleTextInputEvent() for > TextCompositionConfirm case. My concerns are: 1. Moving |TextEvent| into |TypingCommand| might make it even more complex; 2. The hierarchy doesn't look very straightforward to me as |TextEvent| also has the ability to trigger |TypingCommand|. But I won't block on this as long as yosin@ is happy and |TextEvent| keeps the original behavior. https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:188: TextEvent::create(frame->domWindow(), text, TextEventInputDrop); Can we use a different type for composition and do the similar filter in |Editor::handleTextEvent()|? https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:228: dispatchTextInputEvent(frame, newText); We need to keep |TextEvent| cancelable. e.g. Check the return value of |target->dispatchEvent()| and abort if canceled. https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right): https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.h:180: static bool m_isIncrementalInsertion; I'm not so comfortable using a static class variable here, maybe yosin@ has some advices.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yabinh@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...
Thanks! yosin@, PTAL. https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:188: TextEvent::create(frame->domWindow(), text, TextEventInputDrop); On 2016/12/05 09:01:46, chongz wrote: > Can we use a different type for composition and do the similar filter in > |Editor::handleTextEvent()|? Done. https://codereview.chromium.org/2530843003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:228: dispatchTextInputEvent(frame, newText); On 2016/12/05 09:01:46, chongz wrote: > We need to keep |TextEvent| cancelable. e.g. Check the return value of > |target->dispatchEvent()| and abort if canceled. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yosin@chromium.org changed reviewers: + xiaochengh@chromium.org
+xiaochengh@, since I'm slow for reviewing... https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:132: Element* element = frame->selection().selection().rootEditableElement(); Make root editable as parameter rather than passing |LocalFrame*|. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:145: VisibleSelection selection = nit: s/VisibleSelection/const VisibleSelection&/ https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:149: selection.setIsDirectional(isDirectional); Use SelectionInDOMTree::Builder::setIsDirectional() https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:154: const VisibleSelection nit: s/const// https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:164: const VisibleSelection selectionForInsertion = nit: s/VisibleSelection/const VisibleSelection&/ https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:171: void InsertIncrementalTextCommand::setSelection(const size_t start, EditingCommand should not set selection by it self. Use endingSelection() to work with undo/redo. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.h (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.h:19: return new InsertIncrementalTextCommand(document, text, selectInsertedText, Let's move to .cpp file: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.h:29: const size_t selectionStart, Let's use |int| since other functions use |int|.
lgtm https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:26: static size_t computeCommonPrefixLength(const String& str1, Let's use unnamed namespace: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... instead of |static| to follow chromium C++ style.
On 2016/12/07 at 05:36:22, Yosi_UTC9 wrote: > lgtm > > https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): > > https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:26: static size_t computeCommonPrefixLength(const String& str1, > Let's use unnamed namespace: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > instead of |static| to follow chromium C++ style. This is accidental click of button. It is NOT "lgtm".
Please eliminate all access to FrameSelection from InsertIncrementalTextCommand. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:69: PlainTextRange(0, commonPrefixLength).createRange(*rootEditableElement); Is this correct when |oldText| is not at the beginning of |rootEditableElement|? https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:99: PlainTextRange(0, oldText.length() - commonSuffixLength) Is this correct when |oldText| is not at the end of |rootEditableElement|? https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:117: static PlainTextRange getSelectionOffsets(LocalFrame* frame) { Let's get rid of this function. See comments on doApply() https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:128: static const VisibleSelection createSelection(const size_t start, Let's get rid of this function. See comments on doApply(). https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:155: InsertIncrementalTextCommand::computeSelectionForInsertion( Let's get rid of this function. See comments on doApply(). https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:176: setStartingSelection(selection); I don't think we should change the starting selection after the command finishes. Setting ending selection seems enough. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:188: const String oldText = frame->selectedText(); Accessing FrameSelection from EditCommand is discouraged. Use the following instead: const EphemeralRange selectionRange(endingSelection().start(), endingSelection().end()); oldText = plainText(selectionRange); https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:201: const PlainTextRange selectionOffsets = getSelectionOffsets(frame); Use CharacterIterator::calculateCharacterSubrange to prevent frequent translation between offsets and Positions, and also avoid accessing FrameSelection: CharacterIterator charIt(selectionRange); const EphemeralRange& rangeForInsertion = charIt.calculateCharacterSubrange(commonPrefixLength, m_text.length()); const VisibleSelection& selectionForInsertion = createVisibleSelection(SelectionInDOMTree::Builder().setBaseAndExtent(rangeForInsertion).setIsDirectional(endingSelection().isDirectional()).build()); https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:209: setStartingSelection(selectionForInsertion); I don't think we should change starting selection. Is there any client of the changed starting selection? https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:218: setSelection(selectionStart, selectionStart + newTextLength, frame); Use |setEndingSelection| instead. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.h:180: static bool m_isIncrementalInsertion; We shouldn't pass parameter by static member. Please make it non-static.
The CQ bit was checked by yabinh@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 yabinh@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...
PTAL https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:26: static size_t computeCommonPrefixLength(const String& str1, On 2016/12/07 05:36:22, Yosi_UTC9 wrote: > Let's use unnamed namespace: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > instead of |static| to follow chromium C++ style. Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:69: PlainTextRange(0, commonPrefixLength).createRange(*rootEditableElement); On 2016/12/07 10:23:26, Xiaocheng wrote: > Is this correct when |oldText| is not at the beginning of |rootEditableElement|? Yes. See InputMethodControllerTest#SetCompositionKeepingStyle https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:99: PlainTextRange(0, oldText.length() - commonSuffixLength) On 2016/12/07 10:23:26, Xiaocheng wrote: > Is this correct when |oldText| is not at the end of |rootEditableElement|? ditto https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:117: static PlainTextRange getSelectionOffsets(LocalFrame* frame) { On 2016/12/07 10:23:26, Xiaocheng wrote: > Let's get rid of this function. See comments on doApply() Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:128: static const VisibleSelection createSelection(const size_t start, On 2016/12/07 10:23:25, Xiaocheng wrote: > Let's get rid of this function. See comments on doApply(). Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:132: Element* element = frame->selection().selection().rootEditableElement(); On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > Make root editable as parameter rather than passing |LocalFrame*|. Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:145: VisibleSelection selection = On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > nit: s/VisibleSelection/const VisibleSelection&/ Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:149: selection.setIsDirectional(isDirectional); On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > Use SelectionInDOMTree::Builder::setIsDirectional() Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:154: const VisibleSelection On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > nit: s/const// This function has been removed. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:155: InsertIncrementalTextCommand::computeSelectionForInsertion( On 2016/12/07 10:23:26, Xiaocheng wrote: > Let's get rid of this function. See comments on doApply(). Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:164: const VisibleSelection selectionForInsertion = On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > nit: s/VisibleSelection/const VisibleSelection&/ This function has been removed. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:171: void InsertIncrementalTextCommand::setSelection(const size_t start, On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > EditingCommand should not set selection by it self. Use endingSelection() to > work with undo/redo. Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:176: setStartingSelection(selection); On 2016/12/07 10:23:25, Xiaocheng wrote: > I don't think we should change the starting selection after the command > finishes. Setting ending selection seems enough. Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:188: const String oldText = frame->selectedText(); On 2016/12/07 10:23:26, Xiaocheng wrote: > Accessing FrameSelection from EditCommand is discouraged. Use the following > instead: > > const EphemeralRange selectionRange(endingSelection().start(), > endingSelection().end()); > oldText = plainText(selectionRange); Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:201: const PlainTextRange selectionOffsets = getSelectionOffsets(frame); On 2016/12/07 10:23:26, Xiaocheng wrote: > Use CharacterIterator::calculateCharacterSubrange to prevent frequent > translation between offsets and Positions, and also avoid accessing > FrameSelection: > > CharacterIterator charIt(selectionRange); > const EphemeralRange& rangeForInsertion = > charIt.calculateCharacterSubrange(commonPrefixLength, m_text.length()); > const VisibleSelection& selectionForInsertion = > createVisibleSelection(SelectionInDOMTree::Builder().setBaseAndExtent(rangeForInsertion).setIsDirectional(endingSelection().isDirectional()).build()); Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:209: setStartingSelection(selectionForInsertion); On 2016/12/07 10:23:26, Xiaocheng wrote: > I don't think we should change starting selection. Is there any client of the > changed starting selection? Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:218: setSelection(selectionStart, selectionStart + newTextLength, frame); On 2016/12/07 10:23:26, Xiaocheng wrote: > Use |setEndingSelection| instead. This function has been removed. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.h (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.h:19: return new InsertIncrementalTextCommand(document, text, selectInsertedText, On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > Let's move to .cpp file: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts Done. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.h:29: const size_t selectionStart, On 2016/12/07 05:23:34, Yosi_UTC9 wrote: > Let's use |int| since other functions use |int|. This function has been removed. https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right): https://codereview.chromium.org/2530843003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.h:180: static bool m_isIncrementalInsertion; On 2016/12/07 10:23:26, Xiaocheng wrote: > We shouldn't pass parameter by static member. Please make it non-static. Done. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:304: Note that the DOM will change after insertion, so we can't simply apply the method used in InsertIncrementalTextCommand.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm almost OK with changes in editing/. Just a few minor comments. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:534: No need to have this extra blank line. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:15: #include "core/frame/LocalFrame.h" We don't need it anymore. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:146: LocalFrame* frame = document().frame(); We don't need |frame| anymore. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:299: document.updateStyleAndLayoutIgnorePendingStylesheets(); Please move it to just before calling |getSelectionOffsets| so that we can reduce the chance of updating layout. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:333: document.updateStyleAndLayoutIgnorePendingStylesheets(); Please move it into adjustSelectionAfterIncrementalInsertion. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:367: document.updateStyleAndLayoutIgnorePendingStylesheets(); Please move it into adjustSelectionAfterIncrementalInsertion. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/CharacterIterator.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/CharacterIterator.cpp:192: if (!length) Hmm... This function assumed that length > 0, but the semantic is still consistent with length = 0. Could you land this part in a separate patch?
The CQ bit was checked by yabinh@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...
PTAL https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:534: On 2016/12/09 05:10:30, Xiaocheng wrote: > No need to have this extra blank line. Done. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:15: #include "core/frame/LocalFrame.h" On 2016/12/09 05:10:30, Xiaocheng wrote: > We don't need it anymore. Done. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:146: LocalFrame* frame = document().frame(); On 2016/12/09 05:10:30, Xiaocheng wrote: > We don't need |frame| anymore. Done. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:299: document.updateStyleAndLayoutIgnorePendingStylesheets(); On 2016/12/09 05:10:30, Xiaocheng wrote: > Please move it to just before calling |getSelectionOffsets| so that we can > reduce the chance of updating layout. Done. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:333: document.updateStyleAndLayoutIgnorePendingStylesheets(); On 2016/12/09 05:10:30, Xiaocheng wrote: > Please move it into adjustSelectionAfterIncrementalInsertion. Done. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:367: document.updateStyleAndLayoutIgnorePendingStylesheets(); On 2016/12/09 05:10:30, Xiaocheng wrote: > Please move it into adjustSelectionAfterIncrementalInsertion. Done. https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/CharacterIterator.cpp (right): https://codereview.chromium.org/2530843003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/CharacterIterator.cpp:192: if (!length) On 2016/12/09 05:10:30, Xiaocheng wrote: > Hmm... This function assumed that length > 0, but the semantic is still > consistent with length = 0. > > Could you land this part in a separate patch? Here is old logic to get endPos: if length > 1, advance by |length - 1| step, then advance 1 more step in endPosition(); if length == 1, don't advance (actually it's the same with the above because it can be considered to advance 0 step), then advance 1 more step in endPosition(); if length == 0, it's exactly the same with the above case. We'll get a range with distance == 1. It's incorrect. I've uploaded a separate patch. See https://codereview.chromium.org/2564763002.
lgtm
On 2016/12/09 06:32:21, Xiaocheng wrote: > lgtm Thanks. foolip@, can you take a look at third_party/WebKit/Source/core/events/ ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aelias@chromium.org changed reviewers: + bokan@chromium.org - foolip@chromium.org
aelias@chromium.org changed reviewers: + foolip@chromium.org - bokan@chromium.org
Seems foolip@ is not available, adding bokan@ for review of tiny change in Source/core/events/
Sorry, after the earlier question I assumed I was no longer needed and starting marking this review as read. third_party/WebKit/Source/core/events/* LGTM
On 2016/12/14 09:15:20, foolip wrote: > Sorry, after the earlier question I assumed I was no longer needed and starting > marking this review as read. third_party/WebKit/Source/core/events/* LGTM Thanks!
The CQ bit was checked by yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from changwan@chromium.org, chongz@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2530843003/#ps160001 (title: "Address xiaochengh@'s review")
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": 160001, "attempt_start_ts": 1481712590080330,
"parent_rev": "4f1549b09d3e5beeed7f6a9c3fe59704ee475dcf", "commit_rev":
"a9b70394dccb697fc733cf4a6a75123e19f8cd06"}
Message was sent while issue was closed.
Description was changed from ========== Introduce InsertIncrementalTextCommand to respect existing style for composition In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition, or inserting new text, this CL introduces InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 ========== to ========== Introduce InsertIncrementalTextCommand to respect existing style for composition In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition, or inserting new text, this CL introduces InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 Review-Url: https://codereview.chromium.org/2530843003 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Introduce InsertIncrementalTextCommand to respect existing style for composition In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition, or inserting new text, this CL introduces InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 Review-Url: https://codereview.chromium.org/2530843003 ========== to ========== Introduce InsertIncrementalTextCommand to respect existing style for composition In order to keep the previous text formatting when continuing typing on the word, confirming the ongoing composition, or inserting new text, this CL introduces InsertIncrementalTextCommand for insertion. The command detects what was there already, and restricts the write to the character(s) that actually changed. Note that this CL doesn't change the behavior of events, i.e., it still sends 'beforeinput', 'input', and 'textInput' events with the entire text. BUG=620549 Committed: https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044 Cr-Commit-Position: refs/heads/master@{#438490} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044 Cr-Commit-Position: refs/heads/master@{#438490} |
