[DMC #1.915] Refactor DocumentMarkerController::RemoveMarkers(const MarkerRemoverPredicate&)

Refactor DocumentMarkerController::RemoveMarkers(const MarkerRemoverPredicate&)

MarkerRemoverPredicate seems to have been added with some amount of generality
in mind, but the way it's written it only serves a single purpose, so I'm
refactoring the RemoveMarkers() method that takes it to just take a vector of
words instead.

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

[rlanday, 2017-04-21 04:02:03]

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

[rlanday, 2017-04-21 04:02:11]

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

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

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-21 05:40:32]

lgtm

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

https://codereview.chromium.org/2833753004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:496:
for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) {
Just noticed that this function is wrong... It removes all markers, not just
spelling.

Anyway, it's out of scope. I filed crbug.com/713995 for it.

[rlanday, 2017-04-21 05:42:00]

On 2017/04/21 at 05:40:32, xiaochengh wrote:
> lgtm
> 
>
https://codereview.chromium.org/2833753004/diff/1/third_party/WebKit/Source/c...
> File
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):
> 
>
https://codereview.chromium.org/2833753004/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:496:
for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) {
> Just noticed that this function is wrong... It removes all markers, not just
spelling.
> 
> Anyway, it's out of scope. I filed crbug.com/713995 for it.

Ah...I noticed that too, sorry I forgot to mention it

[commit-bot: I haz the power, 2017-04-21 07:38:30]

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

[commit-bot: I haz the power, 2017-04-21 07:38:30]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-04-21 08:29:33]

lgtm w/ nits

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

https://codereview.chromium.org/2833753004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:504:
unsigned start = marker.StartOffset();
nit: s/unsigned/const unsigned/

https://codereview.chromium.org/2833753004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:505:
unsigned length = marker.EndOffset() - marker.StartOffset();
nit: s/unsigned/const unsigned/

https://codereview.chromium.org/2833753004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:507:
String marker_text = ToText(node).data().Substring(start, length);
nit: s/String/const String&/

[rlanday, 2017-04-21 08:43:58]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-04-21 08:43:59]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2833753004/#ps40001
(title: "Use const")

[commit-bot: I haz the power, 2017-04-21 08:44:18]

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

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

[commit-bot: I haz the power, 2017-04-21 10:17:27]

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_...)

[rlanday, 2017-04-21 23:38:45]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-04-21 23:39: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-22 00:57:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492817925072020,
"parent_rev": "33265b4e4ca639a36a080a857b4fcca1654707c6", "commit_rev":
"ad6b1f378b7706541d97b227638ba93c446fc8f1"}

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

Description was changed from

==========
Refactor DocumentMarkerController::RemoveMarkers(const MarkerRemoverPredicate&)

MarkerRemoverPredicate seems to have been added with some amount of generality
in mind, but the way it's written it only serves a single purpose, so I'm
refactoring the RemoveMarkers() method that takes it to just take a vector of
words instead.

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

to

==========
Refactor DocumentMarkerController::RemoveMarkers(const MarkerRemoverPredicate&)

MarkerRemoverPredicate seems to have been added with some amount of generality
in mind, but the way it's written it only serves a single purpose, so I'm
refactoring the RemoveMarkers() method that takes it to just take a vector of
words instead.

BUG=707867

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

[commit-bot: I haz the power, 2017-04-22 00:58:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ad6b1f378b7706541d97b227638b...

[rlanday, 2017-04-22 10:03:23]

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

https://codereview.chromium.org/2833753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:519:
const String marker_text = ToText(node).data().Substring(start, length);
oops, missed making this a reference here, I'm moving this to
DocumentMarkerListEditor anyway in https://codereview.chromium.org/2836563002
though so I can fix it there