[DMC #1.3] Add DocumentMarkerController::ListForType()

Add DocumentMarkerController::ListForType()

Helper method that makes it slightly cleaner to look up the MarkerList* for a
given type when we're holding the MarkerLists* for a Node. Alternatively, we
could have a method that takes a Node* instead of a MarkerLists*, but then we'd
lose the optimization in the call sites where we can return early if there's no
MarkerLists* for the given Node.

I found two places to use this in the current version of the code, and there
will be more after we rewrite some of the loops to use the DocumentMarkerList
interface being added in https://codereview.chromium.org/2820633002 .

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

[rlanday, 2017-04-17 08:19:36]

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

[rlanday, 2017-04-17 08:19:46]

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

https://codereview.chromium.org/2822963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:81:
DocumentMarkerController::ListForType(MarkerLists* marker_lists,
I was confused when writing this why I have to specify
"DocumentMarkerController::" before MarkerList in the return type but not before
MarkerLists in the parameter list (both types are specified by private using
declarations in DocumentMarkerController), does anyone know?

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

https://codereview.chromium.org/2822963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:160:
static Member<MarkerList>& ListForType(MarkerLists*,
I added this as a static method instead of just putting it in an anonymous
namespace in the cpp file because we need to be able to use the private
MarkerList and MarkerLists typedefs, which don't really seem like they should be
public (but that would be another option).

[rlanday, 2017-04-17 08:19:52]

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:20:10]

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

[yosin_UTC9, 2017-04-17 09:13:44]

lgtm

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

https://codereview.chromium.org/2822963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:83:
size_t marker_list_index = MarkerTypeToMarkerIndex(type);
nit: s/size_t/const size_t/

https://codereview.chromium.org/2822963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:84:
return marker_lists->at(marker_list_index);
nit: It is OK to use |marker_lists[marker_list_index]|, since
|MarkerTYpeToMarkerIndex()| returns
valid index. Also, we don't use exception.

If we want to validate index, let's add
|DCHECK_LT(marker_list_index, marker_lists.size())|

https://codereview.chromium.org/2822963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:237:
DocumentMarker::MarkerType new_marker_type = new_marker.GetType();
nit: s/DocumentMarker::MarkerType/const DocumentMarker::MarkerType/

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

https://codereview.chromium.org/2822963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:160:
static Member<MarkerList>& ListForType(MarkerLists*,
On 2017/04/17 at 08:19:46, rlanday wrote:
> I added this as a static method instead of just putting it in an anonymous
namespace in the cpp file because we need to be able to use the private
MarkerList and MarkerLists typedefs, which don't really seem like they should be
public (but that would be another option).

Maybe introduce |DocumentMarkerListSet| class?

[commit-bot: I haz the power, 2017-04-17 09:51:54]

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

[commit-bot: I haz the power, 2017-04-17 09:51:54]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-18 01:48:04]

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

https://codereview.chromium.org/2822963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:84:
return marker_lists->at(marker_list_index);
Ah, I didn't realize this did bounds checking, I just thought
(*marker_lists)[marker_list_index] looked funny...

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

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:55:09]

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 03:33:01]

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

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

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-04-18 03:34:30]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-04-18 03:34:30]

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/2822963002/#ps20001
(title: "Fix nits, remove a dependency")

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

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:38:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492486470033240,
"parent_rev": "341333dfb5561bc987995a3cfe05aefc3ecb17e7", "commit_rev":
"b5a0bdbe8b745676efac4996f50c0558bb17b839"}

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

Description was changed from

==========
Add DocumentMarkerController::ListForType()

Helper method that makes it slightly cleaner to look up the MarkerList* for a
given type when we're holding the MarkerLists* for a Node. Alternatively, we
could have a method that takes a Node* instead of a MarkerLists*, but then we'd
lose the optimization in the call sites where we can return early if there's no
MarkerLists* for the given Node.

I found two places to use this in the current version of the code, and there
will be more after we rewrite some of the loops to use the DocumentMarkerList
interface being added in https://codereview.chromium.org/2820633002 .

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

to

==========
Add DocumentMarkerController::ListForType()

Helper method that makes it slightly cleaner to look up the MarkerList* for a
given type when we're holding the MarkerLists* for a Node. Alternatively, we
could have a method that takes a Node* instead of a MarkerLists*, but then we'd
lose the optimization in the call sites where we can return early if there's no
MarkerLists* for the given Node.

I found two places to use this in the current version of the code, and there
will be more after we rewrite some of the loops to use the DocumentMarkerList
interface being added in https://codereview.chromium.org/2820633002 .

BUG=707867

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

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b5a0bdbe8b745676efac4996f50c...