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

Issue 2665923002: Modify DocumentMarkerController to support overlapping and nested DocumentMarkers (Closed)

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

Description

Modify DocumentMarkerController to support overlapping and nested DocumentMarkers I have a CL in progress right now that adds support for Android SuggestionSpans (underlined ranges of text marked by the IME that a user can tap on to get a list of suggested replacements): https://codereview.chromium.org/2650113004 I was refactoring it and noticed DocumentMarkerController::removeMarkers() doesn't work properly with nested markers. There's a comment above addMarker() noting that markers of the same type should not overlap, but then in the very same method, mergeOverlapping() is not called for TextMatch or Composition markers, which suggests that those marker types *are* allowed to overlap. So I think this comment is out-of-date, and someone added support for overlapping markers without fixing removeMarkers(), which is unfortunate. Basically the issue is that markers are stored ordered by start position, and if they're allowed to overlap, we can't assume that ordering them by end position results in the same order, so I've just removed any code from this file that tries to make that faulty assumption (so I also updated setMarkersActive()). BUG=686850

Patch Set 1 #

Total comments: 1

Patch Set 2 : Fix DocumentMarkerController::setMarkersActive() #

Patch Set 3 : Update commit message #

Patch Set 4 : Make sure DocumentMarker lists stay sorted after removing a range that overlaps multiple markers #

Patch Set 5 : Remove comment that accidentally got left in #

Patch Set 6 : Remove debug code accidentally left in test #

Total comments: 2

Patch Set 7 : Clean up according to yosin's comments #

Patch Set 8 : Fix bug (caught by layout test) #

Patch Set 9 : Fix bug resulting from potential iterator invalidation (one of the trybots seems to have run into t… #

Total comments: 4

Patch Set 10 : Use range-for statement in setMarkersActive() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+179 lines, -33 lines) Patch
M third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp View 1 2 3 4 5 6 1 chunk +153 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp View 1 2 3 4 5 6 7 8 9 5 chunks +26 lines, -33 lines 0 comments Download

Messages

