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

third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp

Index: third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
diff --git a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
index 55a36db1069def335e18c7ab8bcb7c1e312d5c87..b90da10da271c790a973cff741ad3298da52c959 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
@@ -77,6 +77,13 @@ DocumentMarker::MarkerTypeIndex MarkerTypeToMarkerIndex(
 
 }  // namespace
 
+Member<DocumentMarkerController::MarkerList>&
+DocumentMarkerController::ListForType(MarkerLists* marker_lists,

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

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?

+                                      DocumentMarker::MarkerType type) {
+  size_t marker_list_index = MarkerTypeToMarkerIndex(type);

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

nit: s/size_t/const size_t/

+  return marker_lists->at(marker_list_index);

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

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())|

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

Ah, I didn't realize this did bounds checking, I just thought
(*marker_lists)[marker_list_index] looked funny...

+}
+
 inline bool DocumentMarkerController::PossiblyHasMarkers(
     DocumentMarker::MarkerTypes types) {
   return possibly_existing_marker_types_.Intersects(types);
@@ -227,13 +234,11 @@ void DocumentMarkerController::AddMarker(Node* node,
     markers->Grow(DocumentMarker::kMarkerTypeIndexesCount);
   }
 
-  DocumentMarker::MarkerTypeIndex marker_list_index =
-      MarkerTypeToMarkerIndex(new_marker.GetType());
-  if (!markers->at(marker_list_index)) {
-    markers->at(marker_list_index) = new MarkerList;
-  }
+  DocumentMarker::MarkerType new_marker_type = new_marker.GetType();

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

nit: s/DocumentMarker::MarkerType/const DocumentMarker::MarkerType/

+  if (!ListForType(markers, new_marker_type))
+    ListForType(markers, new_marker_type) = new MarkerList;
 
-  Member<MarkerList>& list = markers->at(marker_list_index);
+  Member<MarkerList>& list = ListForType(markers, new_marker_type);
   DocumentMarkerListEditor::AddMarker(list, &new_marker);
 
   // repaint the affected node
@@ -774,8 +779,7 @@ bool DocumentMarkerController::SetMarkersActive(Node* node,
     return false;
 
   bool doc_dirty = false;
-  Member<MarkerList>& list =
-      (*markers)[MarkerTypeToMarkerIndex(DocumentMarker::kTextMatch)];
+  Member<MarkerList>& list = ListForType(markers, DocumentMarker::kTextMatch);
   if (!list)
     return false;
   MarkerList::iterator start_pos =