[DMC #1] Refactor DocumentMarkerController on top of new DocumentMarkerListEditor class

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 ad1da82cbb6413d7efc0093cb387a2629e542d0a..b3269732d96d7fd13d85256ff9f64740eb2d9870 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
@@ -209,6 +209,25 @@ static void UpdateMarkerRenderedRect(const Node& node,
   range->Dispose();
 }
 
+// TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
+void DocumentMarkerListEditor::AddMarker(MarkerList* list,
+                                         const DocumentMarker* marker) {
+  RenderedDocumentMarker* rendered_marker =
+      RenderedDocumentMarker::Create(*marker);
+  if (list->IsEmpty() || list->back()->EndOffset() < marker->StartOffset()) {
+    list->push_back(rendered_marker);
+  } else {
+    if (marker->GetType() != DocumentMarker::kTextMatch &&
+        marker->GetType() != DocumentMarker::kComposition) {
+      MergeOverlapping(list, rendered_marker);
+    } else {
+      MarkerList::iterator pos =
+          std::lower_bound(list->begin(), list->end(), marker, StartsFurther);
+      list->insert(pos - list->begin(), rendered_marker);
+    }
+  }
+}
+
 // Markers are stored in order sorted by their start offset.
 // Markers of the same type do not overlap each other.
 
@@ -234,20 +253,7 @@ void DocumentMarkerController::AddMarker(Node* node,
   }
 
   Member<MarkerList>& list = markers->at(marker_list_index);
-  RenderedDocumentMarker* new_rendered_marker =

[yosin_UTC9, 2017/04/14 04:14:22]

Please implement DocumentMarkerListEditor::AddMarker() here, if so patch can be:

+   DocumentMarkerListEditor::AddMarker(list, &new_marker);
+ }
+
+  DocumentMarkerListEditor::AddMarker(MarkerList* list,
                                         const DocumentMarker* marker) {

[Xiaocheng, 2017/04/14 04:37:54]

There are some other code in DMC::AddMarker after the call site of
DMLEditor::AddMarker, so the code can't look like that...

-      RenderedDocumentMarker::Create(new_marker);
-  if (list->IsEmpty() || list->back()->EndOffset() < new_marker.StartOffset()) {
-    list->push_back(new_rendered_marker);
-  } else {
-    if (new_marker.GetType() != DocumentMarker::kTextMatch &&
-        new_marker.GetType() != DocumentMarker::kComposition) {
-      MergeOverlapping(list.Get(), new_rendered_marker);
-    } else {
-      MarkerList::iterator pos = std::lower_bound(list->begin(), list->end(),
-                                                  &new_marker, StartsFurther);
-      list->insert(pos - list->begin(), new_rendered_marker);
-    }
-  }
+  DocumentMarkerListEditor::AddMarker(list, &new_marker);
 
   // repaint the affected node
   if (node->GetLayoutObject()) {
@@ -256,7 +262,8 @@ void DocumentMarkerController::AddMarker(Node* node,
   }
 }
 
-void DocumentMarkerController::MergeOverlapping(
+// TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
+void DocumentMarkerListEditor::MergeOverlapping(
     MarkerList* list,
     RenderedDocumentMarker* to_insert) {
   MarkerList::iterator first_overlapping =
@@ -275,6 +282,34 @@ void DocumentMarkerController::MergeOverlapping(
   }
 }
 
+// TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
+bool DocumentMarkerListEditor::MoveMarkers(MarkerList* src_list,
+                                           int length,
+                                           MarkerList* dst_list) {
+  DCHECK_GT(length, 0);
+  bool didMoveMarker = false;
+  unsigned end_offset = length - 1;
+
+  MarkerList::iterator it;
+  for (it = src_list->begin(); it != src_list->end(); ++it) {
+    DocumentMarker& marker = **it;
+    if (marker.StartOffset() > end_offset)
+      break;
+
+    // pin the marker to the specified range and apply the shift delta
+    if (marker.EndOffset() > end_offset)
+      marker.SetEndOffset(end_offset);
+
+    DocumentMarkerListEditor::AddMarker(dst_list, &marker);
+    didMoveMarker = true;
+  }
+
+  // Remove the range of markers that were moved to dstNode
+  src_list->erase(0, it - src_list->begin());
+
+  return didMoveMarker;
+}
+
 // Moves markers from src_node to dst_node. Markers are moved if their start
 // offset is less than length. Markers that run past that point are truncated.
 void DocumentMarkerController::MoveMarkers(Node* src_node,
@@ -287,34 +322,31 @@ void DocumentMarkerController::MoveMarkers(Node* src_node,
     return;
   DCHECK(!markers_.IsEmpty());
 
-  MarkerLists* markers = markers_.at(src_node);
-  if (!markers)
+  MarkerLists* src_markers = markers_.at(src_node);
+  if (!src_markers)
     return;
 
-  bool doc_dirty = false;
-  for (Member<MarkerList> list : *markers) {
-    if (!list)
-      continue;
-
-    unsigned end_offset = length - 1;
-    MarkerList::iterator it;
-    for (it = list->begin(); it != list->end(); ++it) {
-      DocumentMarker* marker = it->Get();
+  if (!markers_.Contains(dst_node)) {
+    markers_.insert(dst_node,
+                    new MarkerLists(DocumentMarker::kMarkerTypeIndexesCount));
+  }
 
-      // stop if we are now past the specified range
-      if (marker->StartOffset() > end_offset)
-        break;
+  MarkerLists* dst_markers = markers_.at(dst_node);
 
-      // pin the marker to the specified range
-      doc_dirty = true;
-      if (marker->EndOffset() > end_offset)
-        marker->SetEndOffset(end_offset);
+  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);
+    if (!src_list)
+      continue;
 
-      AddMarker(dst_node, *marker);
+    if (!dst_markers->at(marker_list_index)) {
+      dst_markers->at(marker_list_index) = new MarkerList;
     }
 
-    // Remove the range of markers that were moved to dstNode
-    list->erase(0, it - list->begin());
+    if (DocumentMarkerListEditor::MoveMarkers(
+            src_list, length, dst_markers->at(marker_list_index)))
+      doc_dirty = true;
   }
 
   // repaint the affected node
@@ -324,6 +356,29 @@ void DocumentMarkerController::MoveMarkers(Node* src_node,
   }
 }
 
+// TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
+bool DocumentMarkerListEditor::RemoveMarkers(MarkerList* list,
+                                             unsigned start_offset,
+                                             int length) {
+  bool doc_dirty = false;
+  unsigned end_offset = start_offset + length;
+  MarkerList::iterator start_pos =
+      std::upper_bound(list->begin(), list->end(), start_offset, EndsBefore);
+  for (MarkerList::iterator i = start_pos; i != list->end();) {
+    DocumentMarker marker(*i->Get());
+
+    // markers are returned in order, so stop if we are now past the specified
+    // range
+    if (marker.StartOffset() >= end_offset)
+      break;
+
+    list->erase(i - list->begin());
+    doc_dirty = true;
+  }
+
+  return doc_dirty;
+}
+
 void DocumentMarkerController::RemoveMarkers(
     Node* node,
     unsigned start_offset,
@@ -354,20 +409,9 @@ void DocumentMarkerController::RemoveMarkers(
     }
     if (!marker_types.Contains((*list->begin())->GetType()))
       continue;
-    unsigned end_offset = start_offset + length;
-    MarkerList::iterator start_pos =
-        std::upper_bound(list->begin(), list->end(), start_offset, EndsBefore);
-    for (MarkerList::iterator i = start_pos; i != list->end();) {
-      DocumentMarker marker(*i->Get());
-
-      // markers are returned in order, so stop if we are now past the specified
-      // range
-      if (marker.StartOffset() >= end_offset)
-        break;
 
-      list->erase(i - list->begin());
+    if (DocumentMarkerListEditor::RemoveMarkers(list, start_offset, length))
       doc_dirty = true;
-    }
 
     if (list->IsEmpty()) {
       list.Clear();
@@ -711,7 +755,9 @@ bool DocumentMarkerController::SetMarkersActive(const EphemeralRange& range,
   for (Node& node : range.Nodes()) {
     int start_offset = node == start_container ? container_start_offset : 0;
     int end_offset = node == end_container ? container_end_offset : INT_MAX;
-    marker_found |= SetMarkersActive(&node, start_offset, end_offset, active);
+
+    if (SetMarkersActive(&node, start_offset, end_offset, active))
+      marker_found = true;
   }
   return marker_found;
 }
@@ -784,6 +830,34 @@ void DocumentMarkerController::ShowMarkers() const {
 }
 #endif
 
+// TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
+bool DocumentMarkerListEditor::ShiftMarkers(MarkerList* list,
+                                            unsigned offset,
+                                            unsigned old_length,
+                                            unsigned new_length) {
+  bool did_shift_marker = false;
+  for (MarkerList::iterator it = list->begin(); it != list->end(); ++it) {
+    RenderedDocumentMarker& marker = **it;
+    Optional<DocumentMarker::MarkerOffsets> result =
+        marker.ComputeOffsetsAfterShift(offset, old_length, new_length);
+    if (result == WTF::kNullopt) {
+      list->erase(it - list->begin());
+      --it;
+      did_shift_marker = true;
+      continue;
+    }
+
+    if (marker.StartOffset() != result.value().start_offset ||
+        marker.EndOffset() != result.value().end_offset) {
+      did_shift_marker = true;
+      marker.SetStartOffset(result.value().start_offset);
+      marker.SetEndOffset(result.value().end_offset);
+    }
+  }
+
+  return did_shift_marker;
+}
+
 // SynchronousMutationObserver
 void DocumentMarkerController::DidUpdateCharacterData(CharacterData* node,
                                                       unsigned offset,
@@ -802,24 +876,8 @@ void DocumentMarkerController::DidUpdateCharacterData(CharacterData* node,
     if (!list)
       continue;
 
-    for (MarkerList::iterator it = list->begin(); it != list->end(); ++it) {
-      RenderedDocumentMarker& marker = **it;
-      Optional<DocumentMarker::MarkerOffsets> result =
-          marker.ComputeOffsetsAfterShift(offset, old_length, new_length);
-      if (result == WTF::kNullopt) {
-        list->erase(it - list->begin());
-        --it;
-        did_shift_marker = true;
-        continue;
-      }
-
-      if (marker.StartOffset() != result.value().start_offset ||
-          marker.EndOffset() != result.value().end_offset) {
-        did_shift_marker = true;
-        marker.SetStartOffset(result.value().start_offset);
-        marker.SetEndOffset(result.value().end_offset);
-      }
-    }
+    did_shift_marker |= DocumentMarkerListEditor::ShiftMarkers(

[Xiaocheng, 2017/04/14 22:32:21]

nit: Please change it to

if (DMLE::ShitMarkers())
  did_shift_marker = true;

to unify the coding style.

+        list, offset, old_length, new_length);
   }
 
   if (!did_shift_marker)