Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763873003:
Adjust common prefix/suffix correctly for InsertIncrementalTextCommand (Closed)
DescriptionAdjust common prefix/suffix correctly for InsertIncrementalTextCommand
In InsertIncrementalTextCommand, when adjusting common prefix/suffix for
grapheme clusters, the old implementation used mismatched offset and node
for the selection. This CL makes them matched. Besides, this CL replaces
wrong DCHECK with truncation when computing common suffix.
BUG=702082
TEST=run_webkit_unit_tests --gtest_filter=
InputMethodControllerTest.SetCompositionWithGraphemeCluster*
Review-Url: https://codereview.chromium.org/2763873003
Cr-Commit-Position: refs/heads/master@{#458669}
Committed: https://chromium.googlesource.com/chromium/src/+/195221de391ce3bf00fb70a7e64995648e535feb
Patch Set 1 #Patch Set 2 #
Total comments: 6
Messages
Total messages: 27 (19 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 ========== stash stash BUG= ========== to ========== Adjust selection correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting the selection for grapheme clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix length. BUG=702082 ==========
Description was changed from ========== Adjust selection correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting the selection for grapheme clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix length. BUG=702082 ========== to ========== Adjust selection correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting the selection for grapheme clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix length. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ==========
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: + yosin@chromium.org
yosin@, PTAL, thanks! https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (left): https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:100: const size_t diff = computeDistanceToRightGraphemeBoundary(position); BTW, I don't think we need worry about the DCHECK in computeCommonGraphemeCluster[Prefix]Length. I couldn't find a case where it fails. So let's keep it as it is.
Description was changed from ========== Adjust selection correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting the selection for grapheme clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix length. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ========== to ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme, clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ==========
Description was changed from ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme, clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ========== to ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ==========
Description was changed from ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ========== to ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ==========
Description was changed from ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ========== to ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ==========
Description was changed from ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ========== to ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces wrong DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (left): https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:100: const size_t diff = computeDistanceToRightGraphemeBoundary(position); On 2017/03/21 at 10:13:44, yabinh wrote: > BTW, I don't think we need worry about the DCHECK in computeCommonGraphemeCluster[Prefix]Length. I couldn't find a case where it fails. So let's keep it as it is. I'm confused. Do you want to keep "newly added" if-statement instead of DCHECK_GE()?
https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (left): https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:100: const size_t diff = computeDistanceToRightGraphemeBoundary(position); On 2017/03/22 02:22:39, yosin_UTC9 wrote: > On 2017/03/21 at 10:13:44, yabinh wrote: > > BTW, I don't think we need worry about the DCHECK in > computeCommonGraphemeCluster[Prefix]Length. I couldn't find a case where it > fails. So let's keep it as it is. > > I'm confused. > > Do you want to keep "newly added" if-statement instead of DCHECK_GE()? Sorry for the confusion. I want to replace DCHECK in [suffix] case, and keep it in [prefix] case.
https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (left): https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:68: const size_t diff = computeDistanceToLeftGraphemeBoundary(position); We don't need replace the DCHECK in [prefix] case.
lgtm
The CQ bit was checked by yabinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces wrong DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster, InputMethodControllerTest.SetCompositionWithGraphemeClusterAndMultipleNodes ========== to ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces wrong DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster* ==========
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490161565743590,
"parent_rev": "24ff0d2969d34f71b43f9f2b96f7ce4d7f0bfd37", "commit_rev":
"195221de391ce3bf00fb70a7e64995648e535feb"}
Message was sent while issue was closed.
Description was changed from ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces wrong DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster* ========== to ========== Adjust common prefix/suffix correctly for InsertIncrementalTextCommand In InsertIncrementalTextCommand, when adjusting common prefix/suffix for grapheme clusters, the old implementation used mismatched offset and node for the selection. This CL makes them matched. Besides, this CL replaces wrong DCHECK with truncation when computing common suffix. BUG=702082 TEST=run_webkit_unit_tests --gtest_filter= InputMethodControllerTest.SetCompositionWithGraphemeCluster* Review-Url: https://codereview.chromium.org/2763873003 Cr-Commit-Position: refs/heads/master@{#458669} Committed: https://chromium.googlesource.com/chromium/src/+/195221de391ce3bf00fb70a7e649... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/195221de391ce3bf00fb70a7e649...
Message was sent while issue was closed.
changwan@chromium.org changed reviewers: + changwan@chromium.org
Message was sent while issue was closed.
Hi, we are considering merging this to M57 in a couple of hours. Are we confident that this is safe? I left some question and comment inline. https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp (right): https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:60: selectionStart.computeContainerNode()->parentNode(); Is it safe not to have a null check here? I mean, both for containernode and parent node. BTW, it seems that selectionNode here just means rootEditable, not the selection itself. https://codereview.chromium.org/2763873003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp:93: selectionStart.computeContainerNode()->parentNode(); Same comment applies here. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
