Chromium Code Reviews
Help | Chromium Project | Sign in
(6)

Issue 2820633002: [DMC #2] Add DocumentMarkerList interface and GenericDocumentMarkerListImpl (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 1 day ago by rlanday
Modified:
2 days, 13 hours ago
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+267 lines, -67 lines) Patch
M third_party/WebKit/Source/core/editing/BUILD.gn View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h View 1 2 3 4 5 6 7 2 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp View 1 2 3 4 5 6 7 8 9 16 chunks +73 lines, -59 lines 0 comments Download
A third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +62 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp View 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
A third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.h View 1 2 3 4 5 6 7 8 9 1 chunk +47 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp View 1 2 3 4 5 6 7 1 chunk +59 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 65 (39 generated)
rlanday
2 weeks, 1 day ago (2017-04-14 02:48:21 UTC) #3
rlanday
I think the three bullet points in the description are basically independent, we could split ...
2 weeks, 1 day ago (2017-04-14 02:52:02 UTC) #4
rlanday
Rebased
1 week, 4 days ago (2017-04-18 04:41:29 UTC) #12
yosin_OOO_til_May_7
https://codereview.chromium.org/2820633002/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/2820633002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode349 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:349: list->AppendMarkersToInputList(&result); I prefer to have list->GetMarkers() -> Vector<Member<DocumentMarker>> as ...
1 week, 2 days ago (2017-04-20 05:28:46 UTC) #19
rlanday
On 2017/04/20 at 05:28:46, yosin wrote: > https://codereview.chromium.org/2820633002/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/2820633002/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode349 ...
1 week, 2 days ago (2017-04-20 05:33:27 UTC) #20
rlanday
Updated
1 week, 2 days ago (2017-04-20 07:53:53 UTC) #24
Xiaocheng
https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode222 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:222: ListForType(markers, new_marker_type) = new GenericDocumentMarkerListImpl; Could you add a ...
1 week, 2 days ago (2017-04-20 08:41:34 UTC) #26
rlanday
https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode524 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 ...
1 week, 2 days ago (2017-04-20 10:03:19 UTC) #29
yosin_OOO_til_May_7
https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/120001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode524 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 ...
1 week, 1 day ago (2017-04-21 01:32:25 UTC) #30
rlanday
On 2017/04/21 at 01:32:25, yosin wrote: > Could you rename DMC::RemoveMarkers() by usage pattern? Overloading ...
1 week, 1 day ago (2017-04-21 02:06:29 UTC) #31
yosin_OOO_til_May_7
https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode38 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:38: virtual bool RemoveMarkersUnderWords(const String& node_text, We don't need to ...
5 days, 11 hours ago (2017-04-24 04:18:56 UTC) #36
rlanday
On 2017/04/24 at 04:18:56, yosin wrote: > https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode38 ...
5 days, 10 hours ago (2017-04-24 04:53:35 UTC) #37
Xiaocheng
lgtm https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/140001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode37 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:37: #include "core/editing/markers/DocumentMarkerListEditor.h" nit: I think we want to ...
5 days ago (2017-04-24 15:13:33 UTC) #38
rlanday
On 2017/04/24 at 15:13:33, xiaochengh wrote: > In the design that we agreed, DMLEditor is ...
4 days, 21 hours ago (2017-04-24 18:16:07 UTC) #39
Xiaocheng
On 2017/04/24 at 18:16:07, rlanday wrote: > On 2017/04/24 at 15:13:33, xiaochengh wrote: > > ...
4 days, 21 hours ago (2017-04-24 18:20:59 UTC) #40
rlanday
On 2017/04/24 at 18:20:59, xiaochengh wrote: > It looks OK to me to have an ...
4 days, 21 hours ago (2017-04-24 18:24:36 UTC) #41
rlanday
Updated with the TODO
4 days, 20 hours ago (2017-04-24 18:35:14 UTC) #43
Xiaocheng
On 2017/04/24 at 18:24:36, rlanday wrote: > On 2017/04/24 at 18:20:59, xiaochengh wrote: > > ...
4 days, 20 hours ago (2017-04-24 18:55:55 UTC) #45
yosin_OOO_til_May_7
https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode215 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/Source/core/editing/markers/DocumentMarkerController.cpp#newcode249 ...
4 days, 7 hours ago (2017-04-25 08:20:59 UTC) #48
rlanday
https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2820633002/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode336 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, ...
3 days, 20 hours ago (2017-04-25 19:29:52 UTC) #49
rlanday
Updated
3 days, 19 hours ago (2017-04-25 19:44:33 UTC) #51
yosin_OOO_til_May_7
lgtm w/ nit https://codereview.chromium.org/2820633002/diff/180001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2820633002/diff/180001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode23 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:23: protected: Please make sections in public, ...
3 days, 11 hours ago (2017-04-26 03:41:51 UTC) #55
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/2820633002/200001
2 days, 19 hours ago (2017-04-26 19:41:52 UTC) #58
commit-bot: I haz the power
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_rel_ng/builds/439821)
2 days, 17 hours ago (2017-04-26 21:46:21 UTC) #60
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/2820633002/200001
2 days, 16 hours ago (2017-04-26 23:07:55 UTC) #62
commit-bot: I haz the power
2 days, 13 hours ago (2017-04-27 02:00:55 UTC) #65
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/c84b714f7a9730eeae4a4ccd84ac...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46