[DMC #1.9] Move DocumentMarkerListEditor to its own .h/.cpp files

Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

This is a follow-up to this CL where the DocumentMarkerListEditor class
was created inside DocumentMarkerController:
https://codereview.chromium.org/2812423002

BUG=707867
Review-Url: https://codereview.chromium.org/2818873002
Cr-Commit-Position: refs/heads/master@{#466224}
Committed: https://chromium.googlesource.com/chromium/src/+/37c5a8829c124800330ab1965fa29615465551e9

[rlanday, 2017-04-13 23:05:03]

Description was changed from

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

BUG=707867
==========

to

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

BUG=707867
==========

[rlanday, 2017-04-13 23:05:03]

rlanday@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yosin@chromium.org

[rlanday, 2017-04-13 23:25:36]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 23:26:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 01:06:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 01:06:21]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-14 02:37:41]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 02:37:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 04:25:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 04:26:00]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-18 01:54:16]

rlanday@chromium.org changed reviewers:
+ yoichio@chromium.org

[rlanday, 2017-04-18 04:24:13]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-04-18 04:24:26]

Rebased

[commit-bot: I haz the power, 2017-04-18 04:24:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-04-18 04:29:57]

https://codereview.chromium.org/2818873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2818873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:270:
if (DocumentMarkerListEditor::MoveMarkers(src_list, length, dst_list))
Doesn't seem properly rebased... There should be only code move in this patch.

[rlanday, 2017-04-18 04:35:27]

Description was changed from

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

BUG=707867
==========

to

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

This is a follow-up to this CL where the DocumentMarkerListEditor class was
created
inside DocumentMarkerController:
https://codereview.chromium.org/2812423002

BUG=707867
==========

[rlanday, 2017-04-18 04:35:40]

Description was changed from

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

This is a follow-up to this CL where the DocumentMarkerListEditor class was
created
inside DocumentMarkerController:
https://codereview.chromium.org/2812423002

BUG=707867
==========

to

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

This is a follow-up to this CL where the DocumentMarkerListEditor class
was created inside DocumentMarkerController:
https://codereview.chromium.org/2812423002

BUG=707867
==========

[rlanday, 2017-04-18 05:02:13]

On 2017/04/18 at 04:29:57, xiaochengh wrote:
>
https://codereview.chromium.org/2818873002/diff/60001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> 
>
https://codereview.chromium.org/2818873002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:270:
if (DocumentMarkerListEditor::MoveMarkers(src_list, length, dst_list))
> Doesn't seem properly rebased... There should be only code move in this patch.

ah I think I accidentally uploaded two patches together...

[rlanday, 2017-04-18 05:03:32]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-04-18 05:03:32]

On 2017/04/18 at 05:02:13, rlanday wrote:
> On 2017/04/18 at 04:29:57, xiaochengh wrote:
> >
https://codereview.chromium.org/2818873002/diff/60001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> > 
> >
https://codereview.chromium.org/2818873002/diff/60001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:270:
if (DocumentMarkerListEditor::MoveMarkers(src_list, length, dst_list))
> > Doesn't seem properly rebased... There should be only code move in this
patch.
> 
> ah I think I accidentally uploaded two patches together...

Fixed

[commit-bot: I haz the power, 2017-04-18 05:03:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 05:06:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 05:06:31]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rlanday, 2017-04-18 05:09:35]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 05:09:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 07:16:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 07:16:34]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-19 03:33:23]

