|
|
DescriptionAdd DocumentMarkerList in preparation for DocumentMarkerController refactor
Split off from https://codereview.chromium.org/2773883003 (Add CompositionMarkerList) since that's getting so big
DocumentMarkerList::copyMarkers() seems to count length in a funny way (I think
it's counting gaps between letters (i.e. positions where markers can start)
instead of the letters themselves), but I think it matches the existing behavior
in DocumentMarkerController.
BUG=707867
Patch Set 1 #Patch Set 2 : Add getShiftedMarkerPosition() test cases #
Total comments: 12
Patch Set 3 : Make requested changes (mostly rewriting tests and improving performance of DML methods) #Patch Set 4 : Rebase #
Total comments: 5
Patch Set 5 : Make requested changes #Patch Set 6 : Rebase (DocumentMarkerTest changes moving to previous CL) #
Total comments: 1
Patch Set 7 : Rebase, use clone() in copyMarkers() #Patch Set 8 : Rebase #Patch Set 9 : Make at() virtual #
Total comments: 2
Patch Set 10 : Remove "explicit" #
Total comments: 3
Patch Set 11 : Remove TODO on copyMarkers() #
Depends on Patchset: Messages
Total messages: 63 (42 generated)
Description was changed from ========== Add DocumentMarkerList in preparation for DocumentMarkerController refactor BUG= ========== to ========== Add DocumentMarkerList in preparation for DocumentMarkerController refactor Split off from https://codereview.chromium.org/2773883003 (Add CompositionMarkerList) since that's getting so big ==========
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
rlanday@chromium.org changed required reviewers: + yosin@chromium.org
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
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 ========== Add DocumentMarkerList in preparation for DocumentMarkerController refactor Split off from https://codereview.chromium.org/2773883003 (Add CompositionMarkerList) since that's getting so big ========== to ========== Add DocumentMarkerList in preparation for DocumentMarkerController refactor Split off from https://codereview.chromium.org/2773883003 (Add CompositionMarkerList) since that's getting so big DocumentMarkerList::copyMarkers() seems to count length in a funny way (I think it's counting gaps between letters (i.e. positions where markers can start) instead of the letters themselves), but I think it matches the existing behavior in DocumentMarkerController. ==========
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Almost good. Please break test cases in DMLTest.cpp further into independent ones. I forgot to mention that earlier... https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:38: enum DidCopyMarkerOrNot { DidNotCopyMarker, DidCopyMarker }; nit: use |enum class|. Ditto for the other two. https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: DidCopyMarkerOrNot copyMarkers(unsigned startOffset, These functions also have quadratic running time... Anyway, I don't prioritize it because we don't have performance bug reports. You can optimize them to linear if you like, as long as it doesn't make the code too hacky. Or putting a "TODO(editing-dev): Optimize its running time to linear." there is good enough for me. https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:52: RemoveMarkersSortedDoRemovePartiallyOverlapping) { Could you break it into independent test cases? https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:96: RemoveMarkersUnsortedDoRemovePartiallyOverlapping) { Could you break it into independent test cases? https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:140: RemoveMarkersSortedDontRemovePartiallyOverlapping) { Could you break it into independent test cases? https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:230: RemoveMarkersUnsortedDontRemovePartiallyOverlapping) { Could you break it into independent test cases? https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:322: EXPECT_EQ(false, m_markerList->shiftMarkers(10, 0, 5)); The result is an enum. https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:332: EXPECT_EQ(true, m_markerList->shiftMarkers(0, 0, 5)); The result is an enum. https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:355: m_markerList->copyMarkers(2, 4, dstList, -1); Please check return value.
https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: DidCopyMarkerOrNot copyMarkers(unsigned startOffset, On 2017/03/27 at 22:38:20, Xiaocheng wrote: > These functions also have quadratic running time... > > Anyway, I don't prioritize it because we don't have performance bug reports. > > You can optimize them to linear if you like, as long as it doesn't make the code too hacky. Or putting a "TODO(editing-dev): Optimize its running time to linear." there is good enough for me. - copyMarkers() appears to be O(n) to me when inserting into a list where add() runs in O(1) time, O(n^2) if inserting into a SpellCheckMarkerList where each insert takes O(n) time (to keep the list sorted). We could potentially optimize this to O(n log n) time by adding complexity here and in SpellCheckMarkerList, but I don't think it's worth it for now - removeMarkers() does appear to be O(n^2) for both the sorted and non-sorted cases and is already a mess, I might try to clean this up (by putting the markers into a second list and swapping) - shiftMarkers() also appears to be O(n^2) for both cases, I think this is easy to fix by creating a second list and swapping
https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:53: dstList->add(marker); This might have some side-effect... Now the same marker may present in multiple lists, which means an operation in one list may modify another list. Fortunately, currently the only caller of copyMarkers() is SplitTextNodeCommand, which removes the duplicated markers right after calling copyMarkers(). Still, this appears as a bad design to me. A clean design should be that each marker is stored exactly once in one marker list, which can be ensured by: 1. Put DocumentMarker and DocumentMarkerDetails off-heap; DM should store DMD with a std::unique_ptr, and DISALLOW_COPY_AND_ASSIGN 2. DML::m_markers should hold its markers with std::unique_ptr (maybe something like Vector<std::unique_ptr<DocumentMarker>>? I haven't tried yet...) 3. Which 2, DML can be made off-heap, which means DMC can also hold its lists via std::unique_ptr Anyway, this seems another big patch. It's fine to just have a TODO there.
https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: DidCopyMarkerOrNot copyMarkers(unsigned startOffset, > - copyMarkers() appears to be O(n) to me when inserting into a list where add() runs in O(1) time, O(n^2) if inserting into a SpellCheckMarkerList where each insert takes O(n) time (to keep the list sorted). We could potentially optimize this to O(n log n) time by adding complexity here and in SpellCheckMarkerList, but I don't think it's worth it for now Actually I think copyMarkers() is only ever going to be used to copy into a sorted list from another sorted list, so I think this is already going to be O(n)
On 2017/03/28 at 00:06:19, rlanday wrote: > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: DidCopyMarkerOrNot copyMarkers(unsigned startOffset, > > - copyMarkers() appears to be O(n) to me when inserting into a list where add() runs in O(1) time, O(n^2) if inserting into a SpellCheckMarkerList where each insert takes O(n) time (to keep the list sorted). We could potentially optimize this to O(n log n) time by adding complexity here and in SpellCheckMarkerList, but I don't think it's worth it for now > > Actually I think copyMarkers() is only ever going to be used to copy into a sorted list from another sorted list, so I think this is already going to be O(n) That is, if we optimize SpellCheckMarkerList to just stick the new marker on the end if possible instead of doing std::lower_bound to find the insertion point (which would bring us to O(n log n), so we should probably do that
On 2017/03/28 at 00:07:56, rlanday wrote: > On 2017/03/28 at 00:06:19, rlanday wrote: > > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > > > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: DidCopyMarkerOrNot copyMarkers(unsigned startOffset, > > > - copyMarkers() appears to be O(n) to me when inserting into a list where add() runs in O(1) time, O(n^2) if inserting into a SpellCheckMarkerList where each insert takes O(n) time (to keep the list sorted). We could potentially optimize this to O(n log n) time by adding complexity here and in SpellCheckMarkerList, but I don't think it's worth it for now > > > > Actually I think copyMarkers() is only ever going to be used to copy into a sorted list from another sorted list, so I think this is already going to be O(n) > > That is, if we optimize SpellCheckMarkerList to just stick the new marker on the end if possible instead of doing std::lower_bound to find the insertion point (which would bring us to O(n log n), so we should probably do that Nice catch. Please do that :)
On 2017/03/28 at 00:02:06, xiaochengh wrote: > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): > > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:53: dstList->add(marker); > This might have some side-effect... > > Now the same marker may present in multiple lists, which means an operation in one list may modify another list. > > Fortunately, currently the only caller of copyMarkers() is SplitTextNodeCommand, which removes the duplicated markers right after calling copyMarkers(). > > Still, this appears as a bad design to me. A clean design should be that each marker is stored exactly once in one marker list, which can be ensured by: > 1. Put DocumentMarker and DocumentMarkerDetails off-heap; DM should store DMD with a std::unique_ptr, and DISALLOW_COPY_AND_ASSIGN > 2. DML::m_markers should hold its markers with std::unique_ptr (maybe something like Vector<std::unique_ptr<DocumentMarker>>? I haven't tried yet...) > 3. Which 2, DML can be made off-heap, which means DMC can also hold its lists via std::unique_ptr > > Anyway, this seems another big patch. It's fine to just have a TODO there. Yes, I noticed that copyMarkers() actually changes the markers in the source list, which would be super sketchy if the source list weren't cleared/destroyed. I'll add a note
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rlanday@chromium.org 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_...)
xiaochengh, I optimized those methods in Patch 3, do they look OK now?
For the performance optimization, I was thinking about: - In a sorted list, we can identify the affected range and modify it with one operation - In an unsorted list, we can remove a marker by swapping it with the last one and then pop_back, and insert a marker by simply push_back And then I started to wonder if it's going to be too sophisticated. The new implementation (in PS4), we no longer gain any benefit from the sorted order. Besides, it's not a strict improvement. For example, we don't want to copy the entire list if the modification is just done to the last few markers. So I kind-of prefer the old implement (in PS2), since it better aligns with the existing implementation, which means at least we won't introduce any regression, given that there's no performance complaint to the existing implementation. Anyway, this is not a very important issue. Your new implementation is still reasonable and acceptable. The only issue is that the test cases should align with the implementation -- if there's no difference for sorted and unsorted lists in the implementation, it doesn't need to be tested at all. Btw, all the three functions can have a shortcut: when the list is sorted, we can easily check if anything needs to be modified, and early-return if not. https://codereview.chromium.org/2773343003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2773343003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:169: DocumentMarker::ShiftMarkerResult DocumentMarker::getShiftedMarkerPosition( nit: I think the previous patch should directly implement this function in DocumentMarker, so that we don't need to move it in this patch. It also makes it easy to compare the behavior change. https://codereview.chromium.org/2773343003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:34: // TODO(rlanday): this method leaves pointers to the same DocumentMarkers in the nit: Let's put this note in the header file. TODOs in .cpp files are easily forgotten eventually... https://codereview.chromium.org/2773343003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:105: nit: Remove the extra blank line. https://codereview.chromium.org/2773343003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:121: didShift = DidShiftMarkerOrNot::DidShiftMarker; nit: We prefer the early-continue style: if (result.shouldRemoveMarker) { didShift = ... continue; } ... https://codereview.chromium.org/2773343003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:135: nit: Remove the extra blank line.
On 2017/03/29 at 19:41:43, xiaochengh wrote: > For the performance optimization, I was thinking about: > - In a sorted list, we can identify the affected range and modify it with one operation > - In an unsorted list, we can remove a marker by swapping it with the last one and then pop_back, and insert a marker by simply push_back > And then I started to wonder if it's going to be too sophisticated. > > The new implementation (in PS4), we no longer gain any benefit from the sorted order. Besides, it's not a strict improvement. For example, we don't want to copy the entire list if the modification is just done to the last few markers. > > So I kind-of prefer the old implement (in PS2), since it better aligns with the existing implementation, which means at least we won't introduce any regression, given that there's no performance complaint to the existing implementation. I agree there's more optimization we can do. I think the implementation I have here is a good tradeoff between performance and complexity since we no longer have to have all the special cases based on whether the list is sorted or not. I'm not really worried it being much slower than the existing implementation since I think all the cases (except maybe some stuff like trying to call removeMarkers() on a sorted list after the last marker?) currently run in at least O(n) time. If performance turns out to be a concern here, we can make this more complicated again. I will update the tests to reflect the implementation.
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...
Updated
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...
https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:53: dstList->add(marker); Just a note that we should better change this to |dstList->add(marker->clone())| after the introduction of DM::clone() is landed.
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...
On 2017/03/30 at 02:13:04, xiaochengh wrote: > https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): > > https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:53: dstList->add(marker); > Just a note that we should better change this to |dstList->add(marker->clone())| after the introduction of DM::clone() is landed. Ok, I've made this change (I'm assuming we're going to land the clone() CL first)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rlanday@chromium.org 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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.
https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: explicit DocumentMarkerList(); I don't actually need to define a constructor in DocumentMarkerList and its subclasses now that it doesn't take any subclasses, right? I can just remove this from here and the subclasses?
https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: explicit DocumentMarkerList(); On 2017/03/31 at 02:44:04, rlanday wrote: > I don't actually need to define a constructor in DocumentMarkerList and its subclasses now that it doesn't take any subclasses, right? I can just remove this from here and the subclasses? Do you mean it doesn't take any parameter? In that case, yeah. Only SpellCheckMarkerList needs a non-default constructor.
On 2017/03/31 at 02:56:19, xiaochengh wrote: > https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: explicit DocumentMarkerList(); > On 2017/03/31 at 02:44:04, rlanday wrote: > > I don't actually need to define a constructor in DocumentMarkerList and its subclasses now that it doesn't take any subclasses, right? I can just remove this from here and the subclasses? > > Do you mean it doesn't take any parameter? > > In that case, yeah. Only SpellCheckMarkerList needs a > non-default constructor. Yeah that's what I meant. Actually I think since we do DISALLOW_COPY_AND_ASSIGN(), which disables the copy constructor, the default constructor is no longer implicitly created. So we still need to declare it, but I can remove the "explicit" keyword since it no longer takes an argument.
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/31 at 17:24:05, rlanday wrote: > On 2017/03/31 at 02:56:19, xiaochengh wrote: > > https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > > > https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: explicit DocumentMarkerList(); > > On 2017/03/31 at 02:44:04, rlanday wrote: > > > I don't actually need to define a constructor in DocumentMarkerList and its subclasses now that it doesn't take any subclasses, right? I can just remove this from here and the subclasses? > > > > Do you mean it doesn't take any parameter? > > > > In that case, yeah. Only SpellCheckMarkerList needs a > > non-default constructor. > > Yeah that's what I meant. Actually I think since we do DISALLOW_COPY_AND_ASSIGN(), which disables the copy constructor, the default constructor is no longer implicitly created. So we still need to declare it, but I can remove the "explicit" keyword since it no longer takes an argument. Removed explicit (I'm going to do this in the other CLs as well)
lgtm https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:38: DocumentMarkerList::DidCopyMarkerOrNot DocumentMarkerList::copyMarkers( Please add |const| to this function to indicate that it doesn't modify the existing markers. Btw, the current implementation modifies the existing markers, while this implementation doesn't. In fact, I like this implementation. Since we are rewriting everything, it should be OK to also fix some existing BS, because a lot of things are changed anyway. ...Or do we want to fix it first, since DM::clone is available due to some previous patch?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add DocumentMarkerList in preparation for DocumentMarkerController refactor Split off from https://codereview.chromium.org/2773883003 (Add CompositionMarkerList) since that's getting so big DocumentMarkerList::copyMarkers() seems to count length in a funny way (I think it's counting gaps between letters (i.e. positions where markers can start) instead of the letters themselves), but I think it matches the existing behavior in DocumentMarkerController. ========== to ========== Add DocumentMarkerList in preparation for DocumentMarkerController refactor Split off from https://codereview.chromium.org/2773883003 (Add CompositionMarkerList) since that's getting so big DocumentMarkerList::copyMarkers() seems to count length in a funny way (I think it's counting gaps between letters (i.e. positions where markers can start) instead of the letters themselves), but I think it matches the existing behavior in DocumentMarkerController. BUG=707867 ==========
https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:38: DocumentMarkerList::DidCopyMarkerOrNot DocumentMarkerList::copyMarkers( On 2017/03/31 at 18:59:18, Xiaocheng wrote: > Please add |const| to this function to indicate that it doesn't modify the existing markers. > > Btw, the current implementation modifies the existing markers, while this implementation doesn't. > > In fact, I like this implementation. Since we are rewriting everything, it should be OK to also fix some existing BS, because a lot of things are changed anyway. > > ...Or do we want to fix it first, since DM::clone is available due to some previous patch? Let's fix it here. I will add const and remove the TODO from the header.
https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:38: DocumentMarkerList::DidCopyMarkerOrNot DocumentMarkerList::copyMarkers( On 2017/04/03 at 20:34:43, rlanday wrote: > On 2017/03/31 at 18:59:18, Xiaocheng wrote: > > Please add |const| to this function to indicate that it doesn't modify the existing markers. > > > > Btw, the current implementation modifies the existing markers, while this implementation doesn't. > > > > In fact, I like this implementation. Since we are rewriting everything, it should be OK to also fix some existing BS, because a lot of things are changed anyway. > > > > ...Or do we want to fix it first, since DM::clone is available due to some previous patch? > > Let's fix it here. I will add const and remove the TODO from the header. We actually already have const here so I'll just remove the TODO :) |