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

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 191 matching lines @@
   if (!exception_state.HadException()) {
     // TODO(yosin): Once we have a |EphemeralRange| version of |boundingBox()|,
     // we should use it instead of |Range| version.
     marker.SetRenderedRect(LayoutRect(range->BoundingBox()));
   } else {
     marker.NullifyRenderedRect();
   }
   range->Dispose();
 }
 
+// TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
+void DocumentMarkerListEditor::MergeOverlapping(

[Xiaocheng, 2017/04/14 00:48:06]

Could you put it at the same place as DMC::MergeOverlapping to ease the
comparison?

[yosin_UTC9, 2017/04/14 01:21:14]

Please to follow xiaochengh@'s comment. The purpose of introducing
|DMListEditor|
for ease of comparison in review.

+    MarkerList* list,
+    RenderedDocumentMarker* to_insert) {
+  MarkerList::iterator first_overlapping =
+      std::lower_bound(list->begin(), list->end(), to_insert, DoesNotOverlap);
+  size_t index = first_overlapping - list->begin();
+  list->insert(index, to_insert);
+  MarkerList::iterator inserted = list->begin() + index;
+  first_overlapping = inserted + 1;
+  for (MarkerList::iterator i = first_overlapping;
+       i != list->end() && (*i)->StartOffset() <= (*inserted)->EndOffset();) {
+    (*inserted)->SetStartOffset(
+        std::min((*inserted)->StartOffset(), (*i)->StartOffset()));
+    (*inserted)->SetEndOffset(
+        std::max((*inserted)->EndOffset(), (*i)->EndOffset()));
+    list->erase(i - list->begin());
+  }
+}
+
+// TODO(rlanday): move DocumentMarkerListEditor into its own .h/.cpp files
+void DocumentMarkerListEditor::AddMarker(MarkerList* list,
+                                         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.
 
 void DocumentMarkerController::AddMarker(Node* node,
                                          const 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());
 
   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;
   }
 
   Member<MarkerList>& list = markers->at(marker_list_index);
   RenderedDocumentMarker* new_rendered_marker =

[Xiaocheng, 2017/04/14 00:48:06]

No need to create RDM here. It's already done in DMLEditor::AddMarker()

       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_rendered_marker);
 
   // repaint the affected node
   if (node->GetLayoutObject()) {
     node->GetLayoutObject()->SetShouldDoFullPaintInvalidation(
         kPaintInvalidationDocumentMarkerChange);
   }
 }
 
-void DocumentMarkerController::MergeOverlapping(
-    MarkerList* list,
-    RenderedDocumentMarker* to_insert) {
-  MarkerList::iterator first_overlapping =
-      std::lower_bound(list->begin(), list->end(), to_insert, DoesNotOverlap);
-  size_t index = first_overlapping - list->begin();
-  list->insert(index, to_insert);
-  MarkerList::iterator inserted = list->begin() + index;
-  first_overlapping = inserted + 1;
-  for (MarkerList::iterator i = first_overlapping;
-       i != list->end() && (*i)->StartOffset() <= (*inserted)->EndOffset();) {
-    (*inserted)->SetStartOffset(
-        std::min((*inserted)->StartOffset(), (*i)->StartOffset()));
-    (*inserted)->SetEndOffset(
-        std::max((*inserted)->EndOffset(), (*i)->EndOffset()));
-    list->erase(i - list->begin());
+// 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,
                                            int length,
                                            Node* dst_node) {
   if (length <= 0)
     return;
 
   if (!PossiblyHasMarkers(DocumentMarker::AllMarkers()))
     return;
   DCHECK(!markers_.IsEmpty());
 
-  MarkerLists* markers = markers_.at(src_node);
-  if (!markers)
+  MarkerLists* src_markers = markers_.at(src_node);
+  if (!src_markers)
     return;
 
+  Member<MarkerLists>& dst_markers =

[Xiaocheng, 2017/04/14 00:48:06]

Could you make the code less hacky?

if (!markers_.Contains(dst_node))
  markers.insert(dst_node, new MarkerLists(DM::kMarkerTypeIndexesCount));
MarkerLists* dst_markers = markers_.at(dst_node);

+      markers_.insert(dst_node, nullptr).stored_value->value;
+  if (!dst_markers) {
+    dst_markers = new MarkerLists;
+    dst_markers->Grow(DocumentMarker::kMarkerTypeIndexesCount);
+  }
+
   bool doc_dirty = false;
-  for (Member<MarkerList> list : *markers) {
-    if (!list)
+  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;
 
-    unsigned end_offset = length - 1;
-    MarkerList::iterator it;
-    for (it = list->begin(); it != list->end(); ++it) {
-      DocumentMarker* marker = it->Get();
-
-      // stop if we are now past the specified range
-      if (marker->StartOffset() > end_offset)
-        break;
-
-      // pin the marker to the specified range
-      doc_dirty = true;
-      if (marker->EndOffset() > end_offset)
-        marker->SetEndOffset(end_offset);
-
-      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());
+    DocumentMarkerListEditor::MoveMarkers(src_list, length,

[Xiaocheng, 2017/04/14 00:48:06]

doc_dirty |= DMLEditor::MoveMarkers(...)

+                                          dst_markers->at(marker_list_index));
   }
 
   // repaint the affected node
   if (doc_dirty && dst_node->GetLayoutObject()) {
     dst_node->GetLayoutObject()->SetShouldDoFullPaintInvalidation(
         kPaintInvalidationDocumentMarkerChange);
   }
 }
 
+// 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,
     int length,
     DocumentMarker::MarkerTypes marker_types) {
   if (length <= 0)
     return;
 
   if (!PossiblyHasMarkers(marker_types))
     return;
@@ skipping 10 matching lines @@
        ++marker_list_index) {
     Member<MarkerList>& list = (*markers)[marker_list_index];
     if (!list || list->IsEmpty()) {
       if (list.Get() && list->IsEmpty())
         list.Clear();
       ++empty_lists_count;
       continue;
     }
     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());
