Clean up DocumentMarkerController::copyMarkers() (now it's moveMarkers())

Clean up DocumentMarkerController::copyMarkers() (now it's moveMarkers())

This method is kind of weird since it leaves the markers in the source list but
sometimes changes one of them (if it spans a node boundary). In this CL, I'm
cleaning it up so it removes the markers from the source list after adding them
to the destination list.

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

[rlanday, 2017-04-08 02:28:26]

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

[commit-bot: I haz the power, 2017-04-08 02:28:45]

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-08 02:28:49]

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

[rlanday, 2017-04-08 02:30:10]

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/SplitTextNodeCommand.cpp
(right):

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/SplitTextNodeCommand.cpp:91:
document().markers().moveMarkers(m_text2.get(), 0, m_offset, m_text1.get(),
I'm glad I wrote the test case in the dependent CL to catch that I needed this!
(It was previously depending on the markers still existing on the unused marker)

[Xiaocheng, 2017-04-08 02:46:40]

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/SplitTextNodeCommand.cpp
(right):

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/SplitTextNodeCommand.cpp:91:
document().markers().moveMarkers(m_text2.get(), 0, m_offset, m_text1.get(),
On 2017/04/08 at 02:30:09, rlanday wrote:
> I'm glad I wrote the test case in the dependent CL to catch that I needed
this! (It was previously depending on the markers still existing on the unused
marker)

Good catch!

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:294:
// moves markers from srcNode to dstNode, applying the specified shift delta to
Also remove the comment related to shifting. It's never used.

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
(right):

https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:80:
int delta);
Let's also get rid of |delta|. It's always 0.

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

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

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

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-10 21:38:22]

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

[rlanday, 2017-04-10 21:38:46]

On 2017/04/08 at 02:46:40, xiaochengh wrote:
>
https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:80:
int delta);
> Let's also get rid of |delta|. It's always 0.

Ok, done. Note that start_offset is also currently always 0.

[commit-bot: I haz the power, 2017-04-10 21:39:09]

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-10 21:47:41]

On 2017/04/10 at 21:38:46, rlanday wrote:
> On 2017/04/08 at 02:46:40, xiaochengh wrote:
> >
https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
> >
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:80:
int delta);
> > Let's also get rid of |delta|. It's always 0.
> 
> Ok, done. Note that start_offset is also currently always 0.

Good catch. Then we should also get rid of start_offset :)

There's no need to keep any dead code in such old code.

[commit-bot: I haz the power, 2017-04-10 21:59:06]

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

[commit-bot: I haz the power, 2017-04-10 21:59:07]

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

[rlanday, 2017-04-10 22:19:41]

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

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

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-10 22:22:08]

On 2017/04/10 at 21:47:41, xiaochengh wrote:
> On 2017/04/10 at 21:38:46, rlanday wrote:
> > On 2017/04/08 at 02:46:40, xiaochengh wrote:
> > >
https://codereview.chromium.org/2810463002/diff/1/third_party/WebKit/Source/c...
> > >
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:80:
int delta);
> > > Let's also get rid of |delta|. It's always 0.
> > 
> > Ok, done. Note that start_offset is also currently always 0.
> 
> Good catch. Then we should also get rid of start_offset :)
> 
> There's no need to keep any dead code in such old code.

Updated

[Xiaocheng, 2017-04-10 22:38:00]

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

https://codereview.chromium.org/2810463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:290:
// before start_offset + length. Markers that run past that point are truncated.
nit: there's no more |start_offset|

https://codereview.chromium.org/2810463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:311:
MarkerList::iterator start_pos =
Is it simply |list->begin()|?

[rlanday, 2017-04-10 22:46:11]

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

https://codereview.chromium.org/2810463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:311:
MarkerList::iterator start_pos =
On 2017/04/10 at 22:38:00, Xiaocheng wrote:
> Is it simply |list->begin()|?

...yes :)

[rlanday, 2017-04-10 22:54:43]

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

[rlanday, 2017-04-10 22:54:48]

On 2017/04/10 at 22:46:11, rlanday wrote:
>
https://codereview.chromium.org/2810463002/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> 
>
https://codereview.chromium.org/2810463002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:311:
MarkerList::iterator start_pos =
> On 2017/04/10 at 22:38:00, Xiaocheng wrote:
> > Is it simply |list->begin()|?
> 
> ...yes :)

Updated

[commit-bot: I haz the power, 2017-04-10 22:55: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-10 23:16:45]

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

[commit-bot: I haz the power, 2017-04-10 23:17:03]

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-10 23:17:09]

Ok, updated again (some stuff got messed up when rebasing everything on top of
the rename)

[Xiaocheng, 2017-04-10 23:30:28]

lgtm

[yosin_UTC9, 2017-04-11 00:57:36]

lgtm

[commit-bot: I haz the power, 2017-04-11 01:25:12]

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

[commit-bot: I haz the power, 2017-04-11 01:25:13]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-11 02:33:44]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-04-11 02:34:37]

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-11 02:59:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491878024156840,
"parent_rev": "e4f39f538c7a9fe52a635d4fc4c92e3a5c82fe8a", "commit_rev":
"0dea0097ea24907ed4c0f30fb4180191f378f00e"}

[commit-bot: I haz the power, 2017-04-11 03:00:50]

Description was changed from

==========
Clean up DocumentMarkerController::copyMarkers() (now it's moveMarkers())

This method is kind of weird since it leaves the markers in the source list but
sometimes changes one of them (if it spans a node boundary). In this CL, I'm
cleaning it up so it removes the markers from the source list after adding them
to the destination list.

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

to

==========
Clean up DocumentMarkerController::copyMarkers() (now it's moveMarkers())

This method is kind of weird since it leaves the markers in the source list but
sometimes changes one of them (if it spans a node boundary). In this CL, I'm
cleaning it up so it removes the markers from the source list after adding them
to the destination list.

BUG=707867

Review-Url: https://codereview.chromium.org/2810463002
Cr-Commit-Position: refs/heads/master@{#463509}
Committed:
https://chromium.googlesource.com/chromium/src/+/0dea0097ea24907ed4c0f30fb418...
==========

[commit-bot: I haz the power, 2017-04-11 03:00:51]

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