Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1)

Issue 2812423002: [DMC #1] Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class (Closed)

Created:
3 years, 8 months ago by rlanday
Modified:
3 years, 8 months ago
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+101 lines, -46 lines) Patch
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h View 1 2 3 4 5 6 2 chunks +19 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp View 1 2 3 4 5 6 6 chunks +82 lines, -45 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 42 (16 generated)
rlanday
3 years, 8 months ago (2017-04-13 01:13:23 UTC) #2
rlanday
https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2812423002/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h#newcode42 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:42: #include "core/editing/markers/RenderedDocumentMarker.h" oops, I can remove this, I was ...
3 years, 8 months ago (2017-04-13 02:31:45 UTC) #3
Xiaocheng
I think this patch will be a perfect incremental change if the following two can ...
3 years, 8 months ago (2017-04-13 02:39:51 UTC) #4
yosin_UTC9
>The ideal cut is to do exactly one thing in a patch. >2. If possible, ...
3 years, 8 months ago (2017-04-13 05:53:57 UTC) #5
rlanday
On 2017/04/13 at 05:53:57, yosin wrote: > BTW, I think it should be done by ...
3 years, 8 months ago (2017-04-13 06:23:13 UTC) #6
yosin_UTC9
On 2017/04/13 at 06:23:13, rlanday wrote: > On 2017/04/13 at 05:53:57, yosin wrote: > > ...
3 years, 8 months ago (2017-04-13 06:43:11 UTC) #7
rlanday
On 2017/04/13 at 06:43:11, yosin wrote: > On 2017/04/13 at 06:23:13, rlanday wrote: > > ...
3 years, 8 months ago (2017-04-13 22:22:40 UTC) #9
Xiaocheng
https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode213 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:213: void DocumentMarkerListEditor::MergeOverlapping( Could you put it at the same ...
3 years, 8 months ago (2017-04-14 00:48:07 UTC) #12
rlanday
https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode414 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:414: doc_dirty = On 2017/04/14 at 00:48:06, Xiaocheng wrote: > ...
3 years, 8 months ago (2017-04-14 01:01:16 UTC) #13
Xiaocheng
On 2017/04/14 at 01:01:16, rlanday wrote: > https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode414 ...
3 years, 8 months ago (2017-04-14 01:06:14 UTC) #14
rlanday
On 2017/04/14 at 01:06:14, xiaochengh wrote: > On 2017/04/14 at 01:01:16, rlanday wrote: > > ...
3 years, 8 months ago (2017-04-14 01:16:29 UTC) #15
rlanday
Updated
3 years, 8 months ago (2017-04-14 01:20:49 UTC) #17
yosin_UTC9
Please try to make patch as small as we can. There are more rooms we ...
3 years, 8 months ago (2017-04-14 01:21:14 UTC) #18
rlanday
https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/40001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#oldcode805 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:805: for (MarkerList::iterator it = list->begin(); it != list->end(); ++it) ...
3 years, 8 months ago (2017-04-14 01:29:37 UTC) #20
Xiaocheng
https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode347 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:347: DocumentMarkerListEditor::MoveMarkers(src_list, length, The return value is still unused...
3 years, 8 months ago (2017-04-14 01:37:01 UTC) #21
rlanday
On 2017/04/14 at 01:37:01, xiaochengh wrote: > https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2812423002/diff/60001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode347 ...
3 years, 8 months ago (2017-04-14 02:34:38 UTC) #22
yosin_UTC9
https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#oldcode237 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:237: RenderedDocumentMarker* new_rendered_marker = Please implement DocumentMarkerListEditor::AddMarker() here, if so ...
3 years, 8 months ago (2017-04-14 04:14:22 UTC) #23
Xiaocheng
https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#oldcode237 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: ...
3 years, 8 months ago (2017-04-14 04:37:54 UTC) #24
Xiaocheng
lgtm https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/80001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode879 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:879: did_shift_marker |= DocumentMarkerListEditor::ShiftMarkers( nit: Please change it to ...
3 years, 8 months ago (2017-04-14 22:32:21 UTC) #25
rlanday
I investigated out to best split off the MoveMarkers() implementation change into a separate CL, ...
3 years, 8 months ago (2017-04-17 04:53:22 UTC) #26
Xiaocheng
Still lgtm. A suggestion about patch description: 1. It should be a clear and self-complete ...
3 years, 8 months ago (2017-04-17 06:49:29 UTC) #28
yosin_UTC9
https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode247 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:247: void DocumentMarkerListEditor::AddMarker(MarkerList* list, Note: Since we can't introduce this ...
3 years, 8 months ago (2017-04-17 06:59:01 UTC) #29
rlanday
On 2017/04/17 at 06:59:01, yosin wrote: > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2812423002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode247 ...
3 years, 8 months ago (2017-04-17 07:19:50 UTC) #32
yosin_UTC9
lgtm Thanks for making patch small!
3 years, 8 months ago (2017-04-17 09:06:39 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2812423002/120001
3 years, 8 months ago (2017-04-18 01:26:34 UTC) #39
commit-bot: I haz the power
3 years, 8 months ago (2017-04-18 01:51:39 UTC) #42
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b59bf346862b89daee2a4f06f02f...

Powered by Google App Engine
This is Rietveld 408576698