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

Issue 2820633002: [DMC #2] Add DocumentMarkerList interface and GenericDocumentMarkerListImpl (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

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 #

Messages

Total messages: 65 (39 generated)
rlanday
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-14 02:52:02 UTC) #4
rlanday
Rebased
3 years, 8 months ago (2017-04-18 04:41:29 UTC) #12
yosin_UTC9
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 ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-20 05:33:27 UTC) #20
rlanday
Updated
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-20 10:03:19 UTC) #29
yosin_UTC9
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 ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-21 02:06:29 UTC) #31
yosin_UTC9
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 ...
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months 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: > > ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-24 18:24:36 UTC) #41
rlanday
Updated with the TODO
3 years, 8 months 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: > > ...
3 years, 8 months ago (2017-04-24 18:55:55 UTC) #45
yosin_UTC9
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 ...
3 years, 8 months 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 years, 8 months ago (2017-04-25 19:29:52 UTC) #49
rlanday
Updated
3 years, 8 months ago (2017-04-25 19:44:33 UTC) #51
yosin_UTC9
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 years, 8 months 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
3 years, 8 months 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)
3 years, 8 months 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
3 years, 8 months ago (2017-04-26 23:07:55 UTC) #62
commit-bot: I haz the power
3 years, 8 months 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...

Powered by Google App Engine
This is Rietveld 408576698