|
|
DescriptionAdd DocumentMarkerList interface and GenericDocumentMarkerListImpl
Based on step 3 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7
This CL does the following:
- Introduces a DocumentMarkerList interface
- Introduces GenericDocumentMarkerListImpl, an implementation
of DocumentMarkerList that works for all marker types and is implemented on top
of DocumentMarkerListEditor
- Refactors DocumentMarkerController to use GenericDocumentMarkerListImpl instead
of using DocumentMarkerListEditor directly
BUG=707867
Review-Url: https://codereview.chromium.org/2820633002
Cr-Commit-Position: refs/heads/master@{#467549}
Committed: https://chromium.googlesource.com/chromium/src/+/c84b714f7a9730eeae4a4ccd84ac1582a6074926
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix build errors #Patch Set 4 : Rebase, use DocumentMarkerList* in some places instead of GDMLI #Patch Set 5 : Move DISALLOW_COPY_AND_ASSIGN to private #Patch Set 6 : Rebase #
Total comments: 1
Patch Set 7 : Change AppendMarkersToInputList() to GetMarkers() #
Total comments: 11
Patch Set 8 : Rebase #
Total comments: 4
Patch Set 9 : Add TODO #
Total comments: 17
Patch Set 10 : Respond to comments #
Total comments: 1
Patch Set 11 : Fix nit #Messages
Total messages: 65 (39 generated)
Description was changed from ========== Add DocumentMarkerList interface and GenericDocumentMarkerListImpl BUG=707867 ========== to ========== Add DocumentMarkerList interface and GenericDocumentMarkerListImpl Based on step 3 of yosin's plan: https://codereview.chromium.org/2812423002#msg7 This CL does the following: - Introduces a DocumentMarkerList interface - Introduces GenericDocumentMarkerListImpl, an implementation of DocumentMarkerList that works for all marker types and is implemented on top of DocumentMarkerListEditor - Refactors DocumentMarkerController to use GenericDocumentMarkerListImpl instead of using DocumentMarkerListEditor directly BUG=707867 ==========
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
I think the three bullet points in the description are basically independent, we could split this up into three CLs if it makes review easier
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...
rlanday@chromium.org changed reviewers: + yoichio@chromium.org
Rebased
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...
https://codereview.chromium.org/2820633002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:349: list->AppendMarkersToInputList(&result); I prefer to have list->GetMarkers() -> Vector<Member<DocumentMarker>> as more C++11-ish. It seems we have more usage patterns of AppendmarkkerToInputList() as DocumentMarkerVector markers_in_list; list->AppendMarkersToInputList(&markers_in_list); Some cases don't require copying vector. We could write: const Vector<Member<DocumentMarker>> markers = list->GetMarkers(); result.insert(result.end(), markers.begin(), markers.end());
On 2017/04/20 at 05:28:46, yosin wrote: > https://codereview.chromium.org/2820633002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2820633002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:349: list->AppendMarkersToInputList(&result); > I prefer to have list->GetMarkers() -> Vector<Member<DocumentMarker>> as more C++11-ish. Yeah, I agree this API is cleaner, but there are a couple call sites where we want to copy out the marker list for multiple types and append them all together, and I was thinking having the AppendMarkersToInputList() API would help us avoid some vector copies. Do we want to have both GetMarkers() AppendMarkersToInputList()? Or should we get rid of AppendMarkersToInputList() and not worry about the extra copying?
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...
https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:222: ListForType(markers, new_marker_type) = new GenericDocumentMarkerListImpl; Could you add a TODO of introducing a new function that decides which impl to create? In general, it's good to add a TODO to anything that's not in its final shape. https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:264: dst_markers->at(marker_list_index) = new GenericDocumentMarkerListImpl; ditto. https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:432: for (DocumentMarker* marker : markers_in_list) { No need to cast back and forth. nit: We can also write |for (DocumentMarker* marker : list->GetMarkers())| to save one line. https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:487: for (DocumentMarker* marker : markers_in_list) No need to cast back and forth. https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:524: DocumentMarkerList* list = markers[marker_list_index]; This impl doesn't look good -- dump everything out, remove, and dump remaining markers bad. We should have DML::RemoveMarkers(predicate) instead. Please add it to DMLEditor first (in-place, with appropriate TODOs) to fit the patch description that this patch does nothing but adding DML interface between DMC and DMLEditor. https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:592: ListForType(markers, type) = nullptr; For consistency, either apply ListForType() at all places, or none of them. https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:689: const auto start_pos = std::upper_bound( This impl assumes that the markers are sorted, which is a detail that should be maintained by DML but not DMC. Please add a TODO about it. https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:131: void AddMarker(Node*, DocumentMarker*); Why do we need to change this function? Can we change it in a different patch?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:524: DocumentMarkerList* list = markers[marker_list_index]; On 2017/04/20 at 08:41:34, Xiaocheng wrote: > This impl doesn't look good -- dump everything out, remove, and dump remaining markers bad. > > We should have DML::RemoveMarkers(predicate) instead. > > Please add it to DMLEditor first (in-place, with appropriate TODOs) to fit the patch description that this patch does nothing but adding DML interface between DMC and DMLEditor. Ah, I was going to clean this up when we introduce SpellCheckMarkerList (since that's really the only list type it's applicable to), but that plan works too https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:131: void AddMarker(Node*, DocumentMarker*); On 2017/04/20 at 08:41:34, Xiaocheng wrote: > Why do we need to change this function? > > Can we change it in a different patch? DocumentMarkerList::AddMarker() needs to take a non-const pointer so we can modify the DocumentMarker in ShiftMarkers(). The current implementation of AddMarker() copies the marker (by calling RenderedDocumentMarker::create() and storing that marker instead) but it is not guaranteed by the interface that the marker will be copied. So this method also needs to take a non-const pointer/ref; I changed it to a pointer because my understanding is that the style guide frowns upon passing non-const refs. We can change it in a separate patch if you want.
https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:524: DocumentMarkerList* list = markers[marker_list_index]; On 2017/04/20 at 10:03:18, rlanday wrote: > On 2017/04/20 at 08:41:34, Xiaocheng wrote: > > This impl doesn't look good -- dump everything out, remove, and dump remaining markers bad. > > > > We should have DML::RemoveMarkers(predicate) instead. > > > > Please add it to DMLEditor first (in-place, with appropriate TODOs) to fit the patch description that this patch does nothing but adding DML interface between DMC and DMLEditor. > > Ah, I was going to clean this up when we introduce SpellCheckMarkerList (since that's really the only list type it's applicable to), but that plan works too Could you rename DMC::RemoveMarkers() by usage pattern? Overloading of RemoveMarkers() makes us confusion. void RemoveMarkers( const EphemeralRange&, DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers()); Used only by spell checker, so RemoveMarkersForSpellChecker() void RemoveMarkers( Node*, unsigned start_offset, int length, DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers()); Used only by void DocumentMarkerController::RemoveMarkers(TextIterator& marked_text, DocumentMarker::MarkerTypes marker_types); RemoveMarkersInternal() and should be private void RemoveMarkers( DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers()); Input Method Controller: IMC::Clear(), SpellChecker, ToggleSpellCheckingEnabled, TextFinder RemoveAllMakers or ClearMarkers, we should MarkerTypes as required parameter. void RemoveMarkers( Node*, DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers()); Node::DidMoveToNewDocument() and RemoveSpellingAndGrammarMarkers() RemoveMarkesForNode() void RemoveMarkers(const MarkerRemoverPredicate& should_remove_marker); Used only in void SpellChecker::RemoveSpellingMarkersUnderWords( const Vector<String>& words) { MarkerRemoverPredicate remover_predicate(words); DocumentMarkerController& marker_controller = GetFrame().GetDocument()->Markers(); marker_controller.RemoveMarkers(remover_predicate); marker_controller.RepaintMarkers(); } It is better to implement this as DMC::RemoveSpellingMarkersUnderWords()
On 2017/04/21 at 01:32:25, yosin wrote: > Could you rename DMC::RemoveMarkers() by usage pattern? Overloading of RemoveMarkers() makes us confusion. > > void RemoveMarkers( > const EphemeralRange&, > DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers()); > > Used only by spell checker, so RemoveMarkersForSpellChecker() I'm pretty sure I'm also going to need this for Suggestion markers, so I'd prefer to call this something like RemoveMarkersInRange(). The other suggestions I agree with, I'll put up a separate CL for this.
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/2820633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:38: virtual bool RemoveMarkersUnderWords(const String& node_text, We don't need to have |RemoveMarkersUnderWords()| in DMList interface, since it is only for spelling marker. GenericDMListImpl or DMListEditor should have RemoveSpellingMarkersUnderWords(). Then we'll move it to SpellingMarkerListImpl.
On 2017/04/24 at 04:18:56, yosin wrote: > https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:38: virtual bool RemoveMarkersUnderWords(const String& node_text, > We don't need to have |RemoveMarkersUnderWords()| in DMList interface, > since it is only for spelling marker. > > GenericDMListImpl or DMListEditor should have RemoveSpellingMarkersUnderWords(). > Then we'll move it to SpellingMarkerListImpl. I don't like this idea: - DocumentMarkerListEditor can't implement this method to be called by DMC (it can implement it and have DMLEditor call it) because the marker list is a private member (and DMC only has access to the DML, not the private list) - If we add it to GenericDMList, DMC has to store the list pointers as GenericDocumentMarkerListImpl* instead of DocumentMarkerList*, which will break as soon as we start adding lists of different types (i.e. SpellingMarkerListImpl would have to be the first MarkerType-specific impl we add, which seems wrong to have as a requirement). So I think we should just have it on DocumentMarkerList for now and move it once we add SpellingMarkerListImpl.
lgtm https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:37: #include "core/editing/markers/DocumentMarkerListEditor.h" nit: I think we want to make DMLEditor only used by list impl classes, so this should be removed. https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:472: for (DocumentMarker* marker : markers_in_list) nit: No need to cast back and forth. https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:38: virtual bool RemoveMarkersUnderWords(const String& node_text, On 2017/04/24 at 04:18:56, yosin_UTC9 wrote: > We don't need to have |RemoveMarkersUnderWords()| in DMList interface, > since it is only for spelling marker. > > GenericDMListImpl or DMListEditor should have RemoveSpellingMarkersUnderWords(). > Then we'll move it to SpellingMarkerListImpl. In the design that we agreed, DMLEditor is for code sharing between different list impls, so there's no need to put a function in DMLEditor if only one impl uses it. I think the only question is whether we want to keep this function in the interface, or keep it only in SpellCheckListImpl and use it with downcast. I prefer the latter. Adding a TODO for moving this function from the DML interface to SpellCheckListImpl looks fine to me.
On 2017/04/24 at 15:13:33, xiaochengh wrote: > In the design that we agreed, DMLEditor is for code sharing between different list impls, so there's no need to put a function in DMLEditor if only one impl uses it. > > I think the only question is whether we want to keep this function in the interface, or keep it only in SpellCheckListImpl and use it with downcast. > > I prefer the latter. Adding a TODO for moving this function from the DML interface to SpellCheckListImpl looks fine to me. Ok, but then SpellCheckListImpl is forced to be the first MarkerType-specific list impl we add, because otherwise, we have to have this method on the DocumentMarkerList interface (since we haven't added SpellCheckListImpl), but we also have a list type that doesn't implement it. This plan is potentially problematic because if we come up with another method we only want on a different list impl, then we'll have more than one list impl we're required to introduce in the same CL, which will make review more difficult.
On 2017/04/24 at 18:16:07, rlanday wrote: > On 2017/04/24 at 15:13:33, xiaochengh wrote: > > In the design that we agreed, DMLEditor is for code sharing between different list impls, so there's no need to put a function in DMLEditor if only one impl uses it. > > > > I think the only question is whether we want to keep this function in the interface, or keep it only in SpellCheckListImpl and use it with downcast. > > > > I prefer the latter. Adding a TODO for moving this function from the DML interface to SpellCheckListImpl looks fine to me. > > Ok, but then SpellCheckListImpl is forced to be the first MarkerType-specific list impl we add, > because otherwise, we have to have this method on the DocumentMarkerList interface (since we > haven't added SpellCheckListImpl), but we also have a list type that doesn't implement it. > > This plan is potentially problematic because if we come up with another method we only want on > a different list impl, then we'll have more than one list impl we're required to introduce in > the same CL, which will make review more difficult. It looks OK to me to have an empty impl of this function in DML.cpp, so that other list impls don't need to have this function. In this way, it should be clearly marked with a TODO to move, so that everyone knows it's not the final shape.
On 2017/04/24 at 18:20:59, xiaochengh wrote: > It looks OK to me to have an empty impl of this function in DML.cpp, so that other list impls don't need to have this function. > > In this way, it should be clearly marked with a TODO to move, so that everyone knows it's not the final shape. Ok. I'll add the empty list impl when we add the first marker list impl that doesn't implement this method (it's not needed when we only have GenericMarkerListImpl).
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated with the TODO
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/24 at 18:24:36, rlanday wrote: > On 2017/04/24 at 18:20:59, xiaochengh wrote: > > It looks OK to me to have an empty impl of this function in DML.cpp, so that other list impls don't need to have this function. > > > > In this way, it should be clearly marked with a TODO to move, so that everyone knows it's not the final shape. > > Ok. I'll add the empty list impl when we add the first marker list impl that doesn't implement this method (it's not needed when > we only have GenericMarkerListImpl). I prefer adding the empty impl in this patch, so that we don't do anything with this function when introducing, say, TextMatchMarkerListImpl. Anyway, this is minor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:215: DocumentMarkerList* list = ListForType(markers, new_marker_type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:249: DocumentMarkerList* src_list = ListForType(src_markers, type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:290: DocumentMarkerList* list = ListForType(markers, type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:332: DocumentMarkerList* list = ListForType(markers, type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:336: result.AppendVector(list->GetMarkers()); Wow, AppendVector() is easy. https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:415: DocumentMarkerList* list = ListForType(markers, type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:507: DocumentMarkerList* list = ListForType(markers, type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:510: bool removed_markers = nit: s/bool/const bool/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:550: DocumentMarkerList* list = ListForType(markers, type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:599: DocumentMarkerList* list = ListForType(markers, type); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:648: DocumentMarkerList* list = ListForType(markers, DocumentMarker::kTextMatch); nit: s/DocumentMarkerList*/DocumentMarkerList* const/ https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: class CORE_EXPORT DocumentMarkerList Could you add short description of this interface to follow [1]? [1] http://dev.chromium.org/blink/coding-style#TOC-Documentation ... 2. Classes should have a class-level comment. ... https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:19: DocumentMarkerList(); ctor should have protected accessibility. Since we can't instantiate this class. https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:49: DISALLOW_COPY_AND_ASSIGN(DocumentMarkerList); DISALLOW_COPY_AND_ASSIGN(...) should have private accessibility. It seems PRESUBMIT doesn't check this for Blink directory. https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h (right): https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h:14: class GenericDocumentMarkerListImpl : public DocumentMarkerList { - Could you mark GenericDocumentMarkerListImpl as "final"? - Could you add short description of this interface to follow [1]? [1] http://dev.chromium.org/blink/coding-style#TOC-Documentation ... 2. Classes should have a class-level comment. ... https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h:18: bool IsEmpty() const final; Could you add comment like below to indicate following members implement DocumentMarkerList interface? // Implement DocumentMarkerList members (or whatever)
https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:336: result.AppendVector(list->GetMarkers()); On 2017/04/25 at 08:20:58, yosin_UTC9 wrote: > Wow, AppendVector() is easy. Except that as far as I'm aware, we haven't solved the problem of how to do this efficiently once some of the marker lists are storing pointers to DocumentMarker and TextMatchMarkerList is storing pointers to RenderedDocumentMarker (maybe we can make them all store DocumentMarker* and cast to RenderedDocumentMarker* in TextMatchMarkerList?).
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: This issue passed the CQ dry run.
lgtm w/ nit https://codereview.chromium.org/2820633002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2820633002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:23: protected: Please make sections in public, protected and private order. https://google.github.io/styleguide/cppguide.html#Class_Format
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2820633002/#ps200001 (title: "Fix nit")
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
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
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493248042698260, "parent_rev": "c106358057df12cec17cb43e6bb06314d47526fa", "commit_rev": "c84b714f7a9730eeae4a4ccd84ac1582a6074926"}
Message was sent while issue was closed.
Description was changed from ========== Add DocumentMarkerList interface and GenericDocumentMarkerListImpl Based on step 3 of yosin's plan: https://codereview.chromium.org/2812423002#msg7 This CL does the following: - Introduces a DocumentMarkerList interface - Introduces GenericDocumentMarkerListImpl, an implementation of DocumentMarkerList that works for all marker types and is implemented on top of DocumentMarkerListEditor - Refactors DocumentMarkerController to use GenericDocumentMarkerListImpl instead of using DocumentMarkerListEditor directly BUG=707867 ========== to ========== Add DocumentMarkerList interface and GenericDocumentMarkerListImpl Based on step 3 of yosin's plan: https://codereview.chromium.org/2812423002#msg7 This CL does the following: - Introduces a DocumentMarkerList interface - Introduces GenericDocumentMarkerListImpl, an implementation of DocumentMarkerList that works for all marker types and is implemented on top of DocumentMarkerListEditor - Refactors DocumentMarkerController to use GenericDocumentMarkerListImpl instead of using DocumentMarkerListEditor directly BUG=707867 Review-Url: https://codereview.chromium.org/2820633002 Cr-Commit-Position: refs/heads/master@{#467549} Committed: https://chromium.googlesource.com/chromium/src/+/c84b714f7a9730eeae4a4ccd84ac... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/c84b714f7a9730eeae4a4ccd84ac... |