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

Issue 2773343003: Add DocumentMarkerList in preparation for DocumentMarkerController refactor (Closed)

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

Description

Add DocumentMarkerList in preparation for DocumentMarkerController refactor Split off from https://codereview.chromium.org/2773883003 (Add CompositionMarkerList) since that's getting so big DocumentMarkerList::copyMarkers() seems to count length in a funny way (I think it's counting gaps between letters (i.e. positions where markers can start) instead of the letters themselves), but I think it matches the existing behavior in DocumentMarkerController. BUG=707867

Patch Set 1 #

Patch Set 2 : Add getShiftedMarkerPosition() test cases #

Total comments: 12

Patch Set 3 : Make requested changes (mostly rewriting tests and improving performance of DML methods) #

Patch Set 4 : Rebase #

Total comments: 5

Patch Set 5 : Make requested changes #

Patch Set 6 : Rebase (DocumentMarkerTest changes moving to previous CL) #

Total comments: 1

Patch Set 7 : Rebase, use clone() in copyMarkers() #

Patch Set 8 : Rebase #

Patch Set 9 : Make at() virtual #

Total comments: 2

Patch Set 10 : Remove "explicit" #

Total comments: 3

Patch Set 11 : Remove TODO on copyMarkers() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+489 lines, -2 lines) Patch
M third_party/WebKit/Source/core/editing/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarker.h View 1 2 3 4 5 6 1 chunk +2 lines, -2 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 +68 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp View 1 2 3 4 5 6 7 8 1 chunk +155 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/editing/markers/DocumentMarkerListTest.cpp View 1 2 3 4 1 chunk +261 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 63 (42 generated)
rlanday
3 years, 9 months ago (2017-03-27 21:16:12 UTC) #6
Xiaocheng
Almost good. Please break test cases in DMLTest.cpp further into independent ones. I forgot to ...
3 years, 9 months ago (2017-03-27 22:38:20 UTC) #13
rlanday
https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode39 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: DidCopyMarkerOrNot copyMarkers(unsigned startOffset, On 2017/03/27 at 22:38:20, Xiaocheng wrote: ...
3 years, 9 months ago (2017-03-27 23:57:41 UTC) #14
Xiaocheng
https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp#newcode53 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:53: dstList->add(marker); This might have some side-effect... Now the same ...
3 years, 9 months ago (2017-03-28 00:02:06 UTC) #15
rlanday
https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode39 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:39: DidCopyMarkerOrNot copyMarkers(unsigned startOffset, > - copyMarkers() appears to be ...
3 years, 9 months ago (2017-03-28 00:06:19 UTC) #16
rlanday
On 2017/03/28 at 00:06:19, rlanday wrote: > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode39 ...
3 years, 9 months ago (2017-03-28 00:07:56 UTC) #17
Xiaocheng
On 2017/03/28 at 00:07:56, rlanday wrote: > On 2017/03/28 at 00:06:19, rlanday wrote: > > ...
3 years, 9 months ago (2017-03-28 00:09:36 UTC) #18
rlanday
On 2017/03/28 at 00:02:06, xiaochengh wrote: > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): > > https://codereview.chromium.org/2773343003/diff/20001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp#newcode53 ...
3 years, 9 months ago (2017-03-28 00:23:44 UTC) #19
rlanday
xiaochengh, I optimized those methods in Patch 3, do they look OK now?
3 years, 8 months ago (2017-03-29 17:55:54 UTC) #28
Xiaocheng
For the performance optimization, I was thinking about: - In a sorted list, we can ...
3 years, 8 months ago (2017-03-29 19:41:43 UTC) #29
rlanday
On 2017/03/29 at 19:41:43, xiaochengh wrote: > For the performance optimization, I was thinking about: ...
3 years, 8 months ago (2017-03-29 20:40:32 UTC) #30
rlanday
Updated
3 years, 8 months ago (2017-03-29 20:53:40 UTC) #33
Xiaocheng
https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp#newcode53 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:53: dstList->add(marker); Just a note that we should better change ...
3 years, 8 months ago (2017-03-30 02:13:04 UTC) #36
rlanday
On 2017/03/30 at 02:13:04, xiaochengh wrote: > https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): > > https://codereview.chromium.org/2773343003/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp#newcode53 ...
3 years, 8 months ago (2017-03-30 02:59:50 UTC) #41
rlanday
https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode16 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: explicit DocumentMarkerList(); I don't actually need to define a ...
3 years, 8 months ago (2017-03-31 02:44:05 UTC) #52
Xiaocheng
https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode16 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:16: explicit DocumentMarkerList(); On 2017/03/31 at 02:44:04, rlanday wrote: > ...
3 years, 8 months ago (2017-03-31 02:56:19 UTC) #53
rlanday
On 2017/03/31 at 02:56:19, xiaochengh wrote: > https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (right): > > https://codereview.chromium.org/2773343003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h#newcode16 ...
3 years, 8 months ago (2017-03-31 17:24:05 UTC) #54
rlanday
On 2017/03/31 at 17:24:05, rlanday wrote: > On 2017/03/31 at 02:56:19, xiaochengh wrote: > > ...
3 years, 8 months ago (2017-03-31 17:30:27 UTC) #57
Xiaocheng
lgtm https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp#newcode38 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:38: DocumentMarkerList::DidCopyMarkerOrNot DocumentMarkerList::copyMarkers( Please add |const| to this function ...
3 years, 8 months ago (2017-03-31 18:59:18 UTC) #58
rlanday
https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp (right): https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp#newcode38 third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:38: DocumentMarkerList::DidCopyMarkerOrNot DocumentMarkerList::copyMarkers( On 2017/03/31 at 18:59:18, Xiaocheng wrote: > ...
3 years, 8 months ago (2017-04-03 20:34:43 UTC) #62
rlanday
3 years, 8 months ago (2017-04-03 21:30:14 UTC) #63
https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp
(right):

https://codereview.chromium.org/2773343003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.cpp:38:
DocumentMarkerList::DidCopyMarkerOrNot DocumentMarkerList::copyMarkers(
On 2017/04/03 at 20:34:43, rlanday wrote:
> On 2017/03/31 at 18:59:18, Xiaocheng wrote:
> > Please add |const| to this function to indicate that it doesn't modify the
existing markers.
> > 
> > Btw, the current implementation modifies the existing markers, while this
implementation doesn't.
> > 
> > In fact, I like this implementation. Since we are rewriting everything, it
should be OK to also fix some existing BS, because a lot of things are changed
anyway.
> > 
> > ...Or do we want to fix it first, since DM::clone is available due to some
previous patch?
> 
> Let's fix it here. I will add const and remove the TODO from the header.

We actually already have const here so I'll just remove the TODO :)

Powered by Google App Engine
This is Rietveld 408576698