-      doc_dirty = true;
-    }
+    doc_dirty =

[Xiaocheng, 2017/04/14 00:48:06]

nit: We can write doc_dirty |= DMLEditor::RemoveMarkers() to save a line.

[rlanday, 2017/04/14 01:01:16]

I don't think so, I think that would be the same as

doc_dirty = doc_dirty | DMLEditor::RemoveMarkers()

which would not do the correct thing because of short-circuit evaluation (it
wouldn't call RemoveMarkers() if doc_dirty were already true)

[yosin_UTC9, 2017/04/14 01:21:14]

foo |= bar() always call |bar()| regard less value of |foo|.

I recommend to write:

if (DocumentMarkerListEditor::RemoveMarkers(list, start_offset, length))
  doc_dirty = true;

This is easier to read.

+        DocumentMarkerListEditor::RemoveMarkers(list, start_offset, length) ||
+        doc_dirty;
 
     if (list->IsEmpty()) {
       list.Clear();
       ++empty_lists_count;
     }
   }
 
   if (empty_lists_count == DocumentMarker::kMarkerTypeIndexesCount) {
     markers_.erase(node);
     if (markers_.IsEmpty())
@@ skipping 284 matching lines @@
     return;
   DCHECK(!markers_.IsEmpty());
 
   // outer loop: process each markered node in the document
   MarkerMap::iterator end = markers_.end();
   for (MarkerMap::iterator i = markers_.begin(); i != end; ++i) {
     const Node* node = i->key;
 
     // 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()) {

[Xiaocheng, 2017/04/14 00:48:06]

Irrelevant change.

Btw, marker type iterators haven't been applied anywhere. I think there should
be a dedicated patch applying these iterators in existing code.

+      size_t marker_list_index = MarkerTypeToMarkerIndex(type);
+      MarkerList* list = (*markers)[marker_list_index];
+      if (!list || list->IsEmpty() || !marker_types.Contains(type))
         continue;
 
       // cause the node to be redrawn
       if (LayoutObject* layout_object = node->GetLayoutObject()) {
         layout_object->SetShouldDoFullPaintInvalidation(
             kPaintInvalidationDocumentMarkerChange);
         break;
       }
     }
   }
@@ skipping 86 matching lines @@
         builder.Append(")");
       }
     }
     builder.Append("\n");
   }
   LOG(INFO) << markers_.size() << " nodes have markers:\n"
             << builder.ToString().Utf8().Data();
 }
 #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,
                                                       unsigned old_length,
                                                       unsigned new_length) {
   if (!PossiblyHasMarkers(DocumentMarker::AllMarkers()))
     return;
   DCHECK(!markers_.IsEmpty());
 
   MarkerLists* markers = markers_.at(node);
   if (!markers)
     return;
 
   bool did_shift_marker = false;
   for (MarkerList* list : *markers) {
     if (!list)
       continue;
 
-    for (MarkerList::iterator it = list->begin(); it != list->end(); ++it) {

[yosin_UTC9, 2017/04/14 01:21:14]

Please put |DMListEditor::ShiftMarkers()| here.

[rlanday, 2017/04/14 01:29:37]

Sorry, I don't understand what you're asking here, the implementation for
ShiftMarkers() is immediately above this one

-      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 00:48:06]

nit: We can write doc_dirty |= DMLEditor::ShiftMarkers() to save a line.

+                           list, offset, old_length, new_length) ||
+                       did_shift_marker;
   }
 
   if (!did_shift_marker)
     return;
   if (!node->GetLayoutObject())
     return;
   InvalidateRectsForMarkersInNode(*node);
   // repaint the affected node
   node->GetLayoutObject()->SetShouldDoFullPaintInvalidation();
 }
 
 }  // namespace blink
 
 #ifndef NDEBUG
 void showDocumentMarkers(const blink::DocumentMarkerController* controller) {
   if (controller)
     controller->ShowMarkers();
 }
 #endif