|
|
DescriptionAdd CompositionMarkerList in preparation for DocumentMarkerController refactor
This CL depends on https://codereview.chromium.org/2772423002 (add EditingMarkerList)
BUG=707867
Patch Set 1 #Patch Set 2 : Fix code that doesn't compile...what am I doing... #
Total comments: 24
Patch Set 3 : Respond to comments #
Total comments: 6
Patch Set 4 : Make requested changes, refactor tests #
Total comments: 9
Patch Set 5 : Attempt to fix weird compilation error on android_arm64 #Patch Set 6 : Make requested changes (before splitting off DocumentMarkerList) #Patch Set 7 : Split off DocumentMarkerList and EditingMarkerList into separate CLs #Patch Set 8 : Rebase #
Total comments: 1
Patch Set 9 : Rebase (leaving allowedMarkerType()) as-is #Patch Set 10 : Rebase #Patch Set 11 : Make allowedMarkerType() private #Patch Set 12 : Rebase #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Patch Set 15 : Rebase #Patch Set 16 : Rebase #Patch Set 17 : Remove "explicit" #
Depends on Patchset: Messages
Total messages: 66 (51 generated)
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: + xiaochengh@chromium.org, yosin@chromium.org
rlanday@chromium.org changed required reviewers: + yosin@chromium.org
The questions around when to sort the marker list still need to be resolved; right now the list starts out as sorted, and marks itself as no longer being sorted if a markers get added out of order, and only becomes sorted again if the list is cleared. I added a sortMarkerList() method but right now it's never used...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_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...
I got halfway through modifying DocumentMarkerList::copyMarkers() to optimize the copying in the case where the marker is sorted, but I decided it would be kind of messy and might not be worth it, so the current implementation just assumes the list is not sorted.
https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerListTest.cpp (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerListTest.cpp:382: // delete middle of marker If we have a marker on some text and we delete part of the interior of the text, what should happen to the marker? E.g. we have: "word1 word2 word3 word4 word5" with a marker on "word2 word3 word4" and then delete "word3". The current behavior is that we end up with: "word1 word2 word4 word5" with one marker on "word2 " and one marker on " word4" The way this CL is currently implemented, it changes the behavior so we end up with a single marker on "word2 word4". For the purposes of implementing SuggestionSpan support, I don't think it matters, since if you delete a piece of text, we delete the SuggestionSpan markers that contain it. But I think not splitting the markers aligns better with the behavior on replacing text. E.g. if we replaced "word3" with "word6", we would have a single marker on "word2 word6 word4" instead of splitting the marker.
Haven't checked the unit tests. Will review them tomorrow. https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:165: // Other classes should not call following setters. Irrelevant change? https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:175: // Offset modifications are done by DocumentMarkerController. DocumentMarkerList also calls them. https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:26: size_t size() const; These getters should be inlined for efficiency. https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:28: virtual DocumentMarker* at(size_t index); I guess this function no longer needs to be virtual. In your reference patch (crrev.com/2723663002), there's no longer any use of TextMatchMarkerList::at(); https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:54: HeapVector<Member<DocumentMarker>>::iterator Is it just |iterator|? https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:61: static bool endsBefore(size_t startOffset, const Member<DocumentMarker>& rhv); This should be just a local function hidden in an anonymous namespace in DML.cpp. Is there any case where we really need a private static function? The only case I can think of is when the implementation of a class is split in multiple files, and more than one file needs this function. https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp:63: m_markers.remove(markerIndex); The handling of each marker in the removal range is basically identical with TextMatchMarker::removeMarkers Could you de-duplicate? https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp:72: if (startOffset > marker.startOffset()) { The case |startOffset > marker.startOffset() && endOffset < marker.endOffset()| doesn't seem well-handled. The two remaining pieces at its ends should be added back. https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp:100: bool EditingMarkerList::shiftMarkers(unsigned offset, This function is completely the same as TextMatchMarkerList::shiftMarkers. No need to make it virtual. https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h:37: void sortMarkerList(); Please do not introduce an unused function. Instead, add a TODO(rlanday): Find out the correct timings to sort the marker list.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:165: // Other classes should not call following setters. On 2017/03/24 at 03:57:47, Xiaocheng wrote: > Irrelevant change? oops, rebase mistake I think https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:175: // Offset modifications are done by DocumentMarkerController. On 2017/03/24 at 03:57:47, Xiaocheng wrote: > DocumentMarkerList also calls them. Yeah, I have the comment updated in my big patch for the whole refactor, I wasn't sure if I should change it here or not if we're not actually using the new DocumentMarkerLists yet https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:26: size_t size() const; On 2017/03/24 at 03:57:47, Xiaocheng wrote: > These getters should be inlined for efficiency. Ok https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:28: virtual DocumentMarker* at(size_t index); On 2017/03/24 at 03:57:47, Xiaocheng wrote: > I guess this function no longer needs to be virtual. In your reference patch (crrev.com/2723663002), there's no longer any use of TextMatchMarkerList::at(); Oh, good catch! https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:54: HeapVector<Member<DocumentMarker>>::iterator On 2017/03/24 at 03:57:47, Xiaocheng wrote: > Is it just |iterator|? Yep! https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:61: static bool endsBefore(size_t startOffset, const Member<DocumentMarker>& rhv); On 2017/03/24 at 03:57:47, Xiaocheng wrote: > This should be just a local function hidden in an anonymous namespace in DML.cpp. > > Is there any case where we really need a private static function? The only case I can think of is when the implementation of a class is split in multiple files, and more than one file needs this function. Ah ok, I wasn't sure what the rule was, I'll remember this https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp:63: m_markers.remove(markerIndex); On 2017/03/24 at 03:57:47, Xiaocheng wrote: > The handling of each marker in the removal range is basically identical with TextMatchMarker::removeMarkers > > Could you de-duplicate? Yeah, I think we can add a helper method that just takes an iterator range to limit the search to, that lets us optimize for the case where the lis tis sorted https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp:72: if (startOffset > marker.startOffset()) { On 2017/03/24 at 03:57:47, Xiaocheng wrote: > The case |startOffset > marker.startOffset() && endOffset < marker.endOffset()| doesn't seem well-handled. > > The two remaining pieces at its ends should be added back. I'm pretty sure this case is handled (it goes through both if statements) Search EditingMarkerListTest for "remove chunk from" for proof https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.cpp:100: bool EditingMarkerList::shiftMarkers(unsigned offset, On 2017/03/24 at 03:57:47, Xiaocheng wrote: > This function is completely the same as TextMatchMarkerList::shiftMarkers. No need to make it virtual. Oh...I think at one point, I thought I could optimize it when the marker list was sorted, and realized I couldn't (markers are sorted by start offset, and there's no way to guarantee that a shift won't affect a marker based only on the marker's start position), and then it became the same...I'll move it to DocumentMarkerList https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h:37: void sortMarkerList(); On 2017/03/24 at 03:57:47, Xiaocheng wrote: > Please do not introduce an unused function. > > Instead, add a TODO(rlanday): Find out the correct timings to sort the marker list. Ok
https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h:37: void sortMarkerList(); On 2017/03/24 at 17:45:49, rlanday wrote: > On 2017/03/24 at 03:57:47, Xiaocheng wrote: > > Please do not introduce an unused function. > > > > Instead, add a TODO(rlanday): Find out the correct timings to sort the marker list. > > Ok Oh, I know why I added this: I use it in SpellingMarkerList so we can merge overlapping markers in O(n log n) time instead of O(n^2) time...but that will be in another CL, so I'll leave it out of this one
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/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:175: // Offset modifications are done by DocumentMarkerController. On 2017/03/24 at 17:45:48, rlanday wrote: > On 2017/03/24 at 03:57:47, Xiaocheng wrote: > > DocumentMarkerList also calls them. > > Yeah, I have the comment updated in my big patch for the whole refactor, I wasn't sure if I should change it here or not if we're not actually using the new DocumentMarkerLists yet Please modify the comment in this patch. Otherwise, the use of these settings in DML looks confusing. https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:61: static bool endsBefore(size_t startOffset, const Member<DocumentMarker>& rhv); On 2017/03/24 at 17:45:48, rlanday wrote: > On 2017/03/24 at 03:57:47, Xiaocheng wrote: > > This should be just a local function hidden in an anonymous namespace in DML.cpp. > > > > Is there any case where we really need a private static function? The only case I can think of is when the implementation of a class is split in multiple files, and more than one file needs this function. > > Ah ok, I wasn't sure what the rule was, I'll remember this I'm not sure if there's a coding style rule about it. Anyway, it's good to keep header files small. https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:54: bool DocumentMarkerList::copyMarkers(unsigned startOffset, With a second thought, I think DML should not depend on DMC. So this function should take a |DML* dstList| parameter for receiving the copied markers, instead of a |Node*|. https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:91: for (; markerIndex < m_markers.size();) { nit: |while| seems better here. https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:172: return std::upper_bound(m_markers.begin(), m_markers.end(), startOffset, Should DCHECK(markerListIsSorted()); https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:32: iterator begin(); These four should also be inlined. https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h:24: virtual void insert(DocumentMarker*); It seems that no DML subclass allows insertion at arbitrary position. So it's better to have a virtual DML::push_back. It also looks better as the DML interface will be functionally complete. https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerListTest.cpp (right): https://codereview.chromium.org/2773883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerListTest.cpp:16: class EditingMarkerListTest : public ::testing::Test { With DML independent from DMC, the unit test can be greatly simplified. There's no need to involve any page, document, html, ... Besides, we should reorganize the test cases: 1. In DMLTest.cpp, define a dummy subclass of DML to test DML::shiftMarkers, DML::copyMarkers and DML::removeMarkers 2. In EditingMLTest.cpp, define a dummy subclass of EditingML to test the behaviors specific to EditingML: maintenance of |m_markersAreSorted| and DML::removeMarkers from a sorted list.
Ok, I'll make those changes. I'm also going to modify the test cases so the marker endpoints don't touch, so we can re-use them for SpellCheckMarkerList (which merges touching markers). I think sortMarkers() actually might not be necessary for SpellCheckMarkerList either, I think that list *always* keeps the list sorted. If it's not sorted on insert, it sorts it to merge markers with touching endpoints, and I don't think there's anywhere other than EditingMarkerList's implementation of insert() where the list can become unsorted.
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
Updated
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Finally I went through all the unit tests... Btw, the base class DocumentMarkerList itself contains quite a lot of details now. I suggest splitting this patch into two parts: 1. DML itself 2. EditingML and CompositionML (I bet yosin@ is going to ask you to split if you don't do that...) https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:170: ShiftMarkerResult getShiftedMarkerPosition(unsigned offset, I think we should add unit test for this function. Then unit tests for DML::shiftMarkers can be simplified: 1. Verify return value when nothing is modified 2. Verify return value and resulting list when a marker is modified 3. Verify return value and resulting list when a marker is removed https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:28: virtual void push_back(DocumentMarker*) = 0; Why not have a default implementation that simply calls |m_markers.push_back()|? https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:38: bool copyMarkers(unsigned startOffset, |bool| return values can be ambiguous and should be avoided (though they are still being used everywhere...). Please use an enum class for better clarity. https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:42: void removeMarkers(unsigned startOffset, No need to use |bool*| to return. Just return it directly. https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp (right): https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:12: class DocumentMarkerListForTesting : public DocumentMarkerList { This is not dummy enough. Let's have: class DMLForTesting : public DML { protected: bool markerListIsSorted() const override { return false; } private: DM::MarkerType allowedMarkerType() const final { ... } }; Use DMLForTesting for shiftMarkers(), copyMarkers() and removeMarkers() with unsorted list. And then: class SortedDMLForTesting final : public DMLForTesting { private: bool markerListIsSorted() const final { return true; } }; Use SortedDMLForTesting for removeMarkers() with sorted list. Actually, I just realized that EditingMarkerListTest only needs to verify the maintenance of |m_markersAreSorted|, which is exactly what you are doing :) https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp:494: // markers are now length 0 Though this matches the current behavior, it doesn't quite make sense... I would like to have a TODO for future investigation. https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerList.h:21: virtual void push_back(DocumentMarker*); void push_back(DM*) override/final? https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/EditingMarkerListTest.cpp (right): https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/EditingMarkerListTest.cpp:43: // The default implementation of push_back() for EditingMarkerList does not No need to check the content of the list, as EditingMarkerList doesn't introduce new behavior about that. Please have four test cases for the maintenance of the sorted state: 1. Newly constructed list is sorted 2. Inserting markers in sorted order keeps the list sorted 3. Inserting markers that are not in sorted order makes the list unsorted 4. Clearing the list makes it sorted
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/2773883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:28: virtual void push_back(DocumentMarker*) = 0; On 2017/03/25 at 00:05:35, Xiaocheng wrote: > Why not have a default implementation that simply calls |m_markers.push_back()|? Note: I'm going to change this to add() here per your comment on the SpellCheckMarkerList CL: https://codereview.chromium.org/2770413003/diff/20001/third_party/WebKit/Sour...
Description was changed from ========== Add CompositionMarkerList in preparation for DocumentMarkerController refactor This CL contains the code necessary to implement CompositionMarkerList from https://codereview.chromium.org/2723663002 (so it also contains DocumentMarker, DocumentMarkerList, and EditingMarkerList). I added a bunch of test cases in EditingMarkerListTest.cpp. They parameterized by type so when we add other subclasses of EditingMarkerList, they can share test code. Adding the tests caught at least two serious bugs in the version of EditingMarkerList in https://codereview.chromium.org/2723663002: - m_markersAreSorted was being initialized to false instead of true - removeMarkers() was sometimes skipping over markers because the loop index was being incremented when it shouldn't, and the code for splitting markers assumed the list was sorted EditingMarkerList::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. copyMarkers() is actually defined in DocumentMarkerList, not EditingMarkerList, so at some point I should add a second test file for code shared between all DocumentMarkerList subtypes, but this CL is already getting fairly large, so I haven't done that here. ========== to ========== Add CompositionMarkerList in preparation for DocumentMarkerController refactor This CL depends on https://codereview.chromium.org/2772423002 (add EditingMarkerList) ==========
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_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm https://codereview.chromium.org/2773883003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h (right): https://codereview.chromium.org/2773883003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerList.h:16: DocumentMarker::MarkerType allowedMarkerType() const final; nit: move it to |private|
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...
lgtm
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_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: This issue passed the CQ dry run.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add CompositionMarkerList in preparation for DocumentMarkerController refactor This CL depends on https://codereview.chromium.org/2772423002 (add EditingMarkerList) ========== to ========== Add CompositionMarkerList in preparation for DocumentMarkerController refactor This CL depends on https://codereview.chromium.org/2772423002 (add EditingMarkerList) BUG=707867 ========== |