Remove DocumentMarkerController::MarkersInRange()

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights
  * reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)
  * Copyright (C) 2015 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
@@ skipping 501 matching lines @@
   Node* inner_node = result.InnerNode();
   VisibleSelectionInFlatTree new_selection;
 
   if (!inner_node || !inner_node->GetLayoutObject())
     return;
 
   const VisiblePositionInFlatTree& pos = VisiblePositionOfHitTestResult(result);
   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(
           SelectionInFlatTree::Builder().Collapse(start).Extend(end).Build());
     }
   }
 
   if (append_trailing_whitespace == AppendTrailingWhitespace::kShouldAppend)
     new_selection.AppendTrailingWhitespace();
 
@@ skipping 473 matching lines @@
 }
 
 static bool HitTestResultIsMisspelled(const HitTestResult& result) {
   Node* inner_node = result.InnerNode();
   if (!inner_node || !inner_node->GetLayoutObject())
     return false;
   VisiblePosition pos = CreateVisiblePosition(
       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;
 }
 
 void SelectionController::SendContextMenuEvent(
     const MouseEventWithHitTestResults& mev,
     const LayoutPoint& position) {
   if (!Selection().IsAvailable())
     return;
   if (Selection().Contains(position) || mev.GetScrollbar() ||
       // FIXME: In the editable case, word selection sometimes selects content
@@ skipping 97 matching lines @@
 
 bool IsExtendingSelection(const MouseEventWithHitTestResults& event) {
   bool is_mouse_down_on_link_or_image =
       event.IsOverLink() || event.GetHitTestResult().GetImage();
   return (event.Event().GetModifiers() & WebInputEvent::Modifiers::kShiftKey) !=
              0 &&
          !is_mouse_down_on_link_or_image;
 }
 
 }  // namespace blink