|
|
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. |
DescriptionRefactor 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 #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 120 (91 generated)
Description was changed from ========== Refactor DocumentMarkerController Based on this design doc: https://docs.google.com/document/d/1BORj8rxmLc52rdeGq-uazinmDO_sDVmIZa_SGliPo6w/ Notes: - The doc shows us storing the marker map as a map from a node to a list of marker lists for the node (one for each marker type). In this CL, I instead have a separate map for each type of marker. I haven't thought very hard about the tradeoffs but it seemed a little bit more straightforward to implement this way. - There is more code duplication in DocumentMarkerController than I would like. I would like to have a method that returns a list of all the MarkerMaps but I don't think there's any common type that would work (e.g. you can't cast a map from Node* to GrammarMarkerList* to a map from Node* to DocumentMarkerList* because then you'd be able to take the casted map and put a type of DocumentMarkerList in it that you shouldn't be able to. Also, it doesn't seem like good design to store the maps as maps from Node* to DocumentMarker* and use polymorphism because then we would need to use casting to call the marker type-specific methods. Maybe the solution is to switch to using a single map that maps to a list of marker lists as in the design doc? Then we could have a method that gives us all the DocumentMarkerLists for a given node and we could type them as DocumentMarkerList*. - 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. ========== to ========== Refactor DocumentMarkerController Based on this design doc: https://docs.google.com/document/d/1BORj8rxmLc52rdeGq-uazinmDO_sDVmIZa_SGliPo6w/ Notes: - The doc shows us storing the marker map as a map from a node to a list of marker lists for the node (one for each marker type). In this CL, I instead have a separate map for each type of marker. I haven't thought very hard about the tradeoffs but it seemed a little bit more straightforward to implement this way. - There is more code duplication in DocumentMarkerController than I would like. I would like to have a method that returns a list of all the MarkerMaps but I don't think there's any common type that would work (e.g. you can't cast a map from Node* to GrammarMarkerList* to a map from Node* to DocumentMarkerList* because then you'd be able to take the casted map and put a type of DocumentMarkerList in it that you shouldn't be able to. Also, it doesn't seem like good design to store the maps as maps from Node* to DocumentMarker* and use polymorphism because then we would need to use casting to call the marker type-specific methods. Maybe the solution is to switch to using a single map that maps to a list of marker lists as in the design doc? Then we could have a method that gives us all the DocumentMarkerLists for a given node and we could type them as DocumentMarkerList*. - 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 ==========
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...
rlanday@chromium.org changed reviewers: + dcheng@chromium.org, yosin@chromium.org, yutak@chromium.org
rlanday@chromium.org changed required reviewers: + dcheng@chromium.org, yutak@chromium.org
@dcheng, I need your LGTM for the minor change to third_party/WebKit/Source/web/TextFinder.cpp @yutak, I need your LGTM for: - third_party/WebKit/Source/core/dom/Document.cpp - third_party/WebKit/Source/core/frame/FrameView.cpp - third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp - third_party/WebKit/Source/core/testing/Internals.cpp @yosin, you should review everything else :) https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:87: DoNotRemovePartiallyOverlappingMarker); It appears that the RemovePartiallyOverlappingMarker param is only ever set for spellcheck markers: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s... So we may be able to remove this param, at the cost of either re-implementing removeMarkers() in SpellcheckMarkerList, or hard-coding a check based on the marker type https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:89: int startOffset, I thought about switching all the instances of unsigned ints in DocumentMarker-related code over to signed ints (just because I think unsigned ints are generally a bad idea, they don't enforce that values passed in are non-negative, they just cause the values to overflow instead), but I ended up punting on that; this change got left in though, how do we feel about this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
There is a browser test case, FindInPageControllerTest.SpanAndListsSearchable, failing because it's violating our assumption that the text match markers should always be inserted in order. The test case constructs the following HTML (from FindRandomTests.html): <p style="font-size: 5em;">My mother has <span class="blue">light blue</span> eyes and my father has <span class="green">dark green</span> eyes.</p> and looks for the string "has light blue eyes and my father has dark". It finds the string, constructs an EphemeralRange, and then DocumentMarkerController uses a TextIterator to split the range across the nodes and add markers. The markers get added as follows: 10 to 14 on node #text "My mother has " 0 to 10 on node #text "light blue" 0 to 1 on node #text " eyes and my father has " 0 to 24 on node #text " eyes and my father has " The last one hits the DCHECK that checks the markers for a given node are inserted in order because 0 (the start position) is not greater than or equal to 1 (the end position of the previous marker). So for some reason the space immediately after the span element gets marked separately, and also gets marked as a part of the following text, and the markers end up overlapping. This seems somewhat problematic even without applying this CL as DocumentMarkerController is not currently set up to handle overlapping markers properly (TextMatch markers do not get merged on insert). But even if we're able to fix this, our assumption that TextMatch markers are only ever added in order might still turn out to be false since it looks like there's code that supports finding text in reverse: https://cs.chromium.org/chromium/src/chrome/browser/ui/find_bar/find_tab_help... Although if TextMatch markers are indeed always inserted in order, either forward or reverse, we may be able to add an invariant that the marker either belongs on the front or end of the list and use a deque to store them efficiently. @yosin, how do you think we should proceed?
xiaochengh@chromium.org changed reviewers: + xiaochengh@chromium.org
This is quite a big patch. As the changes outside editing/markers/ are just function renaming, how about splitting this patch as follows, so that the iteration can be more efficient? 1. Introduce the DocumentMarker and DocumentMarkerList hierarchy, and use them inside DMC. Do not change the declarations of DMC's public functions, so that there's no need to request review for anything outside editing/markers. 2. Rename/Replace/Whatever-you-like the public functions of DMC. This part will be technically light. https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:131: RenderedTextMatchMarker* asRenderedTextMatchMarker(); Please use the DEFINE_TYPE_CASTS macro to define type casts. Example: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c... https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:87: DoNotRemovePartiallyOverlappingMarker); On 2017/02/28 at 00:53:51, rlanday wrote: > It appears that the RemovePartiallyOverlappingMarker param is only ever set for spellcheck markers: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s... > > So we may be able to remove this param, at the cost of either re-implementing removeMarkers() in SpellcheckMarkerList, or hard-coding a check based on the marker type It's good if we can get rid of such a parameter. In fact, the removal of stale spelling markers (namely, SpellChecker::updateMarkersForWordsBlablabla) has been a performance bottleneck for a long time. If seems that we can get rid of that function, and simply maintain existing markers as a SynchronousMutationObserver... https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:89: int startOffset, On 2017/02/28 at 00:53:51, rlanday wrote: > I thought about switching all the instances of unsigned ints in DocumentMarker-related code over to signed ints (just because I think unsigned ints are generally a bad idea, they don't enforce that values passed in are non-negative, they just cause the values to overflow instead), but I ended up punting on that; this change got left in though, how do we feel about this? Let's discuss in crbug.com/696831 https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp (right): https://codereview.chromium.org/2723663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp:22: // Build a second vector and swap with m_markers to avoid O(n^2) performance Nice!
Ok, I can split up the patch. I've filed a bug for the overlapping marker issue, this seems like a problem that we need to address before we can proceed with this refactor. crbug.com/697654
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 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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: linux_chromium_chromeos_ozone_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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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.
I'm going to try to reduce the amount of code duplication a bit
https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:220: docDirty = docDirty || spellingIt->value->copyMarkers(startOffset, length, There's a bug here, I need to order these checks the other way, otherwise short circuit evaluation means we might not copy all the markers https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:522: didShiftMarker || nodeMarkers.value->shiftMarkers(startOffset, delta); Same bug here
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/03/17 at 01:40:15, rlanday wrote: > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:220: docDirty = docDirty || spellingIt->value->copyMarkers(startOffset, length, > There's a bug here, I need to order these checks the other way, otherwise short circuit evaluation means we might not copy all the markers > > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:522: didShiftMarker || nodeMarkers.value->shiftMarkers(startOffset, delta); > Same bug here
On 2017/03/17 at 03:18:42, rlanday wrote: > On 2017/03/17 at 01:40:15, rlanday wrote: > > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > > > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:220: docDirty = docDirty || spellingIt->value->copyMarkers(startOffset, length, > > There's a bug here, I need to order these checks the other way, otherwise short circuit evaluation means we might not copy all the markers > > > > https://codereview.chromium.org/2723663002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:522: didShiftMarker || nodeMarkers.value->shiftMarkers(startOffset, delta); > > Same bug here Oops, meant to say that I fixed these, not to re-post the same comment again...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
For some reason, some test cases that were previously passing started failing for me locally; one of them was because I was checking if an iterator != end in TextMatchMarkerList::removeMarkers() instead of using <, the other is because the method replaceTextInNodePreservingMarkers() is trying to add markers in a way not supported by TextMatchMarkerList (which only supports adding them at the end). I'm planning to remove that method anyway (in a follow-up CL to the one that refactors DocumentMarkerController on top of SynchronousMutationObserver), but I guess this CL will have to wait for that one.
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/03/17 at 21:25:35, rlanday wrote: > For some reason, some test cases that were previously passing started failing for me locally; one of them was because I was checking if an iterator != end in TextMatchMarkerList::removeMarkers() instead of using <, the other is because the method replaceTextInNodePreservingMarkers() is trying to add markers in a way not supported by TextMatchMarkerList (which only supports adding them at the end). I'm planning to remove that method anyway (in a follow-up CL to the one that refactors DocumentMarkerController on top of SynchronousMutationObserver), but I guess this CL will have to wait for that one. If you make this patch depending on the SMO patch, is this patch going to be cleaner?
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/03/17 at 21:58:16, xiaochengh wrote: > On 2017/03/17 at 21:25:35, rlanday wrote: > > For some reason, some test cases that were previously passing started failing for me locally; one of them was because I was checking if an iterator != end in TextMatchMarkerList::removeMarkers() instead of using <, the other is because the method replaceTextInNodePreservingMarkers() is trying to add markers in a way not supported by TextMatchMarkerList (which only supports adding them at the end). I'm planning to remove that method anyway (in a follow-up CL to the one that refactors DocumentMarkerController on top of SynchronousMutationObserver), but I guess this CL will have to wait for that one. > > If you make this patch depending on the SMO patch, is this patch going to be cleaner? Well, I actually did that already in Patch 8; I don't think it makes much difference (but I do think that patch is necessary to make this work properly)
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...
Haven't gone through the entire patch. I'll finish reading this patch and give more comments later. https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h (right): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:13: WTF_MAKE_NONCOPYABLE(CompositionMarkerList); I personally prefer DISALLOW_COPY_AND_ASSIGN since it's defined in base/ https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:131: RenderedTextMatchMarker* asRenderedTextMatchMarker(); Please use the DEFINE_TYPE_CASTS macro. https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:124: Vector<IntRect> renderedRectsForMarkers(DocumentMarker::MarkerType); Please do not change the signature of this function, so that we can focus on editing/markers in this patch. https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:35: I think we should store |m_markers| directly in DocumentMarkerList, so that the accessor functions don't need to be virtual. Unless you want different subclasses store their markers with different structures. https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:58: static bool startsAfter(const Member<DocumentMarker>&, size_t startOffset); This function is defined but not used. https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:59: static bool endsBefore(size_t startOffset, const Member<DocumentMarker>& rhv); If we store |m_markers| directly in DML, we can also hide |endsBefore| as a local function in DML.cpp, and expose a member function of DML to return |std::upper_bound(m_markers.begin(), m_markers.end(), ..., endsBefore)| https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:60: static bool compareByStart(const Member<DocumentMarker>& lhv, This should be a local function in EditingMarkerList.cpp. https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:63: ShiftMarkerResult getShiftedMarkerPosition(const DocumentMarker&, This should be a member function of DocumentMarker https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp (right): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp:22: // Build a second vector and swap with m_markers to avoid O(n^2) performance This is still quadratic running time: O(m_markers.size() * words.size()). Anyway, it doesn't seem to be a serious issue, and there's no easy way to get rid of this nested loop. I guess there's no need to overkill it with Aho-Corasick algorithm... https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (left): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:9: #include "core/editing/markers/RenderedDocumentMarker.h" Please remove this redundant include in a different patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... 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: > Please do not change the signature of this function, so that we can focus on editing/markers in this patch. I can do that, but then the method will just have to ignore the passed-in MarkerType because with the refactor, we're only creating the rendered rects for text match nodes https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:35: On 2017/03/17 at 23:56:33, Xiaocheng wrote: > I think we should store |m_markers| directly in DocumentMarkerList, so that the accessor functions don't need to be virtual. > > Unless you want different subclasses store their markers with different structures. I think one of the ideas behind the refactor was that we might potentially want to have different data structures, although it's not currently necessary, so we might just want to do what you're suggesting https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp (right): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp:22: // Build a second vector and swap with m_markers to avoid O(n^2) performance On 2017/03/17 at 23:56:34, Xiaocheng wrote: > This is still quadratic running time: O(m_markers.size() * words.size()). > > Anyway, it doesn't seem to be a serious issue, and there's no easy way to get rid of this nested loop. I guess there's no need to overkill it with Aho-Corasick algorithm... It's linear in m_markers.size() and words.size(), quadratic if you increase them together :) Taking the substring might also be slow if the markers contain a lot of text...I think we're at O(# markers * (avg. length of marked text + words.size())) One easy optimization for the asymptotic complexity would be storing the list of words in a hash set, I suspect the number of words is typically going to be small here though (I think this is called when you add a word to the custom dictionary) so I don't know if that would actually help or not.
https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... 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: > On 2017/03/17 at 23:56:33, Xiaocheng wrote: > > Please do not change the signature of this function, so that we can focus on editing/markers in this patch. > > I can do that, but then the method will just have to ignore the passed-in MarkerType because with the refactor, we're only creating the rendered rects for text match nodes That's exactly what I mean. Or we can first upload another patch that removes the pased-in MarkerType. In either way, this part should be orthogonal to the DMC refactoring. https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp (right): https://codereview.chromium.org/2723663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellingMarkerList.cpp:22: // Build a second vector and swap with m_markers to avoid O(n^2) performance On 2017/03/18 at 01:12:27, rlanday wrote: > On 2017/03/17 at 23:56:34, Xiaocheng wrote: > > This is still quadratic running time: O(m_markers.size() * words.size()). > > > > Anyway, it doesn't seem to be a serious issue, and there's no easy way to get rid of this nested loop. I guess there's no need to overkill it with Aho-Corasick algorithm... > > It's linear in m_markers.size() and words.size(), quadratic if you increase them together :) > > Taking the substring might also be slow if the markers contain a lot of text...I think we're at > > O(# markers * (avg. length of marked text + words.size())) > > One easy optimization for the asymptotic complexity would be storing the list of words in a hash set, I suspect the number of words is typically going to be small here though (I think this is called when you add a word to the custom dictionary) so I don't know if that would actually help or not. Using anything more complicated than a simple nested loop might be an overkill here, unless we have evidence this part is a performance bottleneck. However, we don't have any evidence. So forget it...
https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:91: new DocumentMarker(markedText.startOffsetInCurrentContainer(), These DocumentMarker ctors really look awkward... It will be much better if we have static functions like DM::createTextMatchMarker to replace DM::ctor(startOffset, endOffset, activeMatch) DM::createCompositionMarker to relace DM::ctor(startOffset, endOffset, underlineColor, thick, bgcolor) ... Anyway, this part is orthogonal. https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:136: static void updateTextMatchMarkerRenderedRect(const Node& node, Can we move this function to RenderedTextMatchMarker? https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:163: switch (newMarker->type()) { Could you try to reduce the code duplication? The first idea that came into my mind is: template <class DocumentMarkerList> void DMC::addIntoMarkerMap(DocumentMarkerMap<DocumentMarkerList>* markerMap, Node* node, DocumentMarker* newMarker) { ... } DMC::addMarker(node, newMarker) { switch (newMarker->type()) { case DocumentMarker::Spelling: addIntoMarkerMap(m_spelling, node, newMarker); break; case ... ... } } Or maybe you can find something better. https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:245: if (markerTypes.contains(DocumentMarker::Spelling)) { Please also try to reduce code duplication here. https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:671: auto spellingIt = m_spelling.find(node); De-duplication needed. https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:50: bool operator()(const DocumentMarker&, const Text&) const; This change seems irrelevant. There should be a subsequent patch removing MarkerRemoverPredicate entirely. https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:58: : public GarbageCollectedFinalized<DocumentMarkerController>, Is this relevant? https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:116: // |endOffset| are found Please elmininate all such irrelevant changes from your patch. https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:12: class TextMatchMarker : public DocumentMarker {}; This class doesn't seem necessary. Could you merge RenderedTextMatchMarker into TextMatchMarker?
https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:91: new DocumentMarker(markedText.startOffsetInCurrentContainer(), On 2017/03/18 at 06:43:14, Xiaocheng wrote: > These DocumentMarker ctors really look awkward... > > It will be much better if we have static functions like > > DM::createTextMatchMarker to replace DM::ctor(startOffset, endOffset, activeMatch) > DM::createCompositionMarker to relace DM::ctor(startOffset, endOffset, underlineColor, thick, bgcolor) > ... > > Anyway, this part is orthogonal. Yeah, let's fix this in the follow-up CL where we're making all the API changes https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:136: static void updateTextMatchMarkerRenderedRect(const Node& node, On 2017/03/18 at 06:43:14, Xiaocheng wrote: > Can we move this function to RenderedTextMatchMarker? Probably https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:163: switch (newMarker->type()) { On 2017/03/18 at 06:43:14, Xiaocheng wrote: > Could you try to reduce the code duplication? The first idea that came into my mind is: > > template <class DocumentMarkerList> > void DMC::addIntoMarkerMap(DocumentMarkerMap<DocumentMarkerList>* markerMap, Node* node, DocumentMarker* newMarker) { > ... > } > > DMC::addMarker(node, newMarker) { > switch (newMarker->type()) { > case DocumentMarker::Spelling: > addIntoMarkerMap(m_spelling, node, newMarker); > break; > case ... > ... > } > } > > Or maybe you can find something better. I'll see what I can come up with, I was running into trouble over the fact that the different marker lists are different types but I hadn't tried using a templated helper like this https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:50: bool operator()(const DocumentMarker&, const Text&) const; On 2017/03/18 at 06:43:14, Xiaocheng wrote: > This change seems irrelevant. There should be a subsequent patch removing MarkerRemoverPredicate entirely. Ok https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:58: : public GarbageCollectedFinalized<DocumentMarkerController>, On 2017/03/18 at 06:43:14, Xiaocheng wrote: > Is this relevant? Hmm I can't remember why I added this, it doesn't seem to be currently necessary https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:116: // |endOffset| are found On 2017/03/18 at 06:43:14, Xiaocheng wrote: > Please elmininate all such irrelevant changes from your patch. Oops, typo https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h (right): https://codereview.chromium.org/2723663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/TextMatchMarker.h:12: class TextMatchMarker : public DocumentMarker {}; On 2017/03/18 at 06:43:14, Xiaocheng wrote: > This class doesn't seem necessary. > > Could you merge RenderedTextMatchMarker into TextMatchMarker? Probably, let me try that
BTW, so glad y'all are working on this. This code needed love for so long.
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...
Description was changed from ========== Refactor DocumentMarkerController Based on this design doc: https://docs.google.com/document/d/1BORj8rxmLc52rdeGq-uazinmDO_sDVmIZa_SGliPo6w/ Notes: - The doc shows us storing the marker map as a map from a node to a list of marker lists for the node (one for each marker type). In this CL, I instead have a separate map for each type of marker. I haven't thought very hard about the tradeoffs but it seemed a little bit more straightforward to implement this way. - There is more code duplication in DocumentMarkerController than I would like. I would like to have a method that returns a list of all the MarkerMaps but I don't think there's any common type that would work (e.g. you can't cast a map from Node* to GrammarMarkerList* to a map from Node* to DocumentMarkerList* because then you'd be able to take the casted map and put a type of DocumentMarkerList in it that you shouldn't be able to. Also, it doesn't seem like good design to store the maps as maps from Node* to DocumentMarker* and use polymorphism because then we would need to use casting to call the marker type-specific methods. Maybe the solution is to switch to using a single map that maps to a list of marker lists as in the design doc? Then we could have a method that gives us all the DocumentMarkerLists for a given node and we could type them as DocumentMarkerList*. - 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 ========== to ========== Refactor DocumentMarkerController Based on this design doc: https://docs.google.com/document/d/1BORj8rxmLc52rdeGq-uazinmDO_sDVmIZa_SGliPo6w/ Notes: - The doc shows us storing the marker map as a map from a node to a list of marker lists for the node (one for each marker type). In this CL, I instead have a separate map for each type of marker. I haven't thought very hard about the tradeoffs but it seemed a little bit more straightforward to implement this way. - There is more code duplication in DocumentMarkerController than I would like. I would like to have a method that returns a list of all the MarkerMaps but I don't think there's any common type that would work (e.g. you can't cast a map from Node* to GrammarMarkerList* to a map from Node* to DocumentMarkerList* because then you'd be able to take the casted map and put a type of DocumentMarkerList in it that you shouldn't be able to). Also, it doesn't seem like good design to store the maps as maps from Node* to DocumentMarker* and use polymorphism because then we would need to use casting to call the marker type-specific methods. Maybe the solution is to switch to using a single map that maps to a list of marker lists as in the design doc? Then we could have a method that gives us all the DocumentMarkerLists for a given node and we could type them as DocumentMarkerList*. - 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Some minor comments after a quick glance https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:12: class CompositionMarkerList : public EditingMarkerList { Please add "final". The same comment applies to other classes. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:13: DISALLOW_COPY_AND_ASSIGN(CompositionMarkerList); nit: Basically all existing usages of DISALLOW_COPY_AND_ASSIGN are at the end of their classes. Please follow that. The same comment applies to other classes. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:33: #include "core/editing/markers/RenderedTextMatchMarker.h" This seems redundant. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: WTF_MAKE_NONCOPYABLE(DocumentMarkerList); nit: It's a little bit weird to me to have both WTF_MAKE_NONCOPYABLE and DISALLOW_COPY_AND_ASSIGN in the same patch. The same comment applies to other classes. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h:46: static bool compareByStart(const Member<DocumentMarker>& lhv, This function should be hidden in an anonymous namespace in EditingMarkerList.cpp https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/RenderedTextMatchMarker.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedTextMatchMarker.h:14: class RenderedTextMatchMarker : public DocumentMarker { How about just naming it |TextMatchMarker|? "Rendered" seems unnecessary. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2599: if (!m_tickmarks.isEmpty()) { This change is irrelevant.
https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:308: DocumentMarker::MarkerType) { I think it's better to: 1. Add a comment that this function is only called with TextMatch marker type, and follow up patches will remove the parameter, and 2. Move the implementation to TextMatchMarkerList. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:340: void DocumentMarkerController::invalidateRectsForMarkersInNode(Node& node) { Implementation of this function should also be moved to TextMatchMarkerList https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:354: void DocumentMarkerController::invalidateRectsForAllMarkers() { Ditto. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:52: private: Irrelevant change. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:149: void addIntoMarkerMap(MarkerMap<MarkerListType>* markerMap, Hmm... I forgot that templated member functions must be inlined. And the callers of these functions still look quite lengthy. A better idea seems to be storing the marker lists simply as |MarkerMap<DocumentMarkerList>|, and let polymorphism handle everything. Sketch: MarkerMap<DocumentMarkerList> m_spelling; MarkerMap<DocumentMarkerList> m_grammar; ... MarkerMap<DocumentMarkerList>& DMC::markerMapFor(type) { switch (type) { case DocumentMarker::Spelling: return m_spelling; case ... ... } NOTREACHED(); } DMC::addMarker(node, newMarker) { ... MarkerMap<DMList>& markerMap = markerMapFor(newMarker->type()); auto it = markerMap.find(...) ... it->value->insert(newMarker); } In this way we don't need to forward declare the subclasses of DocumentMarkerList, either. https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/EditingMarkerListThatMergesTouchingMarkers.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/EditingMarkerListThatMergesTouchingMarkers.h:12: class EditingMarkerListThatMergesTouchingMarkers : public EditingMarkerList { The two subclasses of this class basically share the same behavior. I don't see any need for them to have very different behaviors, either. So how about merging them into the same class: class SpellCheckMarkerList final : public EditingMarkerList { public: explicit SpellCheckMarkerList(DMC*, DM::MarkerType); DM::MarkerType allowedMarkerType() const final; // returns m_type; void insert(DM*) final; void removeMarkersForWords(...); private: DM::MarkerType m_type; }
https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (left): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:52: private: On 2017/03/21 at 19:12:11, Xiaocheng wrote: > Irrelevant change. This is necessary because I'm changing the implementation of removeMarkers(const MarkerRemoverPredicate&) to pass m_words to a method on SpellCheckMarkerList https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2723663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:149: void addIntoMarkerMap(MarkerMap<MarkerListType>* markerMap, On 2017/03/21 at 19:12:11, Xiaocheng wrote: > Hmm... I forgot that templated member functions must be inlined. And the callers of these functions still look quite lengthy. > > A better idea seems to be storing the marker lists simply as |MarkerMap<DocumentMarkerList>|, and let polymorphism handle everything. Sketch: > > MarkerMap<DocumentMarkerList> m_spelling; > MarkerMap<DocumentMarkerList> m_grammar; > ... > > MarkerMap<DocumentMarkerList>& DMC::markerMapFor(type) { > switch (type) { > case DocumentMarker::Spelling: > return m_spelling; > case ... > ... > } > NOTREACHED(); > } > > DMC::addMarker(node, newMarker) { > ... > MarkerMap<DMList>& markerMap = markerMapFor(newMarker->type()); > auto it = markerMap.find(...) > ... > it->value->insert(newMarker); > } > > In this way we don't need to forward declare the subclasses of DocumentMarkerList, either. We can try that, but then we'll have to cast the marker lists to call certain methods
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2723663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:218: static_cast<DocumentMarker::MarkerType>(1 << markerListIndex); Iterating over the MarkerTypes like this feels sloppy to me, I'm wondering if we should do something like adding a method on MarkerType that returns a list of all the types, or maybe making MarkerTypes allow iterating over the types it's holding
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... 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()| functions. Btw there is code duplication. Only the last lines (calling push_back or insert) depend on the marker type. https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:214: for (size_t markerListIndex = static_cast<DocumentMarker::MarkerType>(0); How about introducing iterators to DocumentMarker::MarkerTypes, so that we can write things like: for (DM::MarkerType type : markerTypes) { ... } Anyway, this is orthogonal. https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:309: static_cast<TextMatchMarkerList*>(nodeIterator->value.get()); Ditto. https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:331: static_cast<TextMatchMarkerList*>(m_textMatches.find(&node)->value.get()) Ditto. https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:387: static_cast<SpellCheckMarkerList*>(nodeMarkers.value.get()) Ditto. https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:458: docDirty = static_cast<TextMatchMarkerList*>(m_textMatches.at(node)) Ditto.
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: linux_chromium_chromeos_ozone_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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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.
Description was changed from ========== Refactor DocumentMarkerController Based on this design doc: https://docs.google.com/document/d/1BORj8rxmLc52rdeGq-uazinmDO_sDVmIZa_SGliPo6w/ Notes: - The doc shows us storing the marker map as a map from a node to a list of marker lists for the node (one for each marker type). In this CL, I instead have a separate map for each type of marker. I haven't thought very hard about the tradeoffs but it seemed a little bit more straightforward to implement this way. - There is more code duplication in DocumentMarkerController than I would like. I would like to have a method that returns a list of all the MarkerMaps but I don't think there's any common type that would work (e.g. you can't cast a map from Node* to GrammarMarkerList* to a map from Node* to DocumentMarkerList* because then you'd be able to take the casted map and put a type of DocumentMarkerList in it that you shouldn't be able to). Also, it doesn't seem like good design to store the maps as maps from Node* to DocumentMarker* and use polymorphism because then we would need to use casting to call the marker type-specific methods. Maybe the solution is to switch to using a single map that maps to a list of marker lists as in the design doc? Then we could have a method that gives us all the DocumentMarkerLists for a given node and we could type them as DocumentMarkerList*. - 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 ========== to ========== 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 ==========
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/03/21 at 22:42:20, xiaochengh wrote: > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... > 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()| functions. > > Btw there is code duplication. Only the last lines (calling push_back or insert) depend on the marker type. > > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:214: for (size_t markerListIndex = static_cast<DocumentMarker::MarkerType>(0); > How about introducing iterators to DocumentMarker::MarkerTypes, so that we can write things like: > > for (DM::MarkerType type : markerTypes) { > ... > } > > Anyway, this is orthogonal. > > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:309: static_cast<TextMatchMarkerList*>(nodeIterator->value.get()); > Ditto. > > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:331: static_cast<TextMatchMarkerList*>(m_textMatches.find(&node)->value.get()) > Ditto. > > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:387: static_cast<SpellCheckMarkerList*>(nodeMarkers.value.get()) > Ditto. > > https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:458: docDirty = static_cast<TextMatchMarkerList*>(m_textMatches.at(node)) > Ditto. Ok, I've done this and cleaned up the code duplication using my MarkerTypes iterator CL: https://codereview.chromium.org/2765013003
I suppose we should add test cases for all the different types of DocumentMarkerList now?
On 2017/03/23 at 01:29:58, rlanday wrote: > I suppose we should add test cases for all the different types of DocumentMarkerList now? Yes, please. To speed up the review iterations, I suggest introducing the list types one by one, each with its own tests; after that, another patch applies them in DMC.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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...
On 2017/03/23 at 02:22:39, xiaochengh wrote: > On 2017/03/23 at 01:29:58, rlanday wrote: > > I suppose we should add test cases for all the different types of DocumentMarkerList now? > > Yes, please. > > To speed up the review iterations, I suggest introducing the list types one by one, each with its own tests; after that, another patch applies them in DMC. Ok, I split off the DocumentMarkerList changes into these CLs (they can be applied in this order): DocumentMarkerList: https://codereview.chromium.org/2773343003 EditingMarkerList: https://codereview.chromium.org/2772423002 CompositionMarkerList: https://codereview.chromium.org/2773883003 SpellCheckMarkerList: https://codereview.chromium.org/2770413003 TextMatchMarkerList: https://codereview.chromium.org/2784753002 Note that this CL is also required to patch these against trunk: https://codereview.chromium.org/2755013004 Also, the change to remove the RenderedDocumentMarker.h include from SVGInlineTextBoxPainter.cpp has its own CL since it requires an LGTM from a paint owner (https://codereview.chromium.org/2763603003), I'm just not currently rebased on top of it. I see that Vector::remove() has been codemodded to Vector::erase() so I'll have to rebase everything anyway to get the trybots working again, so I will probably stick that CL somewhere in the stack.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) 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...) 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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 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-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/03/29 at 00:32:38, rlanday wrote: > On 2017/03/23 at 02:22:39, xiaochengh wrote: > > On 2017/03/23 at 01:29:58, rlanday wrote: > > > I suppose we should add test cases for all the different types of DocumentMarkerList now? > > > > Yes, please. > > > > To speed up the review iterations, I suggest introducing the list types one by one, each with its own tests; after that, another patch applies them in DMC. > > Ok, I split off the DocumentMarkerList changes into these CLs (they can be applied in this order): > > DocumentMarkerList: https://codereview.chromium.org/2773343003 > EditingMarkerList: https://codereview.chromium.org/2772423002 > CompositionMarkerList: https://codereview.chromium.org/2773883003 > SpellCheckMarkerList: https://codereview.chromium.org/2770413003 > TextMatchMarkerList: https://codereview.chromium.org/2784753002 > > Note that this CL is also required to patch these against trunk: > https://codereview.chromium.org/2755013004 > > Also, the change to remove the RenderedDocumentMarker.h include from SVGInlineTextBoxPainter.cpp has its own CL since it requires an LGTM from a paint owner (https://codereview.chromium.org/2763603003), I'm just not currently rebased on top of it. I see that Vector::remove() has been codemodded to Vector::erase() so I'll have to rebase everything anyway to get the trybots working again, so I will probably stick that CL somewhere in the stack. I've updated this again, hopefully the trybots will turn green and not red this time :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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-...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
Description was changed from ========== 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 ========== to ========== 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 ========== |