Remove DocumentMarkerController::MarkersInRange()

third_party/WebKit/Source/core/editing/SelectionController.cpp

Index: third_party/WebKit/Source/core/editing/SelectionController.cpp
diff --git a/third_party/WebKit/Source/core/editing/SelectionController.cpp b/third_party/WebKit/Source/core/editing/SelectionController.cpp
index ac77e75ae8a58515ef8d1d34c7b81d5f84a3035f..de5c7459f6b5198cb8d96adcd1d71bce893f8d3a 100644
--- a/third_party/WebKit/Source/core/editing/SelectionController.cpp
+++ b/third_party/WebKit/Source/core/editing/SelectionController.cpp
@@ -519,12 +519,11 @@ void SelectionController::SelectClosestMisspellingFromHitTestResult(
   if (pos.IsNotNull()) {
     const PositionInFlatTree& marker_position =
         pos.DeepEquivalent().ParentAnchoredEquivalent();
+    Node* container_node = marker_position.ComputeContainerNode();
     DocumentMarkerVector markers =
-        inner_node->GetDocument().Markers().MarkersInRange(
-            EphemeralRange(ToPositionInDOMTree(marker_position)),
-            DocumentMarker::MisspellingMarkers());
+        inner_node->GetDocument().Markers().MarkersFor(
+            container_node, DocumentMarker::MisspellingMarkers());
     if (markers.size() == 1) {

[Xiaocheng, 2017/05/09 21:55:15]

This can break something if there are multiple markers on |container_node| but
only one covers |marker_position|. Same applies to all other call sites.

Maybe we need a helper function that filters out markers in an offset range from
a DocumentMarkerVector?

[rlanday, 2017/05/09 22:04:17]

This seems like something we should push back to DocumentMarkerList so we can do
it more efficiently (e.g. since the list is sorted, we don't need to copy out
every single marker, only the ones in the applicable range). But once we do
that, it starts to seem like we should leave MarkersInRange() on DMC after all
and just fix the API somehow.

yosin: what do you think?

Note that the current implementation of MarkersInRange() is kind of ridiculously
inefficient; it calls MarkersFor() to get the full list of markers for each node
without passing the specified marker types, then filters out the ones for the
other types *afterward*...

[yosin_UTC9, 2017/05/10 01:46:59]

Good catch! MarkesForNode() isn't equivalent to MakersAtPosition().

DocumentMarkerVector MakersAtPosition(const Position& position,
DocumentMakrerTypes mrker_type) {
  ...
  for (const auto& marker marker : MakresFor(position.ComputeContainerNode())) {
    if (marker contains position.ComputeOffsetInContainer())
      add to result
  }
  ...
}

We may want to introduce DMList::MarkersAtPosition() to utilized sorted list
impl in another patch.

It seems introducing DMC::MakersAtPosition() makes this patch smaller since we
don't need to
call Position::ComputeContainerNode() in each call site.

-      Node* container_node = marker_position.ComputeContainerNode();
       const PositionInFlatTree start(container_node, markers[0]->StartOffset());
       const PositionInFlatTree end(container_node, markers[0]->EndOffset());
       new_selection = CreateVisibleSelection(
@@ -1018,12 +1017,12 @@ static bool HitTestResultIsMisspelled(const HitTestResult& result) {
       inner_node->GetLayoutObject()->PositionForPoint(result.LocalPoint()));
   if (pos.IsNull())
     return false;
+  const Position& marker_position =
+      pos.DeepEquivalent().ParentAnchoredEquivalent();
+  Node* container_node = marker_position.ComputeContainerNode();
   return inner_node->GetDocument()
              .Markers()
-             .MarkersInRange(
-                 EphemeralRange(
-                     pos.DeepEquivalent().ParentAnchoredEquivalent()),
-                 DocumentMarker::MisspellingMarkers())
+             .MarkersFor(container_node, DocumentMarker::MisspellingMarkers())
              .size() > 0;
 }