[DMC #1.1] Refactor DocumentMarkerController::MoveMarkers()

Refactor DocumentMarkerController::MoveMarkers()

As we (yosin, xiaochengh, and I) discussed, I'm splitting the implementation
changes for this method I previously was including in
https://codereview.chromium.org/2812423002 into a separate CL.

We want to separate out the implementation changes we need to pull out the main
part of this method into a method on DocumentMarkerListEditor, and actually
adding that method.

This CL needs the AddMarker() method I'm adding in that CL, so I've removed the
MoveMarkers() changes entirely from that one and will upload a third CL that
adds DocumentMarkerListEditor::MoveMarkers().

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

[rlanday, 2017-04-17 05:14:36]

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

[rlanday, 2017-04-17 05:14:49]

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

[commit-bot: I haz the power, 2017-04-17 05:14:50]

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-17 05:19:33]

Wait, I think we actually want to do the change without a helper method in this
CL...

[rlanday, 2017-04-17 06:30:49]

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

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

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-17 06:34:15]

On 2017/04/17 at 05:19:33, rlanday wrote:
> Wait, I think we actually want to do the change without a helper method in
this CL...

Ok, I think this is what we want: refactoring this method to prepare to add the
helper method without actually doing it yet

[rlanday, 2017-04-17 06:34:23]

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

[Xiaocheng, 2017-04-17 06:37:29]

https://codereview.chromium.org/2819173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
(right):

https://codereview.chromium.org/2819173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:152:
bool MoveMarkersToList(MarkerList* src_list,
Please get rid of this.

[rlanday, 2017-04-17 07:02:21]

Description was changed from

==========
Add DocumentMarkerController::MoveMarkersToList() helper method

As we (yosin, xiaochengh, and I) discussed, I'm splitting the implementation
changes for this method I previously was including in
https://codereview.chromium.org/2812423002 into a separate CL.

This CL needs the AddMarker() method I'm adding in that CL, so I've removed the
MoveMarkers() changes entirely from that one and will upload a third CL that
adds DocumentMarkerListEditor::MoveMarkers();

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

to

==========
Refactor DocumentMarkerController::MoveMarkers()

As we (yosin, xiaochengh, and I) discussed, I'm splitting the implementation
changes for this method I previously was including in
https://codereview.chromium.org/2812423002 into a separate CL.

We want to separate out the implementation changes we need to pull out the main
part of this method into a method on DocumentMarkerListEditor, and actually
adding that method.

This CL needs the AddMarker() method I'm adding in that CL, so I've removed the
MoveMarkers() changes entirely from that one and will upload a third CL that
adds DocumentMarkerListEditor::MoveMarkers().

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

[rlanday, 2017-04-17 07:02:21]

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

[rlanday, 2017-04-17 07:02:31]

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

[commit-bot: I haz the power, 2017-04-17 07:02:42]

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-17 07:02:50]

On 2017/04/17 at 06:37:29, xiaochengh wrote:
>
https://codereview.chromium.org/2819173002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
(right):
> 
>
https://codereview.chromium.org/2819173002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:152:
bool MoveMarkersToList(MarkerList* src_list,
> Please get rid of this.

ahh sorry, fixed

[rlanday, 2017-04-17 08:10:32]

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

[commit-bot: I haz the power, 2017-04-17 08:10:36]

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-17 09:40:49]

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

[commit-bot: I haz the power, 2017-04-17 09:40:50]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-17 21:37:44]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2017-04-17 21:37:45]

+tkent as yosin is OOO.

lgtm with nits.

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

https://codereview.chromium.org/2819173002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:311:
marker_list_index++) {
nit: ++marker_list_index

Google coding style prefers prefix increment.

https://codereview.chromium.org/2819173002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:316:
if (!dst_markers->at(marker_list_index)) {
nit: no need to have extra braces

[tkent, 2017-04-18 01:25:26]

lgtm

[rlanday, 2017-04-18 01:38:57]

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

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

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-18 02:45:23]

The CQ bit was unchecked by rlanday@chromium.org

[rlanday, 2017-04-18 02:45:25]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-04-18 02:45:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2819173002/#ps80001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-04-18 02:45:52]

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 03:19:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492483525903340,
"parent_rev": "ff0fb24270d54a21f036fa77132b11fe03eafd06", "commit_rev":
"a92abb9ad049f637596371997cc125f182ae3cc3"}

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

Description was changed from

==========
Refactor DocumentMarkerController::MoveMarkers()

As we (yosin, xiaochengh, and I) discussed, I'm splitting the implementation
changes for this method I previously was including in
https://codereview.chromium.org/2812423002 into a separate CL.

We want to separate out the implementation changes we need to pull out the main
part of this method into a method on DocumentMarkerListEditor, and actually
adding that method.

This CL needs the AddMarker() method I'm adding in that CL, so I've removed the
MoveMarkers() changes entirely from that one and will upload a third CL that
adds DocumentMarkerListEditor::MoveMarkers().

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

to

==========
Refactor DocumentMarkerController::MoveMarkers()

As we (yosin, xiaochengh, and I) discussed, I'm splitting the implementation
changes for this method I previously was including in
https://codereview.chromium.org/2812423002 into a separate CL.

We want to separate out the implementation changes we need to pull out the main
part of this method into a method on DocumentMarkerListEditor, and actually
adding that method.

This CL needs the AddMarker() method I'm adding in that CL, so I've removed the
MoveMarkers() changes entirely from that one and will upload a third CL that
adds DocumentMarkerListEditor::MoveMarkers().

BUG=707867

Review-Url: https://codereview.chromium.org/2819173002
Cr-Commit-Position: refs/heads/master@{#465124}
Committed:
https://chromium.googlesource.com/chromium/src/+/a92abb9ad049f637596371997cc1...
==========

[commit-bot: I haz the power, 2017-04-18 03:20:21]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a92abb9ad049f637596371997cc1...