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

Issue 2723663002: Refactor DocumentMarkerController (Closed)

Created:
3 years, 9 months ago by rlanday
Modified:
3 years, 8 months ago
CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-frames_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dglazkov+blink, dshwang, eae+blinkwatch, groby+blinkspell_chromium.org, kinuko+watch, rwlbuis, sof, timvolodine
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Refactor DocumentMarkerController Based on this design doc: https://docs.google.com/document/d/1BORj8rxmLc52rdeGq-uazinmDO_sDVmIZa_SGliPo6w/ Notes: - The MarkerMaps are all stored as maps from Node* to Member<DocumentMarkerList>. So when we call methods that need a certain derived type of DocumentMarkerList, we have to use a cast function. - We talked about getting rid of DocumentMarkerDetails and using polymorphism in DocumentMarker instead. I haven't done that in this CL since it seems orthogonal to refactoring DocumentMarkerController itself. - Right now the way EditingMarkerList handles the sorted/not sorted cases is kind of lame. Right now the only time the list gets sorted is when we need to merge overlapping (touching really, none of the DocumentMarker types that currently exist should actually be overlapped) markers, since that would otherwise be an O(n^2) time operation and sorting lets us do it in O(n log n). The design doc mentions sorting "when log is bigger than threshold", but I don't know how we would determine this. Note that any individual method call (except for mergeOverlapping(), where I added the sort) is faster if we don't sort. Sorting only speeds things up if we expect a lot of method calls that we would be able to do faster if the list were sorted. The design doc also mentions sorting before painting; I haven't added that yet since I don't understand the rationale. Ideally I'd like to avoid having any methods have special-cases for sorted vs. not sorted like I'm doing now. Or maybe we can put code to handle the "sorted" and "not-sorted" versions in a base class so we can e.g. share code between the sorted case for EditingMarkerList with TextMatchMarkerList. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=707867

Patch Set 1 #

Total comments: 6

Patch Set 2 : Remove API changes #

Patch Set 3 : Add logging to try to debug test failures #

