Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(58)

Issue 2895353003: Split DMLEditor::ShiftMarkers() into content-dependent and -independent versions (Closed)

Created:
3 years, 7 months ago by rlanday
Modified:
3 years, 7 months ago
Reviewers:
yosin_UTC9, Xiaocheng
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Split DMLEditor::ShiftMarkers() into content-dependent and -independent versions I modified the logic that updates DocumentMarker in response to edit operations in https://codereview.chromium.org/2755013004. However, it introduced a bug when doing spellcheck replace (the old behavior removed the marker when the marked text was changed and the new behavior doesn't). This CL renames DocumentMarkerListEditor::ShiftMarkers() to ShiftMarkersContentIndependent() and adds a new method that implements the old behavior as ShiftMarkersContentDependent(). Spelling, Grammar, and TextMatch markers should be removed when the text they mark changes, so those marker list impls are being changed to use the old behavior. Composition markers don't depend on the text they mark, so they're still using the new behavior (although in practice it doesn't matter much since Composition markers are cleared by most editing operations). Where we really need the new behavior is to implement Android SuggestionSpan support (see crbug.com/672259). As all non-Composition markers currently use the ContentDependent behavior, I had to remove/change the expected behavior for a lot of InputMethodController tests that were testing the new update behavior. BUG=707867, 722721 Review-Url: https://codereview.chromium.org/2895353003 Cr-Commit-Position: refs/heads/master@{#474245} Committed: https://chromium.googlesource.com/chromium/src/+/25c86433d672606b4daaceecc589664365d63ddf

Patch Set 1 #

Messages

Total messages: 15 (8 generated)
rlanday
I'm planning to re-add the current versions of the InputMethodControllerTest cases I'm changing when I ...
3 years, 7 months ago (2017-05-22 22:40:38 UTC) #4
Xiaocheng
lgtm judging only from code change (since this patch is requested by yosin@).
3 years, 7 months ago (2017-05-24 05:26:30 UTC) #7
yosin_UTC9
On 2017/05/22 at 22:40:38, rlanday wrote: >Should I add some tests for testing DocumentMarkerListEditor::ShiftMarkersContent[Ind|D]ependent() directly? ...
3 years, 7 months ago (2017-05-24 08:57:51 UTC) #8
yosin_UTC9
lgtm
3 years, 7 months ago (2017-05-24 08:58:15 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2895353003/1
3 years, 7 months ago (2017-05-24 08:58:36 UTC) #11
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/25c86433d672606b4daaceecc589664365d63ddf
3 years, 7 months ago (2017-05-24 10:54:50 UTC) #14
rlanday
3 years, 7 months ago (2017-05-24 21:29:32 UTC) #15
Message was sent while issue was closed.
Should we merge this into version 59? Or should we merge the one-line fix at
https://codereview.chromium.org/2891403004 ?

Powered by Google App Engine
This is Rietveld 408576698