Improve how DocumentMarkerController updates markers in response to text edits

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 f76a22b37cb8f1d9ffce213a976438a412427ee2..db6261f2268911111e07f22f0fdf839b71f7fb82 100644
--- a/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
+++ b/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
@@ -177,11 +177,6 @@ static bool startsFurther(const Member<RenderedDocumentMarker>& lhv,
   return lhv->startOffset() < rhv->startOffset();
 }
 
-static bool startsAfter(const Member<RenderedDocumentMarker>& marker,
-                        size_t startOffset) {
-  return marker->startOffset() < startOffset;
-}
-
 static bool endsBefore(size_t startOffset,
                        const Member<RenderedDocumentMarker>& rhv) {
   return startOffset < rhv->endOffset();
@@ -309,10 +304,7 @@ void DocumentMarkerController::copyMarkers(Node* srcNode,
     return;
 
   bool docDirty = false;
-  for (size_t markerListIndex = 0;
-       markerListIndex < DocumentMarker::MarkerTypeIndexesCount;
-       ++markerListIndex) {
-    Member<MarkerList>& list = (*markers)[markerListIndex];
+  for (Member<MarkerList> list : *markers) {
     if (!list)
       continue;
 
@@ -654,6 +646,49 @@ void DocumentMarkerController::removeMarkers(
   m_possiblyExistingMarkerTypes.remove(markerTypes);
 }
 
+bool DocumentMarkerController::relocateMarkerIfNeeded(
+    RenderedDocumentMarker* marker,
+    unsigned offset,
+    unsigned oldLength,
+    unsigned newLength) {
+  bool didShiftMarker = false;
+
+  // algorithm inspired by https://dom.spec.whatwg.org/#concept-cd-replace
+  // but with some changes
+  if (marker->startOffset() > offset) {

[rlanday, 2017/03/23 22:52:00]

I think there's one more tweak we want to make: as written, if an insertion
happens right where a marker starts, the new text will be included in the
marker. E.g. if we have a marker from 5 to 10 and do an insert with offset 5,
oldLength 0, and newLength 5, the new marker will be from 5 to 15 instead of 10
to 15. We can fix this by writing >= here instead of >.

+    // Deviation from the concept-cd-replace algorithm: < instead of <= in
+    // the next line
+    if (marker->startOffset() < offset + oldLength) {

[rlanday, 2017/03/23 22:52:00]

Since we're moving the marker start to the end of the new text, it doesn't
matter if this is <= or < (offset + newLength == marker->startOffset() +
newLength - oldLength if marker->startOffset() == offset + oldLength), so I'm
going to change this back to <= to be consistent with the concept-cd-replace
algorithm

+      // Marker start was in the replaced text. Move to end of new text
+      // (Deviation from the concept-cd-replace algorithm: that algorithm
+      // would move to the beginning of the new text here)
+      marker->setStartOffset(offset + newLength);
+    } else {
+      // Marker start was after the replaced text. Shift by length
+      // difference
+      unsigned oldStartOffset = marker->startOffset();
+      marker->setStartOffset(oldStartOffset + newLength - oldLength);
+    }
+    didShiftMarker = true;
+  }
+
+  if (marker->endOffset() > offset) {
+    // Deviation from the concept-cd-replace algorithm: < instead of <= in
+    // the next line
+    if (marker->endOffset() < offset + oldLength) {
+      // Marker end was in the replaced text. Move to beginning of new text
+      marker->setEndOffset(offset);
+    } else {
+      // Marker end was after the replaced text. Shift by length difference
+      unsigned oldEndOffset = marker->endOffset();
+      marker->setEndOffset(oldEndOffset + newLength - oldLength);
+    }
+    didShiftMarker = true;
+  }
+
+  return didShiftMarker;
+}
+
 void DocumentMarkerController::removeMarkersFromList(
     MarkerMap::iterator iterator,
     DocumentMarker::MarkerTypes markerTypes) {
@@ -736,47 +771,6 @@ void DocumentMarkerController::repaintMarkers(
   }
 }
 
-void DocumentMarkerController::shiftMarkers(Node* node,
-                                            unsigned startOffset,
-                                            int delta) {
-  if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))
-    return;
-  DCHECK(!m_markers.isEmpty());
-
-  MarkerLists* markers = m_markers.at(node);
-  if (!markers)
-    return;
-
-  bool didShiftMarker = false;
-  for (size_t markerListIndex = 0;
-       markerListIndex < DocumentMarker::MarkerTypeIndexesCount;
-       ++markerListIndex) {
-    Member<MarkerList>& list = (*markers)[markerListIndex];
-    if (!list)
-      continue;
-    MarkerList::iterator startPos =
-        std::lower_bound(list->begin(), list->end(), startOffset, startsAfter);
-    for (MarkerList::iterator marker = startPos; marker != list->end();
-         ++marker) {
-#if DCHECK_IS_ON()
-      int startOffset = (*marker)->startOffset();
-      DCHECK_GE(startOffset + delta, 0);
-#endif
-      (*marker)->shiftOffsets(delta);
-      didShiftMarker = true;
-    }
-  }
-
-  if (didShiftMarker) {
-    invalidateRectsForMarkersInNode(*node);
-    // repaint the affected node
-    if (node->layoutObject()) {
-      node->layoutObject()->setShouldDoFullPaintInvalidation(
-          PaintInvalidationDocumentMarkerChange);
-    }
-  }
-}
-
 bool DocumentMarkerController::setMarkersActive(const EphemeralRange& range,
                                                 bool active) {
   if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))
@@ -876,10 +870,45 @@ void DocumentMarkerController::didUpdateCharacterData(CharacterData* node,
                                                       unsigned offset,
                                                       unsigned oldLength,
                                                       unsigned newLength) {
-  // Shift markers as if we first remove the old text, then insert the new text
-  removeMarkers(node, offset, oldLength);
-  shiftMarkers(node, offset + oldLength, 0 - oldLength);
-  shiftMarkers(node, offset, newLength);
+  // If we're doing a pure remove operation, remove the markers in the range
+  // being removed (markers containing, but larger than, the range, will be
+  // split)
+  if (newLength == 0)
+    removeMarkers(node, offset, oldLength);
+
+  if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))
+    return;
+  DCHECK(!m_markers.isEmpty());
+
+  MarkerLists* markers = m_markers.at(node);
+  if (!markers)
+    return;
+
+  bool didShiftMarker = false;
+  for (MarkerList* list : *markers) {
+    if (!list)
+      continue;
+
+    for (MarkerList::iterator it = list->begin(); it != list->end(); ++it) {
+      RenderedDocumentMarker& marker = **it;
+      if (relocateMarkerIfNeeded(&marker, offset, oldLength, newLength)) {
+        didShiftMarker = true;
+
+        if (marker.startOffset() < marker.endOffset())
+          continue;
+        list->remove(it - list->begin());
+        --it;
+      }
+    }
+  }
+
+  if (!didShiftMarker)
+    return;
+  if (!node->layoutObject())
+    return;
+  invalidateRectsForMarkersInNode(*node);
+  // repaint the affected node
+  node->layoutObject()->setShouldDoFullPaintInvalidation();
 }
 
 }  // namespace blink