https://codereview.chromium.org/2818873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2818873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:662:
static bool EndsBefore(size_t start_offset,
It's weird to have a chunk of new code in an old file in a patch named "move
code". It's also bad to have the same function duplicated in two files.

How about moving these static functions to, say, DocumentMarker as static
members first? So that they can be used everywhere without having duplicate
code.

Btw, can these functions take |const DocumentMarker*| as parameters? If so, we
can do things like:

1. Change these functions to take |const DocumentMarker*| instead of |const
Member<RDM>&|
2. Move these functions as static members of DocumentMarker

[yosin_UTC9, 2017-04-20 01:43:01]

https://codereview.chromium.org/2818873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2818873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:663:
const Member<RenderedDocumentMarker>& rhv) {
Please use descriptive parameter name instead of |rhv|, I could not get what
this stands for...

[rlanday, 2017-04-20 02:19:05]

https://codereview.chromium.org/2818873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2818873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:662:
static bool EndsBefore(size_t start_offset,
On 2017/04/19 at 03:33:23, Xiaocheng wrote:
> It's weird to have a chunk of new code in an old file in a patch named "move
code". It's also bad to have the same function duplicated in two files.

This isn't new code, I moved it because I thought it made sense to put it closer
to where it's used since I was removing the other functions it was grouped with.

https://codereview.chromium.org/2818873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:663:
const Member<RenderedDocumentMarker>& rhv) {
On 2017/04/20 at 01:43:01, yosin_UTC9 wrote:
> Please use descriptive parameter name instead of |rhv|, I could not get what
this stands for...

I'm only moving this code; I think it stands for "right-hand value", but we can
call it something like "marker" if that's better...

[rlanday, 2017-04-20 02:22:04]

On 2017/04/19 at 03:33:23, xiaochengh wrote:
> How about moving these static functions to, say, DocumentMarker as static
members first? So that they can be used everywhere without having duplicate
code.

I think we should do this if/when we actually end up using these methods in
multiple files (e.g. in multiple DocumentMarkerList implementations)


> Btw, can these functions take |const DocumentMarker*| as parameters?

Yes, that works

[rlanday, 2017-04-20 02:42:22]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-04-20 02:42:32]

Updated

[commit-bot: I haz the power, 2017-04-20 02:42:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-04-20 03:09:15]

https://codereview.chromium.org/2818873002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2818873002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:662:
static bool EndsBefore(size_t start_offset,
Moving this function in the same file is an unnecessary change.

Besides, we shouldn't have duplicated code in different files. Please either fix
it or add a TODO for deduplication.

[rlanday, 2017-04-20 03:15:37]

On 2017/04/20 at 03:09:15, xiaochengh wrote:
>
https://codereview.chromium.org/2818873002/diff/110001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> 
>
https://codereview.chromium.org/2818873002/diff/110001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:662:
static bool EndsBefore(size_t start_offset,
> Moving this function in the same file is an unnecessary change.
> 
> Besides, we shouldn't have duplicated code in different files. Please either
fix it or add a TODO for deduplication.

Oh sorry, I missed that I'm using this in two places...

[rlanday, 2017-04-20 03:35:40]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 03:35:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-04-20 03:36:16]

https://codereview.chromium.org/2818873002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2818873002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:662:
static bool EndsBefore(size_t start_offset,
On 2017/04/20 at 03:09:15, Xiaocheng wrote:
> Moving this function in the same file is an unnecessary change.
> 
> Besides, we shouldn't have duplicated code in different files. Please either
fix it or add a TODO for deduplication.

Fixed

[Xiaocheng, 2017-04-20 03:40:37]

Thanks for the change, but...

https://codereview.chromium.org/2818873002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2818873002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:183: static bool
EndsBefore(size_t start_offset, const DocumentMarker*);
Please split...

[rlanday, 2017-04-20 04:13:13]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 04:13:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-04-20 04:13:36]

On 2017/04/20 at 03:40:37, xiaochengh wrote:
> Thanks for the change, but...
> 
>
https://codereview.chromium.org/2818873002/diff/130001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):
> 
>
https://codereview.chromium.org/2818873002/diff/130001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:183: static
bool EndsBefore(size_t start_offset, const DocumentMarker*);
> Please split...

Ok, I have split it off into https://codereview.chromium.org/2828093004 and
updated this patch

[Xiaocheng, 2017-04-20 04:15:37]

lgtm

[yosin_UTC9, 2017-04-20 04:37:47]

lgtm

Thanks!

[commit-bot: I haz the power, 2017-04-20 06:17:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 06:17:11]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-20 06:38:29]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 06:38:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 08:30:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 08:30:05]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-20 08:43:02]

https://codereview.chromium.org/2818873002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp
(right):

https://codereview.chromium.org/2818873002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:11:
static bool StartsFurther(const Member<RenderedDocumentMarker>& lhv,
Please also rebase this part. These two functions should also become lambdas.

[rlanday, 2017-04-20 09:50:35]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-04-20 09:50:53]

On 2017/04/20 at 08:43:02, xiaochengh wrote:
>
https://codereview.chromium.org/2818873002/diff/170001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp
(right):
> 
>
https://codereview.chromium.org/2818873002/diff/170001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:11:
static bool StartsFurther(const Member<RenderedDocumentMarker>& lhv,
> Please also rebase this part. These two functions should also become lambdas.

Updated

[commit-bot: I haz the power, 2017-04-20 09:51:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-04-20 09:57:32]

lgtm

[commit-bot: I haz the power, 2017-04-20 11:36:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 11:36:47]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-21 01:54:44]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-04-21 01:54:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2818873002/#ps190001
(title: "Remove static comparator functions from DocumentMarkerListEditor")

[commit-bot: I haz the power, 2017-04-21 01:55:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 02:00:51]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1492739684813090,
"parent_rev": "90cbb1bd3931fb8ee58c729bbbcdb32f4915dd1f", "commit_rev":
"37c5a8829c124800330ab1965fa29615465551e9"}

[commit-bot: I haz the power, 2017-04-21 02:02:47]

Description was changed from

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

This is a follow-up to this CL where the DocumentMarkerListEditor class
was created inside DocumentMarkerController:
https://codereview.chromium.org/2812423002

BUG=707867
==========

to

==========
Move DocumentMarkerListEditor to its own .h/.cpp files

Based on Step 2 of yosin's plan:
https://codereview.chromium.org/2812423002#msg7

This is a follow-up to this CL where the DocumentMarkerListEditor class
was created inside DocumentMarkerController:
https://codereview.chromium.org/2812423002

BUG=707867

Review-Url: https://codereview.chromium.org/2818873002
Cr-Commit-Position: refs/heads/master@{#466224}
Committed:
https://chromium.googlesource.com/chromium/src/+/37c5a8829c124800330ab1965fa2...
==========

[commit-bot: I haz the power, 2017-04-21 02:02:48]

Committed patchset #11 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/37c5a8829c124800330ab1965fa2...