Refactor DocumentMarkerController::Add*Marker() methods

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 123 matching lines @@
   AddSpellCheckMarker(start, end, DocumentMarker::kGrammar, description);
 }
 
 void DocumentMarkerController::AddSpellCheckMarker(
     const Position& start,
     const Position& end,
     DocumentMarker::MarkerType type,
     const String& description) {
   DCHECK(type == DocumentMarker::kSpelling || type == DocumentMarker::kGrammar)
       << type;
-  // Use a TextIterator to visit the potentially multiple nodes the range
-  // covers.
-  for (TextIterator marked_text(start, end); !marked_text.AtEnd();
-       marked_text.Advance()) {
-    AddMarker(marked_text.CurrentContainer(),
-              new DocumentMarker(
-                  type, marked_text.StartOffsetInCurrentContainer(),
-                  marked_text.EndOffsetInCurrentContainer(), description));
-  }
+  AddMarkerInternal(EphemeralRange(start, end),
+                    [type, &description](int start_offset, int end_offset) {
+                      return new DocumentMarker(type, start_offset, end_offset,
+                                                description);
+                    });
 }
 
 void DocumentMarkerController::AddTextMatchMarker(
     const EphemeralRange& range,
     DocumentMarker::MatchStatus match_status) {
   DCHECK(!document_->NeedsLayoutTreeUpdate());
-
-  // Use a TextIterator to visit the potentially multiple nodes the range
-  // covers.
-  for (TextIterator marked_text(range.StartPosition(), range.EndPosition());
-       !marked_text.AtEnd(); marked_text.Advance()) {
-    AddMarker(marked_text.CurrentContainer(),
-              new DocumentMarker(marked_text.StartOffsetInCurrentContainer(),
-                                 marked_text.EndOffsetInCurrentContainer(),
-                                 match_status));
-  }
+  AddMarkerInternal(range, [match_status](int start_offset, int end_offset) {
+    return new DocumentMarker(start_offset, end_offset, match_status);
+  });
   // Don't invalidate tickmarks here. TextFinder invalidates tickmarks using a
   // throttling algorithm. crbug.com/6819.
 }
 
 void DocumentMarkerController::AddCompositionMarker(const EphemeralRange& range,
                                                     Color underline_color,
                                                     bool thick,
                                                     Color background_color) {
   DCHECK(!document_->NeedsLayoutTreeUpdate());
-
-  for (TextIterator marked_text(range.StartPosition(), range.EndPosition());
-       !marked_text.AtEnd(); marked_text.Advance()) {
-    AddMarker(marked_text.CurrentContainer(),
-              new DocumentMarker(marked_text.StartOffsetInCurrentContainer(),
-                                 marked_text.EndOffsetInCurrentContainer(),
-                                 underline_color, thick, background_color));
-  }
+  AddMarkerInternal(range, [underline_color, thick, background_color](
+                               int start_offset, int end_offset) {
+    return new DocumentMarker(start_offset, end_offset, underline_color, thick,
+                              background_color);
+  });
 }
 
 void DocumentMarkerController::PrepareForDestruction() {
   Clear();
 }
 
 void DocumentMarkerController::RemoveMarkers(
     TextIterator& marked_text,
     DocumentMarker::MarkerTypes marker_types) {
   for (; !marked_text.AtEnd(); marked_text.Advance()) {
@@ skipping 10 matching lines @@
 
 void DocumentMarkerController::RemoveMarkersInRange(
     const EphemeralRange& range,
     DocumentMarker::MarkerTypes marker_types) {
   DCHECK(!document_->NeedsLayoutTreeUpdate());
 
   TextIterator marked_text(range.StartPosition(), range.EndPosition());
   DocumentMarkerController::RemoveMarkers(marked_text, marker_types);
 }
 
+void DocumentMarkerController::AddMarkerInternal(
+    const EphemeralRange& range,
+    std::function<DocumentMarker*(int, int)> create_marker_from_offsets) {
+  for (TextIterator marked_text(range.StartPosition(), range.EndPosition());
+       !marked_text.AtEnd(); marked_text.Advance()) {
+    const int start_offset_in_current_container =
+        marked_text.StartOffsetInCurrentContainer();
+    const int end_offset_in_current_container =
+        marked_text.EndOffsetInCurrentContainer();
+
+    DCHECK_GE(end_offset_in_current_container,
+              start_offset_in_current_container);
+
+    // TODO(editing-dev): TextIterator sometimes emits ranges where the current

[Xiaocheng, 2017/05/30 23:51:19]

Emitting text with non-text container is legit. For example:

<div>foo</div><div>bar</div>

where a "\n" is emitted between the two <div>s.

Could you make the comment mention the equal-offset issue?

[rlanday, 2017/05/30 23:52:27]

It does mention the equal-offset issue; I will update the other part

+    // container isn't a text node, or where the start and end offsets are the
+    // same. Investigate if TextIterator should be changed to not do this. See
+    // crbug.com/727929
+    if (end_offset_in_current_container == start_offset_in_current_container)
+      continue;
+    Node* node = marked_text.CurrentContainer();
+    if (!node->IsTextNode())
+      continue;
+
+    DocumentMarker* const new_marker = create_marker_from_offsets(
+        start_offset_in_current_container, end_offset_in_current_container);
+    AddMarkerToNode(node, new_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,
-                                         DocumentMarker* new_marker) {
-  DCHECK_GE(new_marker->EndOffset(), new_marker->StartOffset());
-  if (new_marker->EndOffset() == new_marker->StartOffset())
-    return;
-
+void DocumentMarkerController::AddMarkerToNode(Node* node,
+                                               DocumentMarker* new_marker) {
   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);
   }
 
   const DocumentMarker::MarkerType new_marker_type = new_marker->GetType();
@@ skipping 473 matching lines @@
 }
 
 }  // namespace blink
 
 #ifndef NDEBUG
 void showDocumentMarkers(const blink::DocumentMarkerController* controller) {
   if (controller)
     controller->ShowMarkers();
 }
 #endif