Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806683002:
Don't ever split DocumentMarkers on remove (Closed)
DescriptionDon't ever split DocumentMarkers on remove
Right now some of the DocumentMarkerController::removeMarkers() methods take an
enum flag, RemovePartiallyOverlappingMarkerOrNot that controls what the method
does with markers that partially intersect the passed-in range but do not fall
entirely within it. If the value DoNotRemovePartiallyOverlappingMarker is
passed, the section in the passed-in range is removed and the remaining piece(s)
of the marker are left in place (otherwise the entire marker is removed). If the
passed-in range contains part of the interior of the marker but neither the
start nor end position, this means the marker is split into two pieces, which
requires copying the marker. yosin thinks copying markers is weird and we should
try to avoid ever having to do it. I believe this is possible if we eliminate
this flag and just never split markers on removal.
I don't think any of the currently-existing marker types (Spelling, Grammar,
Composition, TextMatch) ever make sense to split, and neither do the new types
I'm going to add (Suggestion, SuggestionHighlight). The only test cases I had to
update after this change are ones I added based on the current behavior.
BUG=707867
Review-Url: https://codereview.chromium.org/2806683002
Cr-Commit-Position: refs/heads/master@{#464524}
Committed: https://chromium.googlesource.com/chromium/src/+/98f1927737cd1c3fd5d471b74cc94333e9e37972
Patch Set 1 #Patch Set 2 : Leave partial markers in place for non-splitting case, preserve endpoints for splitting case #
Total comments: 1
Patch Set 3 : Move deletion behavior change to previous CL #Patch Set 4 : Rebase on test cases I'm adding in another CL #
Total comments: 1
Patch Set 5 : Rebase, remove enum #Patch Set 6 : Rebase (no manual changes) #
Messages
Total messages: 45 (24 generated)
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
The CQ bit was checked by xiaochengh@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...
Is there a more detailed analysis of the existing call sites, so that we are sure they don't really need to keep partial markers?
On 2017/04/07 at 02:22:42, xiaochengh wrote: > Is there a more detailed analysis of the existing call sites, so that we are sure they don't really need to keep partial markers? The call sites (ignoring the removeMarkers() overloads that remove all markers vs. taking a range) are: https://cs.chromium.org/search/?q=removemarkers+package:%5Echromium$&m=25&typ... - SpellChecker::removeMarkers(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s... Called by SpellChecker::ignoreSpelling(); I haven't traced back exactly how this works, but I think it's pretty clear that if you have a misspelled word and want to ignore the misspelling, splitting the marker and ending up with two new misspelled words doesn't make any sense - SpellChecker::updateMarkersForWordsAffectedByEditing(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s... passes DocumentMarkerController::RemovePartiallyOverlappingMarker so there's no behavioral change here - TextFinder::stopFindingAndClearSelection(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/TextFinder... - SpellCheckRequester::didCheckSucceed(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s... Again, not sure exactly how this works but either this doesn't need to split the spellcheck marker, or there's some case where it's currently checking half a word and removing half a marker, which doesn't make any sense... - HotModeSpellCheckRequester::checkSpellingAt(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s... Passes emovePartiallyOverlappingMarker, no behavioral change here
On 2017/04/07 at 03:02:49, rlanday wrote: > On 2017/04/07 at 02:22:42, xiaochengh wrote: > > Is there a more detailed analysis of the existing call sites, so that we are sure they don't really need to keep partial markers? > > TextFinder::stopFindingAndClearSelection(): > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/TextFinder... Ignore this one, this one doesn't actually pass a range
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It seems we need to study for didUpdateCharacterData() with calls removeMarkers() with |DoNotRemovePartiallyOverlappingMarker|. Other than IME, all markers based on content == if content is changed, marker becomes meaningless. But for IME marker, it is based on range. Actually, I'm not sure we change Text node content with IME marker. # Brain dump: Summary of changes: Before: We keep partially overlapped markers == split marker After: We remove partially overlapped makers == no split marker There are two use case for removeMakers(): 1. Remove spelling markers which need to be checking again by Spell checker. 2. Text node associated to marker is modified; via didUpdateCharacterData() For case #1, it seems OK to remove partially removed markers as rlandy@ analysis. For case #2, we should remove overlapped markers since text was changed, e.g. "dox" => "do"; no more misspell, no more text match. How about IME? Note: |RemovePartiallyOverlappingMarkerOrNot| is introduced by patch[1]. [1] http://wkb.ug/53255 - Reversion should not be marked as misspelled Changeset: https://trac.webkit.org/changeset/77577/webkit
On 2017/04/07 at 04:05:50, yosin wrote: > For case #2, we should remove overlapped markers since text was changed, e.g. > "dox" => "do"; no more misspell, no more text match. How about IME? The current behavior (in trunk) when you remove a piece of text that partially overlaps a marker is to leave the other part of the marker in place. For example, check out the old behavior of Marker_Deletions. If you have a marker on "abc" and delete "b", the current behavior is that you end up with "ac" with one marker on "a" and another on "c". This CL changes the behavior so that the marker is just removed. However, we could change also change the behavior so that you just end up with one marker on "ac" (I think this is actually what I meant to do). Currently, my patch that changes how markers are updated (https://codereview.chromium.org/2755013004/) treats deletions and replacements differently. For example if you have "abc" and delete "b", you'll end up with two markers ("a" and "c) but if you replace "b" with "d", you'll end up with only one marker on "adc". I think we should probably make this behavior consistent. Suggestion markers are kind of an awkward in-between case: we need to not remove the marker if the text was altered by a suggestion replacement operation, but we do want to remove the marker if the text is altered for any other reason.
I don't think the composition marker needs to be updated in response to text edits, or the current version of the code wouldn't work properly (DocumentMarkerController::shiftMarkers() never changes the length of a marker right now)
On 2017/04/07 at 17:45:07, rlanday wrote: > On 2017/04/07 at 04:05:50, yosin wrote: > > For case #2, we should remove overlapped markers since text was changed, e.g. > > "dox" => "do"; no more misspell, no more text match. How about IME? > > The current behavior (in trunk) when you remove a piece of text that partially overlaps a marker is to leave the other part of the marker in place. For example, check out the old behavior of Marker_Deletions. > > If you have a marker on "abc" and delete "b", the current behavior is that you end up with "ac" with one marker on "a" and another on "c". This CL changes the behavior so that the marker is just removed. However, we could change also change the behavior so that you just end up with one marker on "ac" (I think this is actually what I meant to do). > > Currently, my patch that changes how markers are updated (https://codereview.chromium.org/2755013004/) treats deletions and replacements differently. For example if you have "abc" and delete "b", you'll end up with two markers ("a" and "c) but if you replace "b" with "d", you'll end up with only one marker on "adc". I think we should probably make this behavior consistent. We should land crrev.com/2755013004 first, so that we don't spend time studying behavior that will be removed. > > Suggestion markers are kind of an awkward in-between case: we need to not remove the marker if the text was altered by a suggestion replacement operation, but we do want to remove the marker if the text is altered for any other reason.
On 2017/04/07 at 19:03:55, xiaochengh wrote: > We should land crrev.com/2755013004 first, so that we don't spend time studying behavior that will be removed. Yeah, this CL is stacked on top of that one. I was trying to avoid changing the existing behavior too much in that CL but I think it would be nice to make deleting a range of text behave the same as replacing the range with the empty string. Since replacing with a non-empty string can't split or remove the marker (so we can properly handle suggestion markers), I think replacing with an empty string should behave the same way (don't split or remove the marker, just try to keep the endpoints in the same place relative to the text), and therefore deleting should also not split or remove the marker. Does that sound like a good plan? I will update this CL to do that.
On 2017/04/07 at 23:53:53, rlanday wrote: > On 2017/04/07 at 19:03:55, xiaochengh wrote: > > We should land crrev.com/2755013004 first, so that we don't spend time studying behavior that will be removed. > > Yeah, this CL is stacked on top of that one. > > I was trying to avoid changing the existing behavior too much in that CL but I think it would be nice to make deleting a range of text behave the same as replacing the range with the empty string. Since replacing with a non-empty string can't split or remove the marker (so we can properly handle suggestion markers), I think replacing with an empty string should behave the same way (don't split or remove the marker, just try to keep the endpoints in the same place relative to the text), and therefore deleting should also not split or remove the marker. Does that sound like a good plan? I will update this CL to do that. Doesn't crrev.com/2755013004 already changes the behavior into what you described (i.e., "deleting a range of text behave the same as replacing the range with the empty string")?
The CQ bit was checked by rlanday@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...
On 2017/04/08 at 00:03:33, xiaochengh wrote: > On 2017/04/07 at 23:53:53, rlanday wrote: > > On 2017/04/07 at 19:03:55, xiaochengh wrote: > > > We should land crrev.com/2755013004 first, so that we don't spend time studying behavior that will be removed. > > > > Yeah, this CL is stacked on top of that one. > > > > I was trying to avoid changing the existing behavior too much in that CL but I think it would be nice to make deleting a range of text behave the same as replacing the range with the empty string. Since replacing with a non-empty string can't split or remove the marker (so we can properly handle suggestion markers), I think replacing with an empty string should behave the same way (don't split or remove the marker, just try to keep the endpoints in the same place relative to the text), and therefore deleting should also not split or remove the marker. Does that sound like a good plan? I will update this CL to do that. > > Doesn't crrev.com/2755013004 already changes the behavior into what you described (i.e., "deleting a range of text behave the same as replacing the range with the empty string")? Oh it does (since there's no way to distinguish these cases), but it splits the marker for both, whereas that's no longer an option if we're trying to avoid copying the marker. So it does the same behavior for both, but it's a different behavior than the one we need.
I've updated the CL. Note that we only need to change one test case now vs. two, so I think this is a smaller behavioral change than what I originally had up.
https://codereview.chromium.org/2806683002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2806683002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:832: // If we're doing a pure remove operation, remove the markers in the range I see. But this change fits better in the previous patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/08 at 00:23:36, xiaochengh wrote: > https://codereview.chromium.org/2806683002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): > > https://codereview.chromium.org/2806683002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:832: // If we're doing a pure remove operation, remove the markers in the range > I see. > > But this change fits better in the previous patch. Ok, I've done that
lgtm with a nit. https://codereview.chromium.org/2806683002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2806683002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:88: enum RemovePartiallyOverlappingMarkerOrNot { Please also remove this enum.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/04/10 at 21:22:10, xiaochengh wrote: > lgtm with a nit. > > https://codereview.chromium.org/2806683002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): > > https://codereview.chromium.org/2806683002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:88: enum RemovePartiallyOverlappingMarkerOrNot { > Please also remove this enum. Updated I'll fix the trybots once https://codereview.chromium.org/2812723002 (the CL that adds tests for this method) lands
lgtm Thanks for simplification!
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2806683002/#ps80001 (title: "Rebase, remove enum")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rlanday@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.
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2806683002/#ps100001 (title: "Rebase (no manual changes)")
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": 100001, "attempt_start_ts": 1492113961242470,
"parent_rev": "d9cff176581d901a8b2b1fed51035c13418f87b9", "commit_rev":
"98f1927737cd1c3fd5d471b74cc94333e9e37972"}
Message was sent while issue was closed.
Description was changed from ========== Don't ever split DocumentMarkers on remove Right now some of the DocumentMarkerController::removeMarkers() methods take an enum flag, RemovePartiallyOverlappingMarkerOrNot that controls what the method does with markers that partially intersect the passed-in range but do not fall entirely within it. If the value DoNotRemovePartiallyOverlappingMarker is passed, the section in the passed-in range is removed and the remaining piece(s) of the marker are left in place (otherwise the entire marker is removed). If the passed-in range contains part of the interior of the marker but neither the start nor end position, this means the marker is split into two pieces, which requires copying the marker. yosin thinks copying markers is weird and we should try to avoid ever having to do it. I believe this is possible if we eliminate this flag and just never split markers on removal. I don't think any of the currently-existing marker types (Spelling, Grammar, Composition, TextMatch) ever make sense to split, and neither do the new types I'm going to add (Suggestion, SuggestionHighlight). The only test cases I had to update after this change are ones I added based on the current behavior. BUG=707867 ========== to ========== Don't ever split DocumentMarkers on remove Right now some of the DocumentMarkerController::removeMarkers() methods take an enum flag, RemovePartiallyOverlappingMarkerOrNot that controls what the method does with markers that partially intersect the passed-in range but do not fall entirely within it. If the value DoNotRemovePartiallyOverlappingMarker is passed, the section in the passed-in range is removed and the remaining piece(s) of the marker are left in place (otherwise the entire marker is removed). If the passed-in range contains part of the interior of the marker but neither the start nor end position, this means the marker is split into two pieces, which requires copying the marker. yosin thinks copying markers is weird and we should try to avoid ever having to do it. I believe this is possible if we eliminate this flag and just never split markers on removal. I don't think any of the currently-existing marker types (Spelling, Grammar, Composition, TextMatch) ever make sense to split, and neither do the new types I'm going to add (Suggestion, SuggestionHighlight). The only test cases I had to update after this change are ones I added based on the current behavior. BUG=707867 Review-Url: https://codereview.chromium.org/2806683002 Cr-Commit-Position: refs/heads/master@{#464524} Committed: https://chromium.googlesource.com/chromium/src/+/98f1927737cd1c3fd5d471b74cc9... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/98f1927737cd1c3fd5d471b74cc9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
