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

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights
  * reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
@@ skipping 59 matching lines @@
     case DocumentMarker::kComposition:
       return DocumentMarker::kCompositionMarkerIndex;
   }
 
   NOTREACHED();
   return DocumentMarker::kSpellingMarkerIndex;
 }
 
 }  // 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);
 }
 
 DocumentMarkerController::DocumentMarkerController(Document& document)
     : possibly_existing_marker_types_(0), document_(&document) {
   SetContext(&document);
 }
 
@@ skipping 130 matching lines @@
 
   possibly_existing_marker_types_.Add(new_marker.GetType());
 
   Member<MarkerLists>& markers =
       markers_.insert(node, nullptr).stored_value->value;
   if (!markers) {
     markers = new MarkerLists;
     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
   if (node->GetLayoutObject()) {
     node->GetLayoutObject()->SetShouldDoFullPaintInvalidation(
         kPaintInvalidationDocumentMarkerChange);
   }
 }
 
 // TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
@@ skipping 520 matching lines @@
 
 bool DocumentMarkerController::SetMarkersActive(Node* node,
                                                 unsigned start_offset,
                                                 unsigned end_offset,
                                                 bool active) {
   MarkerLists* markers = markers_.at(node);
   if (!markers)
     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 =
       std::upper_bound(list->begin(), list->end(), start_offset, EndsBefore);
   for (MarkerList::iterator marker = start_pos; marker != list->end();
        ++marker) {
     // Markers are returned in order, so stop if we are now past the specified
     // range.
     if ((*marker)->StartOffset() >= end_offset)
       break;
@@ skipping 105 matching lines @@
 }
 
 }  // namespace blink
 
 #ifndef NDEBUG
 void showDocumentMarkers(const blink::DocumentMarkerController* controller) {
   if (controller)
     controller->ShowMarkers();
 }
 #endif