[DMC #2] Add DocumentMarkerList interface and GenericDocumentMarkerListImpl

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 36c053690e7d8cbf30bd4a2ca52940f4bc3247ab..699d94b71d46efbf30dc64f893a15059543b96eb 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
@@ -35,6 +35,7 @@
 #include "core/dom/Text.h"
 #include "core/editing/iterators/TextIterator.h"
 #include "core/editing/markers/DocumentMarkerListEditor.h"
+#include "core/editing/markers/GenericDocumentMarkerListImpl.h"
 #include "core/editing/markers/RenderedDocumentMarker.h"
 #include "core/frame/FrameView.h"
 #include "core/layout/LayoutObject.h"
@@ -78,9 +79,9 @@ DocumentMarker::MarkerTypeIndex MarkerTypeToMarkerIndex(
 
 }  // namespace
 
-Member<DocumentMarkerController::MarkerList>&
-DocumentMarkerController::ListForType(MarkerLists* marker_lists,
-                                      DocumentMarker::MarkerType type) {
+Member<DocumentMarkerList>& DocumentMarkerController::ListForType(
+    MarkerLists* marker_lists,
+    DocumentMarker::MarkerType type) {
   const size_t marker_list_index = MarkerTypeToMarkerIndex(type);
   return (*marker_lists)[marker_list_index];
 }
@@ -108,10 +109,10 @@ void DocumentMarkerController::AddMarker(const Position& start,
   // covers.
   for (TextIterator marked_text(start, end); !marked_text.AtEnd();
        marked_text.Advance()) {
-    AddMarker(
-        marked_text.CurrentContainer(),
-        DocumentMarker(type, marked_text.StartOffsetInCurrentContainer(),
-                       marked_text.EndOffsetInCurrentContainer(), description));
+    AddMarker(marked_text.CurrentContainer(),
+              new DocumentMarker(
+                  type, marked_text.StartOffsetInCurrentContainer(),
+                  marked_text.EndOffsetInCurrentContainer(), description));
   }
 }
 
@@ -125,9 +126,9 @@ void DocumentMarkerController::AddTextMatchMarker(
   for (TextIterator marked_text(range.StartPosition(), range.EndPosition());
        !marked_text.AtEnd(); marked_text.Advance()) {
     AddMarker(marked_text.CurrentContainer(),
-              DocumentMarker(marked_text.StartOffsetInCurrentContainer(),
-                             marked_text.EndOffsetInCurrentContainer(),
-                             match_status));
+              new DocumentMarker(marked_text.StartOffsetInCurrentContainer(),
+                                 marked_text.EndOffsetInCurrentContainer(),
+                                 match_status));
   }
   // Don't invalidate tickmarks here. TextFinder invalidates tickmarks using a
   // throttling algorithm. crbug.com/6819.
@@ -141,11 +142,12 @@ void DocumentMarkerController::AddCompositionMarker(const Position& start,
   DCHECK(!document_->NeedsLayoutTreeUpdate());
 
   for (TextIterator marked_text(start, end); !marked_text.AtEnd();
-       marked_text.Advance())
+       marked_text.Advance()) {
     AddMarker(marked_text.CurrentContainer(),
-              DocumentMarker(marked_text.StartOffsetInCurrentContainer(),
-                             marked_text.EndOffsetInCurrentContainer(),
-                             underline_color, thick, background_color));
+              new DocumentMarker(marked_text.StartOffsetInCurrentContainer(),
+                                 marked_text.EndOffsetInCurrentContainer(),
+                                 underline_color, thick, background_color));
+  }
 }
 
 void DocumentMarkerController::PrepareForDestruction() {
@@ -201,12 +203,12 @@ static void UpdateMarkerRenderedRect(const Node& node,
 // Markers of the same type do not overlap each other.
 
 void DocumentMarkerController::AddMarker(Node* node,
-                                         const DocumentMarker& new_marker) {
-  DCHECK_GE(new_marker.EndOffset(), new_marker.StartOffset());
-  if (new_marker.EndOffset() == new_marker.StartOffset())
+                                         DocumentMarker* new_marker) {
+  DCHECK_GE(new_marker->EndOffset(), new_marker->StartOffset());
+  if (new_marker->EndOffset() == new_marker->StartOffset())
     return;
 
-  possibly_existing_marker_types_.Add(new_marker.GetType());
+  possibly_existing_marker_types_.Add(new_marker->GetType());
 
   Member<MarkerLists>& markers =
       markers_.insert(node, nullptr).stored_value->value;
@@ -215,12 +217,12 @@ void DocumentMarkerController::AddMarker(Node* node,
     markers->Grow(DocumentMarker::kMarkerTypeIndexesCount);
   }
 
-  const DocumentMarker::MarkerType new_marker_type = new_marker.GetType();
+  const DocumentMarker::MarkerType new_marker_type = new_marker->GetType();
   if (!ListForType(markers, new_marker_type))
-    ListForType(markers, new_marker_type) = new MarkerList;
+    ListForType(markers, new_marker_type) = new GenericDocumentMarkerListImpl;

[Xiaocheng, 2017/04/20 08:41:34]

Could you add a TODO of introducing a new function that decides which impl to
create?

In general, it's good to add a TODO to anything that's not in its final shape.

 
-  Member<MarkerList>& list = ListForType(markers, new_marker_type);
-  DocumentMarkerListEditor::AddMarker(list, &new_marker);
+  Member<DocumentMarkerList>& list = ListForType(markers, new_marker_type);
+  list->Add(new_marker);
 
   // repaint the affected node
   if (node->GetLayoutObject()) {
@@ -254,15 +256,14 @@ void DocumentMarkerController::MoveMarkers(Node* src_node,
   bool doc_dirty = false;
   for (size_t marker_list_index = 0; marker_list_index < src_markers->size();
        ++marker_list_index) {
-    MarkerList* src_list = src_markers->at(marker_list_index);
+    DocumentMarkerList* src_list = src_markers->at(marker_list_index);
     if (!src_list)
       continue;
 
     if (!dst_markers->at(marker_list_index))
-      dst_markers->at(marker_list_index) = new MarkerList;
-    MarkerList* dst_list = dst_markers->at(marker_list_index);
+      dst_markers->at(marker_list_index) = new GenericDocumentMarkerListImpl;

[Xiaocheng, 2017/04/20 08:41:34]

ditto.

 
-    if (DocumentMarkerListEditor::MoveMarkers(src_list, length, dst_list))
+    if (src_list->MoveMarkers(length, dst_markers->at(marker_list_index)))
       doc_dirty = true;
   }
 
@@ -291,24 +292,23 @@ void DocumentMarkerController::RemoveMarkers(
 
   bool doc_dirty = false;
   size_t empty_lists_count = 0;
-  for (size_t marker_list_index = 0;
-       marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
-       ++marker_list_index) {
-    Member<MarkerList>& list = (*markers)[marker_list_index];
+  for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) {
+    size_t marker_list_index = MarkerTypeToMarkerIndex(type);
+    DocumentMarkerList* list = (*markers)[marker_list_index];
     if (!list || list->IsEmpty()) {
-      if (list.Get() && list->IsEmpty())
-        list.Clear();
+      if (list && list->IsEmpty())
+        (*markers)[marker_list_index] = nullptr;
       ++empty_lists_count;
       continue;
     }
-    if (!marker_types.Contains((*list->begin())->GetType()))
+    if (!marker_types.Contains(type))
       continue;
 
-    if (DocumentMarkerListEditor::RemoveMarkers(list, start_offset, length))
+    if (list->RemoveMarkers(start_offset, length))
       doc_dirty = true;
 
     if (list->IsEmpty()) {
-      list.Clear();
+      (*markers)[marker_list_index] = nullptr;
       ++empty_lists_count;
     }
   }
@@ -335,16 +335,13 @@ DocumentMarkerVector DocumentMarkerController::MarkersFor(
   if (!markers)
     return result;
 
-  for (size_t marker_list_index = 0;
-       marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
-       ++marker_list_index) {
-    Member<MarkerList>& list = (*markers)[marker_list_index];
-    if (!list || list->IsEmpty() ||
-        !marker_types.Contains((*list->begin())->GetType()))
+  for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) {
+    size_t marker_list_index = MarkerTypeToMarkerIndex(type);
+    DocumentMarkerList* list = (*markers)[marker_list_index];
+    if (!list || list->IsEmpty() || !marker_types.Contains(type))
       continue;
 
-    for (size_t i = 0; i < list->size(); ++i)
-      result.push_back(list->at(i).Get());
+    result.AppendVector(list->GetMarkers());
   }
 
   std::sort(result.begin(), result.end(),
@@ -362,9 +359,10 @@ DocumentMarkerVector DocumentMarkerController::Markers() {
     for (size_t marker_list_index = 0;
          marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
          ++marker_list_index) {
-      Member<MarkerList>& list = (*markers)[marker_list_index];
-      for (size_t j = 0; list.Get() && j < list->size(); ++j)
-        result.push_back(list->at(j).Get());
+      DocumentMarkerList* list = (*markers)[marker_list_index];
+      if (!list)
+        continue;
+      result.AppendVector(list->GetMarkers());
     }
   }
   std::sort(result.begin(), result.end(),
@@ -423,20 +421,21 @@ Vector<IntRect> DocumentMarkerController::RenderedRectsForMarkers(
     if (!node.isConnected())
       continue;
     MarkerLists* markers = node_iterator->value.Get();
-    for (size_t marker_list_index = 0;
-         marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
-         ++marker_list_index) {
-      Member<MarkerList>& list = (*markers)[marker_list_index];
-      if (!list || list->IsEmpty() ||
-          (*list->begin())->GetType() != marker_type)
+    for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) {
+      size_t marker_list_index = MarkerTypeToMarkerIndex(type);
+      DocumentMarkerList* list = (*markers)[marker_list_index];
+      if (!list || list->IsEmpty() || type != marker_type)
         continue;
-      for (unsigned marker_index = 0; marker_index < list->size();
-           ++marker_index) {
-        RenderedDocumentMarker* marker = list->at(marker_index).Get();
-        UpdateMarkerRenderedRectIfNeeded(node, *marker);
-        if (!marker->IsRendered())
+
+      const HeapVector<Member<RenderedDocumentMarker>>& markers_in_list =
+          list->GetMarkers();
+      for (DocumentMarker* marker : markers_in_list) {

[Xiaocheng, 2017/04/20 08:41:34]

No need to cast back and forth.

nit: We can also write |for (DocumentMarker* marker : list->GetMarkers())| to
save one line.

+        RenderedDocumentMarker* rendered_marker =
+            ToRenderedDocumentMarker(marker);
+        UpdateMarkerRenderedRectIfNeeded(node, *rendered_marker);
+        if (!rendered_marker->IsRendered())
           continue;
-        result.push_back(marker->RenderedRect());
+        result.push_back(rendered_marker->RenderedRect());
       }
     }
   }
@@ -466,10 +465,12 @@ void DocumentMarkerController::InvalidateRectsForMarkersInNode(
     if (!marker_list || marker_list->IsEmpty())
       continue;
 
-    for (auto& marker : *marker_list)
+    const HeapVector<Member<RenderedDocumentMarker>>& markers_in_list =
+        marker_list->GetMarkers();
+    for (auto& marker : markers_in_list)
       marker->Invalidate();
 
-    if (marker_list->front()->GetType() == DocumentMarker::kTextMatch)
+    if (markers_in_list.front()->GetType() == DocumentMarker::kTextMatch)
       InvalidatePaintForTickmarks(node);
   }
 }
@@ -481,10 +482,12 @@ void DocumentMarkerController::InvalidateRectsForAllMarkers() {
       if (!marker_list || marker_list->IsEmpty())
         continue;
 
-      for (auto& marker : *marker_list)
-        marker->Invalidate();
+      const HeapVector<Member<RenderedDocumentMarker>>& markers_in_list =
+          marker_list->GetMarkers();
+      for (DocumentMarker* marker : markers_in_list)

[Xiaocheng, 2017/04/20 08:41:34]

No need to cast back and forth.

+        ToRenderedDocumentMarker(marker)->Invalidate();
 
-      if (marker_list->front()->GetType() == DocumentMarker::kTextMatch)
+      if (markers_in_list.front()->GetType() == DocumentMarker::kTextMatch)
         InvalidatePaintForTickmarks(node);
     }
   }
@@ -518,17 +521,25 @@ void DocumentMarkerController::RemoveMarkers(
     for (size_t marker_list_index = 0;
          marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
          ++marker_list_index) {
-      Member<MarkerList>& list = markers[marker_list_index];
+      DocumentMarkerList* list = markers[marker_list_index];

[Xiaocheng, 2017/04/20 08:41:34]

This impl doesn't look good -- dump everything out, remove, and dump remaining
markers bad.

We should have DML::RemoveMarkers(predicate) instead.

Please add it to DMLEditor first (in-place, with appropriate TODOs) to fit the
patch description that this patch does nothing but adding DML interface between
DMC and DMLEditor.

[rlanday, 2017/04/20 10:03:18]

Ah, I was going to clean this up when we introduce SpellCheckMarkerList (since
that's really the only list type it's applicable to), but that plan works too

[yosin_UTC9, 2017/04/21 01:32:25]

Could you rename DMC::RemoveMarkers() by usage pattern? Overloading of
RemoveMarkers() makes us confusion.

 void RemoveMarkers(
      const EphemeralRange&,
      DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());

Used only by spell checker, so RemoveMarkersForSpellChecker()

  void RemoveMarkers(
      Node*,
      unsigned start_offset,
      int length,
      DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());

Used only by void DocumentMarkerController::RemoveMarkers(TextIterator&
marked_text, DocumentMarker::MarkerTypes marker_types);

RemoveMarkersInternal() and should be private

 void RemoveMarkers(
      DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());

Input Method Controller: IMC::Clear(), 
SpellChecker, ToggleSpellCheckingEnabled,
TextFinder

RemoveAllMakers or ClearMarkers, we should MarkerTypes as required parameter.

 void RemoveMarkers(
      Node*,
      DocumentMarker::MarkerTypes = DocumentMarker::AllMarkers());

Node::DidMoveToNewDocument() and RemoveSpellingAndGrammarMarkers()

RemoveMarkesForNode()

  void RemoveMarkers(const MarkerRemoverPredicate& should_remove_marker);

Used only in

void SpellChecker::RemoveSpellingMarkersUnderWords(
    const Vector<String>& words) {
  MarkerRemoverPredicate remover_predicate(words);

  DocumentMarkerController& marker_controller =
      GetFrame().GetDocument()->Markers();
  marker_controller.RemoveMarkers(remover_predicate);
  marker_controller.RepaintMarkers();
}

It is better to implement this as DMC::RemoveSpellingMarkersUnderWords()

       if (!list)
         continue;
       bool removed_markers = false;
-      for (size_t j = list->size(); j > 0; --j) {
-        if (should_remove_marker(*list->at(j - 1),
+
+      HeapVector<Member<RenderedDocumentMarker>> markers_in_list =
+          list->GetMarkers();
+      for (size_t j = markers_in_list.size(); j > 0; --j) {
+        if (should_remove_marker(*markers_in_list[j - 1],
                                  static_cast<const Text&>(node))) {
-          list->erase(j - 1);
+          markers_in_list.erase(j - 1);
           removed_markers = true;
         }
       }
+
+      list->Clear();
+      for (DocumentMarker* marker : markers_in_list)
+        list->Add(marker);
+
       if (removed_markers &&
           marker_list_index == DocumentMarker::kTextMatchMarkerIndex)
         InvalidatePaintForTickmarks(node);
@@ -567,19 +578,18 @@ void DocumentMarkerController::RemoveMarkersFromList(
   } else {
     MarkerLists* markers = iterator->value.Get();
 
-    for (size_t marker_list_index = 0;
-         marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
-         ++marker_list_index) {
-      Member<MarkerList>& list = (*markers)[marker_list_index];
+    for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) {
+      size_t marker_list_index = MarkerTypeToMarkerIndex(type);
+      DocumentMarkerList* list = (*markers)[marker_list_index];
       if (!list || list->IsEmpty()) {
-        if (list.Get() && list->IsEmpty())
-          list.Clear();
+        if (list && list->IsEmpty())
+          (*markers)[marker_list_index] = nullptr;
         ++empty_lists_count;
         continue;
       }
-      if (marker_types.Contains((*list->begin())->GetType())) {
-        list->clear();
-        list.Clear();
+      if (marker_types.Contains(type)) {
+        list->Clear();
+        ListForType(markers, type) = nullptr;

[Xiaocheng, 2017/04/20 08:41:34]

For consistency, either apply ListForType() at all places, or none of them.

         ++empty_lists_count;
         needs_repainting = true;
       }
@@ -618,12 +628,10 @@ void DocumentMarkerController::RepaintMarkers(
 
     // inner loop: process each marker in the current node
     MarkerLists* markers = i->value.Get();
-    for (size_t marker_list_index = 0;
-         marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
-         ++marker_list_index) {
-      Member<MarkerList>& list = (*markers)[marker_list_index];
-      if (!list || list->IsEmpty() ||
-          !marker_types.Contains((*list->begin())->GetType()))
+    for (DocumentMarker::MarkerType type : DocumentMarker::AllMarkers()) {
+      size_t marker_list_index = MarkerTypeToMarkerIndex(type);
+      DocumentMarkerList* list = (*markers)[marker_list_index];
+      if (!list || list->IsEmpty() || !marker_types.Contains(type))
         continue;
 
       // cause the node to be redrawn
@@ -671,16 +679,19 @@ bool DocumentMarkerController::SetMarkersActive(Node* node,
     return false;
 
   bool doc_dirty = false;
-  Member<MarkerList>& list = ListForType(markers, DocumentMarker::kTextMatch);
+  DocumentMarkerList* list = ListForType(markers, DocumentMarker::kTextMatch);
+
   if (!list)
     return false;
-  MarkerList::iterator start_pos = std::upper_bound(
-      list->begin(), list->end(), start_offset,
-      [](size_t start_offset, const Member<RenderedDocumentMarker>& marker) {
+
+  const HeapVector<Member<RenderedDocumentMarker>>& markers_in_list =
+      list->GetMarkers();
+  const auto start_pos = std::upper_bound(

[Xiaocheng, 2017/04/20 08:41:34]

This impl assumes that the markers are sorted, which is a detail that should be
maintained by DML but not DMC.

Please add a TODO about it.

+      markers_in_list.begin(), markers_in_list.end(), start_offset,
+      [](size_t start_offset, const Member<DocumentMarker>& marker) {
         return start_offset < marker->EndOffset();
       });
-  for (MarkerList::iterator marker = start_pos; marker != list->end();
-       ++marker) {
+  for (auto marker = start_pos; marker != markers_in_list.end(); ++marker) {
     // Markers are returned in order, so stop if we are now past the specified
     // range.
     if ((*marker)->StartOffset() >= end_offset)
@@ -710,10 +721,11 @@ void DocumentMarkerController::ShowMarkers() const {
     for (size_t marker_list_index = 0;
          marker_list_index < DocumentMarker::kMarkerTypeIndexesCount;
          ++marker_list_index) {
-      Member<MarkerList>& list = (*markers)[marker_list_index];
-      for (unsigned marker_index = 0; list.Get() && marker_index < list->size();
-           ++marker_index) {
-        DocumentMarker* marker = list->at(marker_index).Get();
+      DocumentMarkerList* list = (*markers)[marker_list_index];
+
+      const HeapVector<Member<RenderedDocumentMarker>>& markers_in_list =
+          list->GetMarkers();
+      for (const DocumentMarker* marker : markers_in_list) {
         builder.Append(" ");
         builder.AppendNumber(marker->GetType());
         builder.Append(":[");
@@ -746,12 +758,11 @@ void DocumentMarkerController::DidUpdateCharacterData(CharacterData* node,
     return;
 
   bool did_shift_marker = false;
-  for (MarkerList* list : *markers) {
+  for (DocumentMarkerList* list : *markers) {
     if (!list)
       continue;
 
-    if (DocumentMarkerListEditor::ShiftMarkers(list, offset, old_length,
-                                               new_length))
+    if (list->ShiftMarkers(offset, old_length, new_length))
       did_shift_marker = true;
   }