Patch Set 4 : More logging (to figure out why handleTextBox() gets called on the Windows trybot but not on my mac… #

Patch Set 5 : Remove logging #

Total comments: 2

Patch Set 6 : Fix short-circuit evaluation bugs, slightly reduce code duplication #

Patch Set 7 : Clean up slightly, fix iterator bug in TextMatchMarkerList #

Patch Set 8 : Rebase on https://codereview.chromium.org/2755013004 #

Total comments: 15

Patch Set 9 : Fix bugs in didUpdateCharacterData #

Total comments: 16

Patch Set 10 : Make requested changes (and fix !=/< iterator bug in TextMatchMarkerList) #

Patch Set 11 : Use correct base commit #

Total comments: 16

Patch Set 12 : Make requested changes, rebase (HashMap::remove() => HashMap::erase()) #

Total comments: 1

Patch Set 13 : Attempt to correctly set dependent CL so trybots can build #

Total comments: 6

Patch Set 14 : Don't use static_cast, reduce code duplication in addMarker(), attempt to fix MSVC compilation #

Patch Set 15 : Eliminate more code duplication using the MarkerTypes iterator I'm adding #

Patch Set 16 : Rebase #

Patch Set 17 : Add NOTREACHABLE() to DocumentMarkerController::createMarkerListOfType() #

Patch Set 18 : Use correct base commit #

Patch Set 19 : Clean up duplication in DocumentMarkerController::hasMarkers() and showMarkers() #

Patch Set 20 : Split off DocumentMarkerList + subclasses into other CLs #

Patch Set 21 : Rebase #

Patch Set 22 : Ensure TextMatch markers are only created as the derived class #

Patch Set 23 : Rebase #

Patch Set 24 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+327 lines, -765 lines) Patch
M third_party/WebKit/Source/core/editing/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 15 chunks +26 lines, -68 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 17 19 20 21 22 5 chunks +18 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 16 chunks +273 lines, -551 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +4 lines, -32 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -1 line 0 comments Download
D third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -90 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 120 (91 generated)
rlanday
@dcheng, I need your LGTM for the minor change to third_party/WebKit/Source/web/TextFinder.cpp @yutak, I need your ...
3 years, 9 months ago (2017-02-28 00:53:51 UTC) #6
rlanday
There is a browser test case, FindInPageControllerTest.SpanAndListsSearchable, failing because it's violating our assumption that the ...
3 years, 9 months ago (2017-02-28 21:54:57 UTC) #9
Xiaocheng
This is quite a big patch. As the changes outside editing/markers/ are just function renaming, ...
3 years, 9 months ago (2017-03-01 22:24:49 UTC) #11
rlanday
Ok, I can split up the patch. I've filed a bug for the overlapping marker ...
3 years, 9 months ago (2017-03-01 23:27:50 UTC) #12
rlanday
I'm going to try to reduce the amount of code duplication a bit
3 years, 9 months ago (2017-03-17 01:40:08 UTC) #31
rlanday
https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode220 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:220: docDirty = docDirty || spellingIt->value->copyMarkers(startOffset, length, There's a bug ...
3 years, 9 months ago (2017-03-17 01:40:15 UTC) #32
rlanday
On 2017/03/17 at 01:40:15, rlanday wrote: > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode220 ...
3 years, 9 months ago (2017-03-17 03:18:42 UTC) #35
rlanday
On 2017/03/17 at 03:18:42, rlanday wrote: > On 2017/03/17 at 01:40:15, rlanday wrote: > > ...
3 years, 9 months ago (2017-03-17 03:19:14 UTC) #36
rlanday
For some reason, some test cases that were previously passing started failing for me locally; ...
3 years, 9 months ago (2017-03-17 21:25:35 UTC) #39
Xiaocheng
On 2017/03/17 at 21:25:35, rlanday wrote: > For some reason, some test cases that were ...
3 years, 9 months ago (2017-03-17 21:58:16 UTC) #42
rlanday
On 2017/03/17 at 21:58:16, xiaochengh wrote: > On 2017/03/17 at 21:25:35, rlanday wrote: > > ...
3 years, 9 months ago (2017-03-17 22:48:38 UTC) #45
Xiaocheng
Haven't gone through the entire patch. I'll finish reading this patch and give more comments ...
3 years, 9 months ago (2017-03-17 23:56:34 UTC) #48
rlanday
https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h#oldcode124 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:124: Vector<IntRect> renderedRectsForMarkers(DocumentMarker::MarkerType); On 2017/03/17 at 23:56:33, Xiaocheng wrote: > ...
3 years, 9 months ago (2017-03-18 01:12:27 UTC) #51
Xiaocheng
https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h#oldcode124 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:124: Vector<IntRect> renderedRectsForMarkers(DocumentMarker::MarkerType); On 2017/03/18 at 01:12:27, rlanday wrote: > ...
3 years, 9 months ago (2017-03-18 01:58:09 UTC) #52
Xiaocheng
https://codereview.chromium.org/2723663002/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/2723663002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode91 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:91: new DocumentMarker(markedText.startOffsetInCurrentContainer(), These DocumentMarker ctors really look awkward... It ...
3 years, 9 months ago (2017-03-18 06:43:14 UTC) #53
rlanday
https://codereview.chromium.org/2723663002/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/2723663002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode91 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:91: new DocumentMarker(markedText.startOffsetInCurrentContainer(), On 2017/03/18 at 06:43:14, Xiaocheng wrote: > ...
3 years, 9 months ago (2017-03-20 18:13:30 UTC) #54
dglazkov
BTW, so glad y'all are working on this. This code needed love for so long.
3 years, 9 months ago (2017-03-20 19:40:09 UTC) #55
Xiaocheng
Some minor comments after a quick glance https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h File third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h#newcode12 third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:12: class CompositionMarkerList ...
3 years, 9 months ago (2017-03-21 03:59:45 UTC) #61
Xiaocheng
https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Source/core/editing/markers/DocumentMarker.h#newcode127 third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:127: ShiftMarkerResult getShiftedMarkerPosition(unsigned offset, nit: add |const| https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp ...
3 years, 9 months ago (2017-03-21 19:12:12 UTC) #62
rlanday
https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h#oldcode52 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:52: private: On 2017/03/21 at 19:12:11, Xiaocheng wrote: > Irrelevant ...
3 years, 9 months ago (2017-03-21 19:49:12 UTC) #63
rlanday
Updated
3 years, 9 months ago (2017-03-21 21:17:35 UTC) #65
rlanday
https://codereview.chromium.org/2723663002/diff/220001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/220001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode218 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:218: static_cast<DocumentMarker::MarkerType>(1 << markerListIndex); Iterating over the MarkerTypes like this ...
3 years, 9 months ago (2017-03-21 21:19:07 UTC) #67
Xiaocheng
https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode157 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:157: static_cast<TextMatchMarkerList*>(it->value.get())->push_back(newMarker); Don't use static_cast. Use DEFINE_TYPE_CASTS to define |toFooMarkerList()| ...
3 years, 9 months ago (2017-03-21 22:42:20 UTC) #74
rlanday
On 2017/03/21 at 22:42:20, xiaochengh wrote: > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode157 ...
3 years, 9 months ago (2017-03-23 01:15:56 UTC) #94
rlanday
I suppose we should add test cases for all the different types of DocumentMarkerList now?
3 years, 9 months ago (2017-03-23 01:29:58 UTC) #95
Xiaocheng
On 2017/03/23 at 01:29:58, rlanday wrote: > I suppose we should add test cases for ...
3 years, 9 months ago (2017-03-23 02:22:39 UTC) #96
rlanday
On 2017/03/23 at 02:22:39, xiaochengh wrote: > On 2017/03/23 at 01:29:58, rlanday wrote: > > ...
3 years, 8 months ago (2017-03-29 00:32:38 UTC) #101
rlanday
On 2017/03/29 at 00:32:38, rlanday wrote: > On 2017/03/23 at 02:22:39, xiaochengh wrote: > > ...
3 years, 8 months ago (2017-03-30 20:53:49 UTC) #116
rlanday
3 years, 8 months ago (2017-03-30 21:05:58 UTC) #119
On 2017/03/30 at 20:53:49, rlanday wrote:
> I've updated this again, hopefully the trybots will turn green and not red
this time :)

I appear to have run into a bug with the trybot patch system where git will
complain about applying a patch to remove a file that an earlier patch modified
(https://codereview.chromium.org/2781623010 to add DocumentMarker::clone()
modifies RenderedDocumentMarker.h). So this patch and its dependencies will most
likely not be able to build on the trybots until the clone() CL lands.

Powered by Google App Engine
This is Rietveld 408576698