[DMC #3.5] Split up DocumentMarkerListEditor::AddMarker() into two methods

Split up DocumentMarkerListEditor::AddMarker() into two methods

This change helps us avoid duplication in the MarkerType-specific list
implementations I'm going to add. Instead of having one
DocumentMarkerListEditor::AddMarker() method that works for
GenericDocumentMarkerListImpl, but then requires each of the other list
implementations to implement the method separately, we'll have one method,
AddAndMergeOverlapping(), that works for Spelling/Grammar markers (and can be
moved into SpellCheckMarkerList once we eliminate
GenericDocumentMarkerListImpl), and another method
AddWithoutMergingOverlapping(), that can be used for the other marker types.
GenericDocumentMarkerListImpl will choose which to use based on the type of the
inserted marker.

Right now both of these methods create a RenderedDocumentMarker from the
passed-in DocumentMarker. I will move this piece out of the method once not all
marker list implementations are still creating RenderedDocumentMarkers.

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

[rlanday, 2017-04-26 20:30:48]

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

[commit-bot: I haz the power, 2017-04-26 20:31:44]

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-26 21:12:39]

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

[Xiaocheng, 2017-04-26 22:12:08]

https://codereview.chromium.org/2842263002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp
(left):

https://codereview.chromium.org/2842263002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:137:
void DocumentMarkerListEditor::MergeOverlapping(
Could you put DocumentMarkerListEditor::AddMarkerAndMergeOverlapping here to
minimize the changeset?

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

https://codereview.chromium.org/2842263002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:11:
void DocumentMarkerListEditor::AddMarkerAndMergeOverlapping(
Could you put DocumentMarkerListEditor::AddMarkerWithoutMergingOverlapping here
to minimize the change set?

https://codereview.chromium.org/2842263002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:24:
const DocumentMarker* marker_rendered_marker) {
No need to rename this parameter.

[rlanday, 2017-04-26 22:14:46]

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

https://codereview.chromium.org/2842263002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:24:
const DocumentMarker* marker_rendered_marker) {
On 2017/04/26 at 22:12:07, Xiaocheng wrote:
> No need to rename this parameter.

Oops, bad find-and-replace :)

[rlanday, 2017-04-26 22:20:25]

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

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

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-26 22:21:06]

Updated

[Xiaocheng, 2017-04-26 22:30:37]

lgtm

[commit-bot: I haz the power, 2017-04-27 00:30:18]

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

[commit-bot: I haz the power, 2017-04-27 00:30:19]

Dry run: 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_...)

[yosin_UTC9, 2017-04-28 01:01:28]

lgtm w/ nits

https://codereview.chromium.org/2842263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp
(right):

https://codereview.chromium.org/2842263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:21:
MarkerList::iterator pos = std::lower_bound(
nit: s/MakerList::iterator/const MarkerList::iterator/
Use |const auto| as other usage of |std::lower_bound()| in this file?

https://codereview.chromium.org/2842263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:134:
RenderedDocumentMarker* rendered_marker =
nit: s/RenderedDocumentMarker*/RenderedDocumentMarker* const/

https://codereview.chromium.org/2842263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp
(right):

https://codereview.chromium.org/2842263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp:29:
NOTREACHED();
Let's print marker->GetType() to add more info to crash log.

e.g. NOTREACHED() << "Unhanded marker type: " << marker->GetType();

[rlanday, 2017-04-28 01:56:11]

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

[commit-bot: I haz the power, 2017-04-28 01:57:28]

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-28 01:58:34]

The CQ bit was unchecked by rlanday@chromium.org

[rlanday, 2017-04-28 01:58:38]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-04-28 01:58:39]

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/2842263002/#ps40001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-04-28 01:59:13]

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-28 04:04:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493344718617940,
"parent_rev": "c454c56eb566695c4f56909366f603bcc1c031ba", "commit_rev":
"db5d3c0b9ba46e665c77be59be3695df59f4e139"}

[commit-bot: I haz the power, 2017-04-28 04:06:16]

Description was changed from

==========
Split up DocumentMarkerListEditor::AddMarker() into two methods

This change helps us avoid duplication in the MarkerType-specific list
implementations I'm going to add. Instead of having one
DocumentMarkerListEditor::AddMarker() method that works for
GenericDocumentMarkerListImpl, but then requires each of the other list
implementations to implement the method separately, we'll have one method,
AddAndMergeOverlapping(), that works for Spelling/Grammar markers (and can be
moved into SpellCheckMarkerList once we eliminate
GenericDocumentMarkerListImpl), and another method
AddWithoutMergingOverlapping(), that can be used for the other marker types.
GenericDocumentMarkerListImpl will choose which to use based on the type of the
inserted marker.

Right now both of these methods create a RenderedDocumentMarker from the
passed-in DocumentMarker. I will move this piece out of the method once not all
marker list implementations are still creating RenderedDocumentMarkers.

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

to

==========
Split up DocumentMarkerListEditor::AddMarker() into two methods

This change helps us avoid duplication in the MarkerType-specific list
implementations I'm going to add. Instead of having one
DocumentMarkerListEditor::AddMarker() method that works for
GenericDocumentMarkerListImpl, but then requires each of the other list
implementations to implement the method separately, we'll have one method,
AddAndMergeOverlapping(), that works for Spelling/Grammar markers (and can be
moved into SpellCheckMarkerList once we eliminate
GenericDocumentMarkerListImpl), and another method
AddWithoutMergingOverlapping(), that can be used for the other marker types.
GenericDocumentMarkerListImpl will choose which to use based on the type of the
inserted marker.

Right now both of these methods create a RenderedDocumentMarker from the
passed-in DocumentMarker. I will move this piece out of the method once not all
marker list implementations are still creating RenderedDocumentMarkers.

BUG=707867

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

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

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