|
|
DescriptionRefactor DocumentMarkerController on top of new DocumentMarkerListEditor class
Based on yosin's comment on a previous version of this CL:
https://codereview.chromium.org/2812423002#msg7
This CL is step 1, "Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small". In this CL, I'm pulling out some of the
DocumentMarkerController implementation code into a DocumentMarkerListEditor class.
In a future CL, I will move this class into its own .h/.cpp files (I'm leaving it
in place for now to make the patch smaller).
BUG=707867
Review-Url: https://codereview.chromium.org/2812423002
Cr-Commit-Position: refs/heads/master@{#465092}
Committed: https://chromium.googlesource.com/chromium/src/+/b59bf346862b89daee2a4f06f02f5d7df59f9164
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add a DocumentMarkerListEditor class instead of DocumentMarkerList #Patch Set 3 : Add two more TODOs #
Total comments: 12
Patch Set 4 : Respond to xiaochengh's comments #
Total comments: 1
Patch Set 5 : Respond to comments #
Total comments: 3
Patch Set 6 : Remove MoveMarkers() changes, move Editor method calls below methods that call them #
Total comments: 5
Patch Set 7 : Respond to comments #
Dependent Patchsets: Messages
Total messages: 42 (16 generated)
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:42: #include "core/editing/markers/RenderedDocumentMarker.h" oops, I can remove this, I was implementing DocumentMarkerList in the header while I was working and I needed to temporarily add this
I think this patch will be a perfect incremental change if the following two can be addressed: 1. Don't change things related to RenderedDocumentMarker in this patch. Just let GenericDML hold a vector of RenderedDocumentMarker as what DMC currently does. RenderedDM is another (big) source of mess, which deserves dedicated patch(es). For this patch, we want to make it focus on encapsulating the marker maintenance logic into a new class. The ideal cut is to do exactly one thing in a patch. 2. If possible, avoid having big chunks of code removal and code introduction. In-place code change is much easier to review than deleting the old impl and adding new impl (with non-trivial modification) somewhere else. There are still some cases where big chunks are unavoidable. Still, we can put new code right next to old code to make it clear. For example, we can put DML::Add() right next to DMC::AddMarker(). That's still the idea of do one thing at a time: do non-trivial code change in one patch, and move them without any change in another. https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:171: static bool StartsFurther(const Member<RenderedDocumentMarker>& lhv, There's no need to touch these functions if we keep RenderedDocumentMarker in this patch. That's going to make the patch smaller. https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:259: void DocumentMarkerController::MergeOverlapping( DML::MergeOverlapping can be put here, so that it is clear that the old impl is not thrown away. https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:59: class DocumentMarkerList Let's use another name, say, GenericDocumentMarkerList, because we will introduce a DocumentMarkerList interface later. https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:78: HeapVector<Member<DocumentMarker>> markers_; RenderedDocumentMarker is another (big) source of mess. To make this patch small and focused on one thing (encapsulate the marker maintenance logic into a dedicated class), I think we should keep RenderedDocumentMarker in this patch, and clean it up later.
>The ideal cut is to do exactly one thing in a patch. >2. If possible, avoid having big chunks of code removal and code introduction. In-place code change is much easier to review than deleting the old impl and adding new impl (with non-trivial modification) somewhere else. We should code movement in two patch. 1. In-place change 2. Move This method assures the patch doesn't change the logic. BTW, I think it should be done by DMListEditor rather than DMList interface. https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:78: HeapVector<Member<DocumentMarker>> markers_; On 2017/04/13 at 02:39:51, Xiaocheng wrote: > RenderedDocumentMarker is another (big) source of mess. > > To make this patch small and focused on one thing (encapsulate the marker maintenance logic into a dedicated class), I think we should keep RenderedDocumentMarker in this patch, and clean it up later. I agree. We would like to have small and focused patch.
On 2017/04/13 at 05:53:57, yosin wrote: > BTW, I think it should be done by DMListEditor rather than DMList interface. I don't think there's a good reason to do this. DocumentMarkerListEditor is supposed to be for manipulating the various DocumentMarkerList implementation classes, which DocumentMarkerController is going to be built on top of. So if we're trying to keep the patches small while avoiding rewriting DocumentMarkerController more times than necessary, the most logical path seems to be closer to what I have here than to introducing some intermediate state where DocumentMarkerController is built on top of something that's not intended to support it directly (DMListEditor), and where we're just going to come through and rewrite DocumentMarkerController again. I still think the easier path is to rewrite all the DocumentMarkerLists (or at least writing a GenericDocumentMarkerList that works for all the marker types) and moving DocumentMarkerController over in one step. I agree that this patch is not straightforward to review, but I think that's because the existing code is a mess and we're maintaining too much of it here, rather than because we're not changing things in small enough pieces.
On 2017/04/13 at 06:23:13, rlanday wrote: > On 2017/04/13 at 05:53:57, yosin wrote: > > BTW, I think it should be done by DMListEditor rather than DMList interface. > > I don't think there's a good reason to do this. DocumentMarkerListEditor is supposed to be for manipulating the various DocumentMarkerList implementation classes, which DocumentMarkerController is going to be built on top of. So if we're trying to keep the patches small while avoiding rewriting DocumentMarkerController more times than necessary, the most logical path seems to be closer to what I have here than to introducing some intermediate state where DocumentMarkerController is built on top of something that's not intended to support it directly (DMListEditor), and where we're just going to come through and rewrite DocumentMarkerController again. > > I still think the easier path is to rewrite all the DocumentMarkerLists (or at least writing a GenericDocumentMarkerList that works for all the marker types) and moving DocumentMarkerController over in one step. I agree that this patch is not straightforward to review, but I think that's because the existing code is a mess and we're maintaining too much of it here, rather than because we're not changing things in small enough pieces. Sorry, we are living with DMC mess. DMCListEditor and GenericDMList are transition thing. They will be removed once we implement all DMListImpl. The patches can be: 1. Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small 2. Move DMLIstEditor impl to DMListEditor.cpp; removal and copy; but this is just copy and paste. 3. Introduce DMList interface with GenericDMList DMList.h contains members defined in DMListEditor; it is straight forward GenericDMListImpl.h = members in DMList.h GenericDMListImpl.cpp = interface members are just call DMListEditor; strait forward 4. Introduce XXXListImpl XXXListImpl.h is similar to GenericDMListImpl.h XXXListImpl.cpp is specific implementation. We need to have careful review DMC.cpp: Check marker type and create specific list impl 5..N Introudce YYYListImpl for other than XXX N+1 There are no more DMListEditor and GenerciDMListImpl. We can remove them. Note: If we can share implementations in specialized list impl we can keep DMListEditor Again, using DMListEditor and GenericDMListImpl is just temporary, will be removed for living with mess of DMC.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/04/13 at 06:43:11, yosin wrote: > On 2017/04/13 at 06:23:13, rlanday wrote: > > On 2017/04/13 at 05:53:57, yosin wrote: > > > BTW, I think it should be done by DMListEditor rather than DMList interface. > > > > I don't think there's a good reason to do this. DocumentMarkerListEditor is supposed to be for manipulating the various DocumentMarkerList implementation classes, which DocumentMarkerController is going to be built on top of. So if we're trying to keep the patches small while avoiding rewriting DocumentMarkerController more times than necessary, the most logical path seems to be closer to what I have here than to introducing some intermediate state where DocumentMarkerController is built on top of something that's not intended to support it directly (DMListEditor), and where we're just going to come through and rewrite DocumentMarkerController again. > > > > I still think the easier path is to rewrite all the DocumentMarkerLists (or at least writing a GenericDocumentMarkerList that works for all the marker types) and moving DocumentMarkerController over in one step. I agree that this patch is not straightforward to review, but I think that's because the existing code is a mess and we're maintaining too much of it here, rather than because we're not changing things in small enough pieces. > > Sorry, we are living with DMC mess. > DMCListEditor and GenericDMList are transition thing. They will be removed once we implement all DMListImpl. > The patches can be: > > 1. Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small > 2. Move DMLIstEditor impl to DMListEditor.cpp; removal and copy; but this is just copy and paste. > 3. Introduce DMList interface with GenericDMList > DMList.h contains members defined in DMListEditor; it is straight forward > GenericDMListImpl.h = members in DMList.h > GenericDMListImpl.cpp = interface members are just call DMListEditor; strait forward > 4. Introduce XXXListImpl > XXXListImpl.h is similar to GenericDMListImpl.h > XXXListImpl.cpp is specific implementation. We need to have careful review > DMC.cpp: Check marker type and create specific list impl > 5..N Introudce YYYListImpl for other than XXX > N+1 There are no more DMListEditor and GenerciDMListImpl. We can remove them. > Note: If we can share implementations in specialized list impl we can keep DMListEditor > > Again, using DMListEditor and GenericDMListImpl is just temporary, will be removed for living with mess of DMC. Ok. I've updated this CL to be step one of your plan (as I understand it). How does it look now?
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 on top of new DocumentMarkerList class Trying another approach to split up this refactor based on yosin's comment here: https://codereview.chromium.org/2805553003/#msg28 In this CL, I'm creating a DocumentMarkerList class to abstract out some of DocumentMarkerController's functionality. In later CLs, I will do the following (perhaps not in this order): - Move DocumentMarkerList into its own file - Move some of the DocumentMarkerList method implementations into a DocumentMarkerListEditor class - Rename DocumentMarkerList to GenericDocumentMarkerList, add subclasses for the different marker types - Do further cleanup on DocumentMarkerController/DocumentMarker - Eliminate RenderedDocumentMarker, create a polymorphic class hierarchy for DocumentMarker BUG=707867 ========== to ========== Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class Based on yosin's comment on a previous version of this CL: https://codereview.chromium.org/2812423002#msg7 This CL is step 1, "Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small" BUG=707867 ==========
https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:213: void DocumentMarkerListEditor::MergeOverlapping( Could you put it at the same place as DMC::MergeOverlapping to ease the comparison? https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:276: RenderedDocumentMarker* new_rendered_marker = No need to create RDM here. It's already done in DMLEditor::AddMarker() https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:331: Member<MarkerLists>& dst_markers = Could you make the code less hacky? if (!markers_.Contains(dst_node)) markers.insert(dst_node, new MarkerLists(DM::kMarkerTypeIndexesCount)); MarkerLists* dst_markers = markers_.at(dst_node); https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:349: DocumentMarkerListEditor::MoveMarkers(src_list, length, doc_dirty |= DMLEditor::MoveMarkers(...) https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:414: doc_dirty = nit: We can write doc_dirty |= DMLEditor::RemoveMarkers() to save a line. https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:721: for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) { Irrelevant change. Btw, marker type iterators haven't been applied anywhere. I think there should be a dedicated patch applying these iterators in existing code. https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:877: did_shift_marker = DocumentMarkerListEditor::ShiftMarkers( nit: We can write doc_dirty |= DMLEditor::ShiftMarkers() to save a line.
https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:414: doc_dirty = On 2017/04/14 at 00:48:06, Xiaocheng wrote: > nit: We can write doc_dirty |= DMLEditor::RemoveMarkers() to save a line. I don't think so, I think that would be the same as doc_dirty = doc_dirty | DMLEditor::RemoveMarkers() which would not do the correct thing because of short-circuit evaluation (it wouldn't call RemoveMarkers() if doc_dirty were already true)
On 2017/04/14 at 01:01:16, rlanday wrote: > https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:414: doc_dirty = > On 2017/04/14 at 00:48:06, Xiaocheng wrote: > > nit: We can write doc_dirty |= DMLEditor::RemoveMarkers() to save a line. > > I don't think so, I think that would be the same as > > doc_dirty = doc_dirty | DMLEditor::RemoveMarkers() > > which would not do the correct thing because of short-circuit evaluation (it wouldn't call RemoveMarkers() if doc_dirty were already true) I would be surprised if a bit operation is executed in that way.
On 2017/04/14 at 01:06:14, xiaochengh wrote: > On 2017/04/14 at 01:01:16, rlanday wrote: > > https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > > > https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:414: doc_dirty = > > On 2017/04/14 at 00:48:06, Xiaocheng wrote: > > > nit: We can write doc_dirty |= DMLEditor::RemoveMarkers() to save a line. > > > > I don't think so, I think that would be the same as > > > > doc_dirty = doc_dirty | DMLEditor::RemoveMarkers() > > > > which would not do the correct thing because of short-circuit evaluation (it wouldn't call RemoveMarkers() if doc_dirty were already true) > > I would be surprised if a bit operation is executed in that way. Ah, you're correct: http://stackoverflow.com/questions/23107162/do-the-and-operators-for-bool-sho...
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated
Please try to make patch as small as we can. There are more rooms we can make patch small. Sorry for puzzle, but it is worth to try. https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:805: for (MarkerList::iterator it = list->begin(); it != list->end(); ++it) { Please put |DMListEditor::ShiftMarkers()| here. https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:213: void DocumentMarkerListEditor::MergeOverlapping( On 2017/04/14 at 00:48:06, Xiaocheng wrote: > Could you put it at the same place as DMC::MergeOverlapping to ease the comparison? Please to follow xiaochengh@'s comment. The purpose of introducing |DMListEditor| for ease of comparison in review. https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:414: doc_dirty = On 2017/04/14 at 01:01:16, rlanday wrote: > On 2017/04/14 at 00:48:06, Xiaocheng wrote: > > nit: We can write doc_dirty |= DMLEditor::RemoveMarkers() to save a line. > > I don't think so, I think that would be the same as > > doc_dirty = doc_dirty | DMLEditor::RemoveMarkers() > > which would not do the correct thing because of short-circuit evaluation (it wouldn't call RemoveMarkers() if doc_dirty were already true) foo |= bar() always call |bar()| regard less value of |foo|. I recommend to write: if (DocumentMarkerListEditor::RemoveMarkers(list, start_offset, length)) doc_dirty = true; This is easier to read.
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/2812423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:805: for (MarkerList::iterator it = list->begin(); it != list->end(); ++it) { On 2017/04/14 at 01:21:14, yosin_UTC9 wrote: > Please put |DMListEditor::ShiftMarkers()| here. Sorry, I don't understand what you're asking here, the implementation for ShiftMarkers() is immediately above this one
https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:347: DocumentMarkerListEditor::MoveMarkers(src_list, length, The return value is still unused...
On 2017/04/14 at 01:37:01, xiaochengh wrote: > https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:347: DocumentMarkerListEditor::MoveMarkers(src_list, length, > The return value is still unused... Oops, fixed! Updated
https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:237: RenderedDocumentMarker* new_rendered_marker = Please implement DocumentMarkerListEditor::AddMarker() here, if so patch can be: + DocumentMarkerListEditor::AddMarker(list, &new_marker); + } + + DocumentMarkerListEditor::AddMarker(MarkerList* list, const DocumentMarker* marker) {
https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:237: RenderedDocumentMarker* new_rendered_marker = On 2017/04/14 at 04:14:22, yosin_UTC9 wrote: > Please implement DocumentMarkerListEditor::AddMarker() here, if so patch can be: > > + DocumentMarkerListEditor::AddMarker(list, &new_marker); > + } > + > + DocumentMarkerListEditor::AddMarker(MarkerList* list, > const DocumentMarker* marker) { There are some other code in DMC::AddMarker after the call site of DMLEditor::AddMarker, so the code can't look like that...
lgtm https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:879: did_shift_marker |= DocumentMarkerListEditor::ShiftMarkers( nit: Please change it to if (DMLE::ShitMarkers()) did_shift_marker = true; to unify the coding style.
I investigated out to best split off the MoveMarkers() implementation change into a separate CL, and decided that, since that implementation change needs the DocumentMarkerListEditor method for adding a marker to a MarkerList, we should do the following: - Land this CL without DocumentMarkerListEditor::MoveMarkers() - Land a second CL to do the implementation change for MoveMarkers() - Land a third CL to add DocumentMarkerListEditor::MoveMarkers() I have updated this CL so it should be ready for review.
rlanday@chromium.org changed reviewers: + yoichio@chromium.org
Still lgtm. A suggestion about patch description: 1. It should be a clear and self-complete summary of what this patch does. Quoting a sentence from a comment in code review is not very helpful, as a git log reader can hardly get the context of the quoted sentence. 2. When a reference to the full design is needed, a design doc is much better than a comment in code review. 3. There is no need to state where the patch idea comes from -- most git log readers are not interested in it. If any reader is interested, s/he will read the review comments, design doc, crbug, ... In one sentence, it should be some concise and formal-looking sentences describing what the patch does. https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:733: nit: remove this extra blank line
https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:247: void DocumentMarkerListEditor::AddMarker(MarkerList* list, Note: Since we can't introduce this function in middle of DMC::AddMarker(), This code is the result of copy-and-paste. We'll do re-factoring in another CL. https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:388: bool DocumentMarkerListEditor::RemoveMarkers(MarkerList* list, Note: Since we can't introduce this function in middle of DMC::RemoveMarker(), This code is the result of copy-and-paste. We'll do re-factoring in another CL. https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:733: nit: We don't need to have this extra blank line. https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:63: static bool ShiftMarkers(MarkerList*, nit: Please add a comment for |bool| return value.
Description was changed from ========== Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class Based on yosin's comment on a previous version of this CL: https://codereview.chromium.org/2812423002#msg7 This CL is step 1, "Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small" BUG=707867 ========== to ========== Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class Based on yosin's comment on a previous version of this CL: https://codereview.chromium.org/2812423002#msg7 This CL is step 1, "Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small". In this CL, I'm pulling out some of the DocumentMarkerController implementation code into a DocumentMarkerListEditor class. In a future CL, I will move this class into its own .h/.cpp files (I'm leaving it in place for now to make the patch smaller). BUG=707867 ==========
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/04/17 at 06:59:01, yosin wrote: > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:247: void DocumentMarkerListEditor::AddMarker(MarkerList* list, > Note: Since we can't introduce this function in middle of DMC::AddMarker(), > This code is the result of copy-and-paste. We'll do re-factoring in another CL. > > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:388: bool DocumentMarkerListEditor::RemoveMarkers(MarkerList* list, > Note: Since we can't introduce this function in middle of DMC::RemoveMarker(), > This code is the result of copy-and-paste. We'll do re-factoring in another CL. > > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:733: > nit: We don't need to have this extra blank line. > > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): > > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:63: static bool ShiftMarkers(MarkerList*, > nit: Please add a comment for |bool| return value. 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 Thanks for making patch small!
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 Link to the patchset: https://codereview.chromium.org/2812423002/#ps120001 (title: "Respond to comments")
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": 120001, "attempt_start_ts": 1492478756400540, "parent_rev": "b6b0a4c62db4757a27a61f19921ed70df20a42be", "commit_rev": "b59bf346862b89daee2a4f06f02f5d7df59f9164"}
Message was sent while issue was closed.
Description was changed from ========== Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class Based on yosin's comment on a previous version of this CL: https://codereview.chromium.org/2812423002#msg7 This CL is step 1, "Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small". In this CL, I'm pulling out some of the DocumentMarkerController implementation code into a DocumentMarkerListEditor class. In a future CL, I will move this class into its own .h/.cpp files (I'm leaving it in place for now to make the patch smaller). BUG=707867 ========== to ========== Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class Based on yosin's comment on a previous version of this CL: https://codereview.chromium.org/2812423002#msg7 This CL is step 1, "Introduce DMListEditor with in-place changes of DMC: Except for DMListEditor.h, changes are small". In this CL, I'm pulling out some of the DocumentMarkerController implementation code into a DocumentMarkerListEditor class. In a future CL, I will move this class into its own .h/.cpp files (I'm leaving it in place for now to make the patch smaller). BUG=707867 Review-Url: https://codereview.chromium.org/2812423002 Cr-Commit-Position: refs/heads/master@{#465092} Committed: https://chromium.googlesource.com/chromium/src/+/b59bf346862b89daee2a4f06f02f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b59bf346862b89daee2a4f06f02f... |