Total messages: 73 (43 generated)
rlanday
https://codereview.chromium.org/2665923002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): https://codereview.chromium.org/2665923002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp#newcode285 third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:285: markerController().removeMarkers( This actually crashes DocumentMarkerController without my patch!
3 years, 10 months ago (2017-01-30 22:45:10 UTC) #3
rlanday
3 years, 10 months ago (2017-02-01 02:24:42 UTC) #9
Xiaocheng
Could you give an example of why overlapping markers are needed? The no-merging behavior for ...
3 years, 10 months ago (2017-02-01 08:27:57 UTC) #14
rlanday
On 2017/02/01 at 08:27:57, xiaochengh wrote: > Could you give an example of why overlapping ...
3 years, 10 months ago (2017-02-01 18:13:35 UTC) #15
Xiaocheng
The example is reasonable. I'm OK with the code change. Please also fix the patch ...
3 years, 10 months ago (2017-02-02 04:23:32 UTC) #16
rlanday
Ok, I've updated the description (and updated the commit message, if that's necessary)
3 years, 10 months ago (2017-02-02 18:12:30 UTC) #18
Xiaocheng
lgtm +tkent as owner
3 years, 10 months ago (2017-02-03 04:30:19 UTC) #20
tkent
lgtm
3 years, 10 months ago (2017-02-03 04:38:48 UTC) #21
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/2665923002/40001
3 years, 10 months ago (2017-02-03 18:55:36 UTC) #23
commit-bot: I haz the power
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from ...
3 years, 10 months ago (2017-02-03 18:55:38 UTC) #25
rlanday
I just noticed there's actually another thing I think I should fix in this CL: ...
3 years, 10 months ago (2017-02-03 19:29:17 UTC) #27
rlanday
On 2017/02/03 at 19:29:17, rlanday wrote: > I just noticed there's actually another thing I ...
3 years, 10 months ago (2017-02-03 20:34:09 UTC) #31
yosin_UTC9
https://codereview.chromium.org/2665923002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2665923002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode418 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:418: std::sort(list->begin(), list->end(), compareByStart); Can we do smarter way rather ...
3 years, 10 months ago (2017-02-06 04:54:58 UTC) #39
rlanday
https://codereview.chromium.org/2665923002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2665923002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode418 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:418: std::sort(list->begin(), list->end(), compareByStart); On 2017/02/06 at 04:54:58, yosin_UTC9 wrote: ...
3 years, 10 months ago (2017-02-09 18:27:40 UTC) #40
rlanday
@yosin, I think I've addressed your comments
3 years, 10 months ago (2017-02-09 23:26:58 UTC) #41
yosin_UTC9
https://codereview.chromium.org/2665923002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2665923002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode373 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:373: for (size_t markerIndex = 0; markerIndex < list->size();) { ...
3 years, 10 months ago (2017-02-10 07:06:27 UTC) #52
yosin_UTC9
>There's a comment above addMarker() >noting that markers of the same type should not overlap, ...
3 years, 10 months ago (2017-02-10 07:30:42 UTC) #53
rlanday
On 2017/02/10 at 07:30:42, yosin wrote: > >There's a comment above addMarker() > >noting that ...
3 years, 10 months ago (2017-02-10 18:59:23 UTC) #56
rlanday
https://codereview.chromium.org/2665923002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2665923002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode373 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:373: for (size_t markerIndex = 0; markerIndex < list->size();) { ...
3 years, 10 months ago (2017-02-10 18:59:35 UTC) #57
yosin_UTC9
On 2017/02/10 at 18:59:35, rlanday wrote: > https://codereview.chromium.org/2665923002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2665923002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode373 ...
3 years, 10 months ago (2017-02-13 08:14:21 UTC) #62
rlanday
> > I don't know of a good way we can use remove_if() and also ...
3 years, 10 months ago (2017-02-13 21:53:28 UTC) #63
yosin_UTC9
On 2017/02/13 at 21:53:28, rlanday wrote: > > > I don't know of a good ...
3 years, 10 months ago (2017-02-14 07:49:03 UTC) #64
rlanday
On 2017/02/14 at 07:49:03, yosin wrote: > On 2017/02/13 at 21:53:28, rlanday wrote: > > ...
3 years, 10 months ago (2017-02-14 21:01:44 UTC) #65
yosin_UTC9
On 2017/02/14 at 21:01:44, rlanday wrote: > On 2017/02/14 at 07:49:03, yosin wrote: > > ...
3 years, 10 months ago (2017-02-15 01:47:33 UTC) #66
rlanday
> > So then we're no longer going to keep the list of markers sorted ...
3 years, 10 months ago (2017-02-15 02:26:56 UTC) #67
yosin_UTC9
On 2017/02/15 at 02:26:56, rlanday wrote: > > > So then we're no longer going ...
3 years, 10 months ago (2017-02-15 04:02:53 UTC) #68
yosin_UTC9
On 2017/02/15 at 04:02:53, yosin_UTC9 wrote: > On 2017/02/15 at 02:26:56, rlanday wrote: > > ...
3 years, 10 months ago (2017-02-15 04:09:11 UTC) #69
rlanday
> > I think it is better to have different implementation for each marker type. ...
3 years, 10 months ago (2017-02-15 04:53:18 UTC) #70
yosin_UTC9
On 2017/02/15 at 04:53:18, rlanday wrote: > > > I think it is better to ...
3 years, 10 months ago (2017-02-15 10:19:41 UTC) #71
aelias_OOO_until_Jul13
3 years, 10 months ago (2017-02-15 22:40:18 UTC) #73
OK, that makes sense.  We wrote a specific class hierarchy proposal for
DocumentMarker and DocumentMarkerList at the bottom of rlanday@'s doc
https://docs.google.com/document/d/1IQQTZS3T0QdP8eb9tOea2HNhbjOJxmRDiWJAHMzdm...
.  yosin@, please let us know if that looks OK, and rlanday@ will start
implementing the refactoring.

Powered by Google App Engine
This is Rietveld 408576698