Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372493002:
Workaround for setComposition styling clobber (Closed)
DescriptionWorkaround for setComposition styling clobber
In order to keep the previous text formatting when continuing typing on
the word, this CL detects when the beginning of the IME commit is
the same at what was there already, and restricts the write to only
the character(s) at the end that actually changed. (This CL is based on
aelias@'s CL: https://codereview.chromium.org/2073863003)
BUG=620549
Committed: https://crrev.com/993074d17afee685ced99283d7ffbe809fdb6f99
Cr-Commit-Position: refs/heads/master@{#424716}
Patch Set 1 #Patch Set 2 : Fix a test #Patch Set 3 : Fix a layout test #
Total comments: 6
Patch Set 4 : test #
Total comments: 6
Patch Set 5 : Avoid duplicate events, and deal with multi-code text #Patch Set 6 : Change a test for multi-node text #
Total comments: 6
Patch Set 7 : Address changwan@'s review #
Total comments: 27
Patch Set 8 : Deal with both prefix and suffix. #
Total comments: 20
Patch Set 9 : Address yosin@'s review #Patch Set 10 #
Total comments: 18
Patch Set 11 : Address yosin@'s review #
Total comments: 30
Patch Set 12 : Address yosin@'s review #
Total comments: 3
Patch Set 13 #Patch Set 14 : refactor #Patch Set 15 : Remove a DCHECK. #
Total comments: 8
Patch Set 16 : Add some DCHECK. #
Messages
Total messages: 96 (64 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...
Description was changed from ========== Keep the previous text formatting when continuing typing on the word The style attribute of a word gets lost if you continue typing on that word. To fix that, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) BUG=644586 ========== to ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) ==========
Description was changed from ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) ========== to ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) ==========
Description was changed from ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) ========== to ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
yabinh@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001-expected.txt (right): https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001-expected.txt:9: PASS event.data is "1" Note that we call commitText() to update the composing text. https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:355: size_t commonPrefixLength = 0; aelias@ suggests to use std::mismatch(): https://codereview.chromium.org/2073863003#msg3 std::mismatch() takes char*. If we assign |composing| to a local char* variable, we'll get incorrect result, which is weird. const char* composingText = composing.utf8().data(); LOG(ERROR)<<composing.utf8().data(); // correct LOG(ERROR)<<composingText; // incorrect, garbled.
Description was changed from ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) ========== to ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) BUG=644591 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:355: size_t commonPrefixLength = 0; On 2016/09/26 10:10:32, yabinh wrote: > aelias@ suggests to use std::mismatch(): > https://codereview.chromium.org/2073863003#msg3 > std::mismatch() takes char*. If we assign |composing| to a local char* variable, > we'll get incorrect result, which is weird. > > const char* composingText = composing.utf8().data(); > LOG(ERROR)<<composing.utf8().data(); // correct > LOG(ERROR)<<composingText; // incorrect, garbled. std::mismatch accepts not only char* but any InputIterator, and it doesn't make much sense to pass char* iterators here because there are many different UTF8 / UTF16 characters that have common prefix bytes. Probably a wrapper around existing UTF16CharIterator may do which already handles surrogate pairs. But if wrapper implementation is complicated, I'd prefer that you implement a separate static UTF16CommonPrefix() function. (BTW, do we still need to consider UTF8 other than test? Probably not.) If you're concerned about incorrect utf8().data() logging, you may want to generate from smallest possible text and get the hex data from the logcat to see what's happened. I have no idea what could go wrong here.
Thanks for picking this up! https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001-expected.txt (right): https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001-expected.txt:9: PASS event.data is "1" On 2016/09/26 at 10:10:32, yabinh wrote: > Note that we call commitText() to update the composing text. This side effect is undesired, so please use insertTextDuringCompositionWithEvents(... TypingCommand::TextCompositionUpdate); instead.
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...
https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001-expected.txt (right): https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/ime-composition-events-001-expected.txt:9: PASS event.data is "1" On 2016/09/26 22:05:40, aelias wrote: > On 2016/09/26 at 10:10:32, yabinh wrote: > > Note that we call commitText() to update the composing text. > > This side effect is undesired, so please use > insertTextDuringCompositionWithEvents(... TypingCommand::TextCompositionUpdate); > instead. Done. https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:355: size_t commonPrefixLength = 0; On 2016/09/26 13:51:36, Changwan Ryu wrote: > On 2016/09/26 10:10:32, yabinh wrote: > > aelias@ suggests to use std::mismatch(): > > https://codereview.chromium.org/2073863003#msg3 > > std::mismatch() takes char*. If we assign |composing| to a local char* > variable, > > we'll get incorrect result, which is weird. > > > > const char* composingText = composing.utf8().data(); > > LOG(ERROR)<<composing.utf8().data(); // correct > > LOG(ERROR)<<composingText; // incorrect, garbled. > > std::mismatch accepts not only char* but any InputIterator, and it doesn't make > much sense to pass char* iterators here because there are many different UTF8 / > UTF16 characters that have common prefix bytes. > > Probably a wrapper around existing UTF16CharIterator may do which already > handles surrogate pairs. > But if wrapper implementation is complicated, I'd prefer that you implement a > separate static UTF16CommonPrefix() function. > (BTW, do we still need to consider UTF8 other than test? Probably not.) > > If you're concerned about incorrect utf8().data() logging, you may want to > generate from smallest possible text and get the hex data from the logcat to see > what's happened. I have no idea what could go wrong here. Done. Implemented a separate static function.
https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:388: extendSelectionAndDelete(composing.length() - commonPrefixLength, 0); Hmm, this has the same JS event problem as the other case you just fixed. How about selecting all the non-shared suffix characters in the selection logic above, and then just always calling insertTextDuringCompositionWithEvents() (possibly with "" for the backspacing case) and relying on the replacement of selection? https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:127: // (UTF16) 0xac000 0xb098 0xb2e4 0xb77c = "가나다라" Can you also add a simpler test case appending and removing single letters to English text?
https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:346: while (commonPrefixLength < length1 && commonPrefixLength < length2 && str1[commonPrefixLength] == str2[commonPrefixLength]) Can you skip a loop for surrogate pairs? Things can get weird if we apply different formats to surrogate head and tail.
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...
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...
https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:346: while (commonPrefixLength < length1 && commonPrefixLength < length2 && str1[commonPrefixLength] == str2[commonPrefixLength]) On 2016/09/27 05:23:03, Changwan Ryu wrote: > Can you skip a loop for surrogate pairs? Things can get weird if we apply > different formats to surrogate head and tail. Done. https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:388: extendSelectionAndDelete(composing.length() - commonPrefixLength, 0); On 2016/09/27 04:55:58, aelias wrote: > Hmm, this has the same JS event problem as the other case you just fixed. How > about selecting all the non-shared suffix characters in the selection logic > above, and then just always calling insertTextDuringCompositionWithEvents() > (possibly with "" for the backspacing case) and relying on the replacement of > selection? Done. https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2372493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:127: // (UTF16) 0xac000 0xb098 0xb2e4 0xb77c = "가나다라" On 2016/09/27 04:55:58, aelias wrote: > Can you also add a simpler test case appending and removing single letters to > English text? Done.
https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:341: size_t InputMethodController::computeUTF16CommonPrefixLength(const String& str1, const String& str2) const could you change the name to something like computeCommonGraphemeClusterPrefixLength? https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:358: Position position = range.endPosition(); The function has dependency on the current selection but still accepts str1 and str2 as parameters as if it's a generic utility function. Can you either find a way to find grapheme boundaries without calling Position or make this function not look like a generic utility function? https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:144: "<div id='sample' contenteditable='true'><i>hello</i>world<b>🏠</b></div>", "sample"); // U+1F3E0 = 0xF0 0x9F 0x8F 0xA0 (UTF8) could you add some explanation to the UTF8 code you used?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:341: size_t InputMethodController::computeUTF16CommonPrefixLength(const String& str1, const String& str2) const On 2016/09/27 09:17:11, Changwan Ryu wrote: > could you change the name to something like > computeCommonGraphemeClusterPrefixLength? Done. https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:358: Position position = range.endPosition(); On 2016/09/27 09:17:11, Changwan Ryu wrote: > The function has dependency on the current selection but still accepts str1 and > str2 as parameters as if it's a generic utility function. Can you either find a > way to find grapheme boundaries without calling Position or make this function > not look like a generic utility function? I haven't found that way, so I'll change the function name. There is a way for code point, but code point doesn't work for some tests. https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2372493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:144: "<div id='sample' contenteditable='true'><i>hello</i>world<b>🏠</b></div>", "sample"); // U+1F3E0 = 0xF0 0x9F 0x8F 0xA0 (UTF8) On 2016/09/27 09:17:11, Changwan Ryu wrote: > could you add some explanation to the UTF8 code you used? Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:168: controller().setComposition(String::fromUTF8("\xE0\xB0\x83"), underlines, 1, 1); Note that if we use code point to decide the common prefix length, this test will not pass. The reason is that, for now, we can only select (and delete) by grapheme cluster. we are not able to delete the second "\xE0\xB0\x83".
lgtm, you can add yosin@ for OWNERS once changwan@ is happy with the CL. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:138: EXPECT_STREQ("<b>he</b>llo", div->innerHTML().utf8().data()); Please add another test case for backspace.
Description was changed from ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) BUG=644591 ========== to ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) BUG=620549 ==========
changwan@chromium.org changed reviewers: + yosin@chromium.org
non-owner lgtm adding yosin@ as reviewer
Should we also check common suffix? WDYT? Before: Hel<b>loWo</b>rld After: abcde<b>loWo</b>rld Replace "Hel" by "abcde". https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:348: while (commonPrefixLength < oldLength && commonPrefixLength < newLength && oldText[commonPrefixLength] == newText[commonPrefixLength]) Could you move this part to another function, e.g. unsigned computeCommonPrefix(const String&, const String&); https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:349: commonPrefixLength++; nit: We prefer |++commonPrefixLength| https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:355: const EphemeralRange range = PlainTextRange(0, commonPrefixLength).createRange(*rootEditableElement); nit: s/const EphemeralRange/const EphemeralRange&/ https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:358: Position position = range.endPosition(); nit: s/Position/const Position&/ https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:359: Position adjustedPosition = previousPositionOf(nextPositionOf(position, PositionMoveType::GraphemeCluster), PositionMoveType::GraphemeCluster); nit: s/Position/const Position&/ https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:360: int diff = position.computeOffsetInContainerNode() - adjustedPosition.computeOffsetInContainerNode(); We should support position.containerNode() != adjustedPosition.containerNode(). https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:378: size_t composingLength = composingText().length(); Please move this part to another function. This part makes |setComposition()| too large. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:381: bool appending = text.length() > commonPrefixLength; nit: s/bool/const bool/ https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:382: bool backspacing = composingLength > commonPrefixLength; nit: s/bool/const bool/; since this variable used for improving readability. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:384: const EphemeralRange range = compositionEphemeralRange(); s/const EphemeralRange/const EphemeralRange&/ Better to compute after |editable| check. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:421: PlainTextRange selectedRange = createSelectionRangeForSetComposition(selectionStart, selectionEnd, text.length()); nit: s/PalinTextRange/const PlainTextRange&/ https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.h:119: size_t computeCommonGraphemeClusterPrefixLengthForSetComposition(const String& newText) const; Do we really need to have |computeCommonGraphemeClusterPrefixLengthForSetComposition()| as member function? It seems we can have this function as static non-member function and pass root editable element.
On 2016/10/04 at 09:00:05, yosin wrote: > Should we also check common suffix? WDYT? > > Before: Hel<b>loWo</b>rld > After: abcde<b>loWo</b>rld > > Replace "Hel" by "abcde". This is best-effort anyway, and I'd prefer we didn't scope creep this patch, which is targeted for M55. We can expand to covering prefix (and possibly infix) cases if and when a bug is filed requesting it.
On 2016/10/04 at 21:23:38, aelias wrote: > On 2016/10/04 at 09:00:05, yosin wrote: > > Should we also check common suffix? WDYT? > > > > Before: Hel<b>loWo</b>rld > > After: abcde<b>loWo</b>rld > > > > Replace "Hel" by "abcde". > > This is best-effort anyway, and I'd prefer we didn't scope creep this patch, which is targeted for M55. We can expand to covering prefix (and possibly infix) cases if and when a bug is filed requesting it. And more specifically this is really a best-effort workaround for a Google Keyboard Latin behavior, which I've just tested in https://jsfiddle.net/9n17does does not do the whole-word-commit thing when typing at the beginning or middle of a word, so it really wouldn't achieve much to add those additional guarantees in practice.
On 2016/10/04 21:31:45, aelias wrote: > On 2016/10/04 at 21:23:38, aelias wrote: > > On 2016/10/04 at 09:00:05, yosin wrote: > > > Should we also check common suffix? WDYT? > > > > > > Before: Hel<b>loWo</b>rld > > > After: abcde<b>loWo</b>rld > > > > > > Replace "Hel" by "abcde". > > > > This is best-effort anyway, and I'd prefer we didn't scope creep this patch, > which is targeted for M55. We can expand to covering prefix (and possibly > infix) cases if and when a bug is filed requesting it. > > And more specifically this is really a best-effort workaround for a Google > Keyboard Latin behavior, which I've just tested in https://jsfiddle.net/9n17does > does not do the whole-word-commit thing when typing at the beginning or middle > of a word, so it really wouldn't achieve much to add those additional guarantees > in practice. Hmmm... Actually it might be just Google Latin Keyboard's current behavior for prepending text that it finishes the current composing text and calls setComposingText() / setComposingRegion() separately (in a batch). If so, we might want to cover those cases for future changes and third party keyboards. But I haven't looked into it myself. Yabin, as we talked offline, could you investigate more?
On 2016/10/05 02:33:56, Changwan Ryu wrote: > On 2016/10/04 21:31:45, aelias wrote: > > On 2016/10/04 at 21:23:38, aelias wrote: > > > On 2016/10/04 at 09:00:05, yosin wrote: > > > > Should we also check common suffix? WDYT? > > > > > > > > Before: Hel<b>loWo</b>rld > > > > After: abcde<b>loWo</b>rld > > > > > > > > Replace "Hel" by "abcde". > > > > > > This is best-effort anyway, and I'd prefer we didn't scope creep this patch, > > which is targeted for M55. We can expand to covering prefix (and possibly > > infix) cases if and when a bug is filed requesting it. > > > > And more specifically this is really a best-effort workaround for a Google > > Keyboard Latin behavior, which I've just tested in > https://jsfiddle.net/9n17does > > does not do the whole-word-commit thing when typing at the beginning or middle > > of a word, so it really wouldn't achieve much to add those additional > guarantees > > in practice. > > Hmmm... Actually it might be just Google Latin Keyboard's current behavior for > prepending text that it finishes the current composing text > and calls setComposingText() / setComposingRegion() separately (in a batch). If > so, we might want to cover those cases for future changes and third party > keyboards. > But I haven't looked into it myself. Yabin, as we talked offline, could you > investigate more? I think we should also deal with the suffix. Here is a test case on this patch(also https://jsfiddle.net/9n17does): HELLO W<b>OR</b>LD! Then we put the cursor before "W" and delete the space, we'll lose bold: HELLOWORLD!
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...
https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:348: while (commonPrefixLength < oldLength && commonPrefixLength < newLength && oldText[commonPrefixLength] == newText[commonPrefixLength]) On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > Could you move this part to another function, e.g. unsigned > computeCommonPrefix(const String&, const String&); Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:349: commonPrefixLength++; On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > nit: We prefer |++commonPrefixLength| Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:355: const EphemeralRange range = PlainTextRange(0, commonPrefixLength).createRange(*rootEditableElement); On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > nit: s/const EphemeralRange/const EphemeralRange&/ Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:358: Position position = range.endPosition(); On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > nit: s/Position/const Position&/ Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:359: Position adjustedPosition = previousPositionOf(nextPositionOf(position, PositionMoveType::GraphemeCluster), PositionMoveType::GraphemeCluster); On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > nit: s/Position/const Position&/ Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:360: int diff = position.computeOffsetInContainerNode() - adjustedPosition.computeOffsetInContainerNode(); On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > We should support position.containerNode() != adjustedPosition.containerNode(). It seems that we couldn't find such a case. As we discussed offline, I'll just add assertion in the next patch. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:378: size_t composingLength = composingText().length(); On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > Please move this part to another function. This part makes |setComposition()| > too large. Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:381: bool appending = text.length() > commonPrefixLength; On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > nit: s/bool/const bool/ Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:382: bool backspacing = composingLength > commonPrefixLength; On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > nit: s/bool/const bool/; since this variable used for improving readability. Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:384: const EphemeralRange range = compositionEphemeralRange(); On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > s/const EphemeralRange/const EphemeralRange&/ > > Better to compute after |editable| check. Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:421: PlainTextRange selectedRange = createSelectionRangeForSetComposition(selectionStart, selectionEnd, text.length()); On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > nit: s/PalinTextRange/const PlainTextRange&/ Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.h:119: size_t computeCommonGraphemeClusterPrefixLengthForSetComposition(const String& newText) const; On 2016/10/04 09:00:05, Yosi_UTC9 wrote: > Do we really need to have > |computeCommonGraphemeClusterPrefixLengthForSetComposition()| as member > function? > It seems we can have this function as static non-member function and pass root > editable element. Done. https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2372493002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:138: EXPECT_STREQ("<b>he</b>llo", div->innerHTML().utf8().data()); On 2016/09/28 23:58:29, aelias wrote: > Please add another test case for backspace. Line#134 is for backspace(from "hello" to "hell").
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> I think we should also deal with the suffix. Here is a test case on this > patch(also https://jsfiddle.net/9n17does): > HELLO W<b>OR</b>LD! > Then we put the cursor before "W" and delete the space, we'll lose bold: > HELLOWORLD! BTW, this case is not related to setComposition, so the latest patch won't fix it. See the comment: https://bugs.chromium.org/p/chromium/issues/detail?id=620549#c5
https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:364: size_t commonPrefixLength = 0; for-loop is better const size_t maxCommonPrefixLength = std::min(length1, length2); for (size_t commonPrefixLength = 0; commonPrefixLength < maxCommonPrefixLength; ++commonPrefixLength) { if (str1[commonPrefixLength] != str2[commonPrefixLength]) break; } https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:379: while (commonSuffixLength < length1 && commonSuffixLength < length2 && for-loop is better const size_t maxCommonPrefixLength = std::min(length1, length2); for (size_t commonPrefixLength = 0; commonPrefixLength < maxCommonPrefixLength; ++commonPrefixLength) { if (str1[length1 - commonPrefixLength] != str2[length2 - commonPrefixLength]) break; } https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:393: // For multi-code text, we should adjust it for grapheme boundary. s/multi-code text/grapheme cluster/ https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:403: DCHECK(position.anchorNode()->isEqualNode(adjustedPosition.anchorNode())); Let's use DCHECK_EQ(position.anchroNode(), adjustedPosition.anchorNode()) to print nodes at assertion failure. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:405: int diff = position.computeOffsetInContainerNode() - Could you add DCHECK_GE(position.computeOffsetInContanerNode(), adjustedPositon.computeOffsetInContainerNode())? to make sure |diff >= 0| https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:427: DCHECK(position.anchorNode()->isEqualNode(adjustedPosition.anchorNode())); See comment in computeCommonGraphemeClusterPrefixLengthForSetComposition() https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:449: size_t commonSuffixLength = |const size_t|? https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:461: if (!editable) We've already check |editable| isn't nullptr at L441. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:478: Element* target = frame().document()->focusedElement(); FrameSeleciton::setSelection() can change document associate to |frame|. So, we should do: const Document& currentDocument = frame().document(); ... setSeleciton(); if (currentDocument != frame().document()) return; Element* target = frame().document()->focusedElement(); ... https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:491: if (!frame().document()) Similar to L478 comment.
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...
https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:364: size_t commonPrefixLength = 0; On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > for-loop is better > > const size_t maxCommonPrefixLength = std::min(length1, length2); > for (size_t commonPrefixLength = 0; commonPrefixLength < maxCommonPrefixLength; > ++commonPrefixLength) { > if (str1[commonPrefixLength] != str2[commonPrefixLength]) > break; > } Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:379: while (commonSuffixLength < length1 && commonSuffixLength < length2 && On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > for-loop is better > > const size_t maxCommonPrefixLength = std::min(length1, length2); > for (size_t commonPrefixLength = 0; commonPrefixLength < maxCommonPrefixLength; > ++commonPrefixLength) { > if (str1[length1 - commonPrefixLength] != str2[length2 - commonPrefixLength]) > break; > } Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:393: // For multi-code text, we should adjust it for grapheme boundary. On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > s/multi-code text/grapheme cluster/ Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:403: DCHECK(position.anchorNode()->isEqualNode(adjustedPosition.anchorNode())); On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > Let's use DCHECK_EQ(position.anchroNode(), adjustedPosition.anchorNode()) to > print nodes at assertion failure. Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:405: int diff = position.computeOffsetInContainerNode() - On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > Could you add DCHECK_GE(position.computeOffsetInContanerNode(), > adjustedPositon.computeOffsetInContainerNode())? > to make sure |diff >= 0| Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:427: DCHECK(position.anchorNode()->isEqualNode(adjustedPosition.anchorNode())); On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > See comment in computeCommonGraphemeClusterPrefixLengthForSetComposition() Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:449: size_t commonSuffixLength = On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > |const size_t|? Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:461: if (!editable) On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > We've already check |editable| isn't nullptr at L441. Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:478: Element* target = frame().document()->focusedElement(); On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > FrameSeleciton::setSelection() can change document associate to |frame|. > So, we should do: > > const Document& currentDocument = frame().document(); > ... > setSeleciton(); > if (currentDocument != frame().document()) > return; > Element* target = frame().document()->focusedElement(); > ... Done. https://codereview.chromium.org/2372493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:491: if (!frame().document()) On 2016/10/11 03:58:10, Yosi_UTC9 wrote: > Similar to L478 comment. Done.
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...
Let's add |const| as much as possible we can. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:365: for (commonPrefixLength = 0; commonPrefixLength < maxCommonPrefixLength; Let's make scope of |commonPrefixLength| narrow. for (size_t index = 0; index < maxCommonPrefixLength; ++index) { if (str1[index] != str2[index]) return index; } return maxCommonPrefixLength; https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:381: for (commonSuffixLength = 0; commonSuffixLength < maxCommonSuffixLength; Ditto as computeCommonPrefixLength https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:408: int diff = position.computeOffsetInContainerNode() - s/int/const int/ https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:433: int diff = adjustedPosition.computeOffsetInContainerNode() - s/int/const int/ https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:436: nit: no need to have an extra blank line. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:467: size_t compositionStart = nit: s/size_t/const size_t/ https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:469: size_t subtractionStart = compositionStart + commonPrefixLength; nit: s/size_t/const size_t/ https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:470: size_t subtractionEnd = nit: s/size_t/const size_t/ https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:674: int selectionOffsetsStart = static_cast<int>(getSelectionOffsets().start()); nit: s/int/const int/
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...
https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:365: for (commonPrefixLength = 0; commonPrefixLength < maxCommonPrefixLength; On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > Let's make scope of |commonPrefixLength| narrow. > > for (size_t index = 0; index < maxCommonPrefixLength; ++index) { > if (str1[index] != str2[index]) > return index; > } > return maxCommonPrefixLength; Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:381: for (commonSuffixLength = 0; commonSuffixLength < maxCommonSuffixLength; On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > Ditto as computeCommonPrefixLength Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:408: int diff = position.computeOffsetInContainerNode() - On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > s/int/const int/ Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:433: int diff = adjustedPosition.computeOffsetInContainerNode() - On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > s/int/const int/ Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:436: On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > nit: no need to have an extra blank line. Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:467: size_t compositionStart = On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > nit: s/size_t/const size_t/ Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:469: size_t subtractionStart = compositionStart + commonPrefixLength; On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > nit: s/size_t/const size_t/ Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:470: size_t subtractionEnd = On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > nit: s/size_t/const size_t/ Done. https://codereview.chromium.org/2372493002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:674: int selectionOffsetsStart = static_cast<int>(getSelectionOffsets().start()); On 2016/10/11 08:32:41, Yosi_UTC9 wrote: > nit: s/int/const int/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:367: nit: Please remove an extra blank line. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:380: nit: Please remove an extra blank line. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:400: const int diff = position.computeOffsetInContainerNode() - Let's do DCHECK_GE(position.computeOffsetInContainerNode(), adjustedPosition.computeOffsetInContainerNode()) rather than DCHECK_GE(diff, 0) to print offset values for helping debug. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:425: DCHECK_GE(diff, 0); Ditto as computeCommonGraphemeClusterPrefixLengthForSetComposition https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:449: const bool appending = s/appending/inserting/ https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:451: const bool subtracting = s/subtracting/deleting/ https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:452: composing.length() > commonPrefixLength + +commonSuffixLength; nit: Remove |+| before |comonSuffixLength|. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:455: // Select the text to be subtracted. s/subtracted/deleted/ https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:466: const Document* currentDocument = frame().document(); s/const Document*/Document* const/ https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: if (*currentDocument != *frame().document()) Remove |*|, we don't need to de-reference for comparison. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:479: String appendingText = text.substring(commonPrefixLength, appendingLength); s/String/const String&/ https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:485: if (!frame().document()) No need to checking null document. |currentDocument != frame().document()| is suffice. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:661: frame().document()->updateStyleAndLayoutIgnorePendingStylesheets(); This code fragment should be in callers rather than newly introduced function. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:665: int start = selectionOffsetsStart + selectionStart; s/int/const int/ https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:666: int end = selectionOffsetsStart + selectionEnd; s/int/const int/
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...
https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:367: On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > nit: Please remove an extra blank line. Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:380: On 2016/10/12 06:35:38, Yosi_UTC9 wrote: > nit: Please remove an extra blank line. Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:400: const int diff = position.computeOffsetInContainerNode() - On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > Let's do > DCHECK_GE(position.computeOffsetInContainerNode(), > adjustedPosition.computeOffsetInContainerNode()) > rather than > DCHECK_GE(diff, 0) > to print offset values for helping debug. Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:425: DCHECK_GE(diff, 0); On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > Ditto as computeCommonGraphemeClusterPrefixLengthForSetComposition Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:449: const bool appending = On 2016/10/12 06:35:38, Yosi_UTC9 wrote: > s/appending/inserting/ Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:451: const bool subtracting = On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > s/subtracting/deleting/ Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:452: composing.length() > commonPrefixLength + +commonSuffixLength; On 2016/10/12 06:35:38, Yosi_UTC9 wrote: > nit: Remove |+| before |comonSuffixLength|. Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:455: // Select the text to be subtracted. On 2016/10/12 06:35:38, Yosi_UTC9 wrote: > s/subtracted/deleted/ Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:466: const Document* currentDocument = frame().document(); On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > s/const Document*/Document* const/ Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: if (*currentDocument != *frame().document()) On 2016/10/12 06:35:38, Yosi_UTC9 wrote: > Remove |*|, we don't need to de-reference for comparison. Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:479: String appendingText = text.substring(commonPrefixLength, appendingLength); On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > s/String/const String&/ Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:485: if (!frame().document()) On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > No need to checking null document. |currentDocument != frame().document()| is > suffice. Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:661: frame().document()->updateStyleAndLayoutIgnorePendingStylesheets(); On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > This code fragment should be in callers rather than newly introduced function. Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:665: int start = selectionOffsetsStart + selectionStart; On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > s/int/const int/ Done. https://codereview.chromium.org/2372493002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:666: int end = selectionOffsetsStart + selectionEnd; On 2016/10/12 06:35:37, Yosi_UTC9 wrote: > s/int/const int/ Done.
lgtm w/ small nit https://codereview.chromium.org/2372493002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:434: Element* editable = frame().selection().rootEditableElement(); Just in case, please add DCHECK_GE(selectionStart, 0); DCHECK_LE(selectionStart, selectionEnd);
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~ https://codereview.chromium.org/2372493002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:434: Element* editable = frame().selection().rootEditableElement(); On 2016/10/12 08:12:52, Yosi_UTC9 wrote: > Just in case, please add > DCHECK_GE(selectionStart, 0); > DCHECK_LE(selectionStart, selectionEnd); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
https://codereview.chromium.org/2372493002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:434: Element* editable = frame().selection().rootEditableElement(); On 2016/10/12 08:18:40, yabinh wrote: > On 2016/10/12 08:12:52, Yosi_UTC9 wrote: > > Just in case, please add > > DCHECK_GE(selectionStart, 0); > > DCHECK_LE(selectionStart, selectionEnd); > > Done. I just found that selectionStart and selectionEnd are relative to getSelectionOffsets().start(), so they can be negative. Even if selectionStart + getSelectionOffsets().start() < 0, it can still work, because we will clamp the selection at 0. https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:381: Refactor. https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:411: Refactor.
https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:408: nit: Remove an extra blank line. https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:409: return commonPrefixLength - diff; DCHECK_GE(commonPrefixLength, diff); https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:439: return commonSuffixLength - diff; DCHECK_GE(commonSuffixLength, diff); Tips: For subtract on unsigned, we should check lhs >= rhs to avoid to return huge number.
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...
https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:408: On 2016/10/12 09:49:32, Yosi_UTC9 wrote: > nit: Remove an extra blank line. Done. https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:409: return commonPrefixLength - diff; On 2016/10/12 09:49:32, Yosi_UTC9 wrote: > DCHECK_GE(commonPrefixLength, diff); Done. https://codereview.chromium.org/2372493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:439: return commonSuffixLength - diff; On 2016/10/12 09:49:32, Yosi_UTC9 wrote: > DCHECK_GE(commonSuffixLength, diff); > > Tips: For subtract on unsigned, we should check lhs >= rhs to avoid to return > huge number. Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2372493002/#ps300001 (title: "Add some DCHECK.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) BUG=620549 ========== to ========== Workaround for setComposition styling clobber In order to keep the previous text formatting when continuing typing on the word, this CL detects when the beginning of the IME commit is the same at what was there already, and restricts the write to only the character(s) at the end that actually changed. (This CL is based on aelias@'s CL: https://codereview.chromium.org/2073863003) BUG=620549 Committed: https://crrev.com/993074d17afee685ced99283d7ffbe809fdb6f99 Cr-Commit-Position: refs/heads/master@{#424716} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/993074d17afee685ced99283d7ffbe809fdb6f99 Cr-Commit-Position: refs/heads/master@{#424716} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
