Return EphemeralRange from Editor::findRangeOfString.

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

Index: third_party/WebKit/Source/core/editing/Editor.cpp
diff --git a/third_party/WebKit/Source/core/editing/Editor.cpp b/third_party/WebKit/Source/core/editing/Editor.cpp
index 7a4237d403016eafaa67be28ab835cdf0c97dda9..d7adb368e311025d2fcb12a65c8d815a60ced601 100644
--- a/third_party/WebKit/Source/core/editing/Editor.cpp
+++ b/third_party/WebKit/Source/core/editing/Editor.cpp
@@ -1145,16 +1145,16 @@ void Editor::computeAndSetTypingStyle(StylePropertySet* style, EditAction editin
 
 bool Editor::findString(const String& target, FindOptions options)
 {
-    VisibleSelection selection = frame().selection().selection();
-
-    // TODO(yosin) We should make |findRangeOfString()| to return
-    // |EphemeralRange| rather than|Range| object.
-    RefPtrWillBeRawPtr<Range> resultRange = findRangeOfString(target, EphemeralRange(selection.start(), selection.end()), static_cast<FindOptions>(options | FindAPICall));

[yosin_UTC9, 2015/10/22 02:28:27]

Just FYI. We have usage counters

- https://www.chromestatus.com/metrics/feature/popularity#WindowFind
 calls of window.find()
-
https://www.chromestatus.com/metrics/feature/popularity#WindowFindWithShadowTree
 number of window.find() enters shadow tree

I'll use "WindowFindWithShadowTree" as evidence to make TextIterator composed
tree
version only.

+    const VisibleSelection& selection = frame().selection().selection();
+    const EphemeralRange& resultRange = findRangeOfString(

[yosin_UTC9, 2015/10/22 02:28:27]

Remove |&|. Does clang allow this?

[Andrey Kraynov, 2015/10/22 09:53:20]

It's a valid C++ code for all compilers ;)

http://stackoverflow.com/questions/2784262/does-a-const-reference-prolong-the...
Temporary will live in the scope of const-reference.
RVO is unpredictable and EphemeralRange doesn't have a move-ctor, but we can
avoid creating a copy.

+        target,
+        EphemeralRange(selection.start(), selection.end()),
+        static_cast<FindOptions>(options | FindAPICall));
 
-    if (!resultRange)
+    if (resultRange.isCollapsed())
         return false;
 
-    frame().selection().setSelection(VisibleSelection(EphemeralRange(resultRange.get())));
+    frame().selection().setSelection(VisibleSelection(resultRange));
     frame().selection().revealSelection();
     return true;
 }
@@ -1162,7 +1162,7 @@ bool Editor::findString(const String& target, FindOptions options)
 template <typename Strategy>
 static PassRefPtrWillBeRawPtr<Range> findStringAndScrollToVisibleAlgorithm(Editor& editor, const String& target, const EphemeralRangeTemplate<Strategy>& previousMatch, FindOptions options)
 {
-    RefPtrWillBeRawPtr<Range> nextMatch = editor.findRangeOfString(target, previousMatch, options);
+    RefPtrWillBeRawPtr<Range> nextMatch = createRange(editor.findRangeOfString(target, previousMatch, options));

[yosin_UTC9, 2015/10/22 02:28:28]

Let's return EphmeralRange here and ask call to handle it.
Note: We can't create Range objects which cross tree boundaries, see comments
for EphemeralRangeInComposedTree
version of createRange()

[Andrey Kraynov, 2015/10/30 18:24:25]

Done.

     if (!nextMatch)
         return nullptr;
 
@@ -1179,24 +1179,43 @@ PassRefPtrWillBeRawPtr<Range> Editor::findStringAndScrollToVisible(const String&
     return findStringAndScrollToVisibleAlgorithm<EditingStrategy>(*this, target, EphemeralRange(range), options);
 }
 
-// TODO(yosin) We should return |EphemeralRange| rather than |Range|. We use
-// |Range| object for checking whether start and end position crossing shadow
-// boundaries, however we can do it without |Range| object.
+// This function checks whether start and end position of the |range|
+// crossing shadow boundaries.
+// It should be equivalent to Range::create(ephemeralRange)->collapsed().
 template <typename Strategy>
-static PassRefPtrWillBeRawPtr<Range> findStringBetweenPositions(const String& target, const EphemeralRangeTemplate<Strategy>& referenceRange, FindOptions options)
+static bool rangeBoundariesAreValid(const EphemeralRangeTemplate<Strategy>& range)
 {
-    EphemeralRangeTemplate<Strategy> searchRange(referenceRange);
+    const Node* endRootContainer = range.endPosition().computeContainerNode();
+    const Node* startRootContainer = range.startPosition().computeContainerNode();
+    if (!startRootContainer || !endRootContainer)
+        return false;
+
+    while (endRootContainer->parentNode())

[yosin_UTC9, 2015/10/22 02:28:28]

Can we use Node::commonAncestor()?

Why not position.compareTo() enough?

[Andrey Kraynov, 2015/10/22 09:53:20]

I looked at the checkForDifferentRootContainer() function and tried to keep
close to it.
checkForDifferentRootContainer checks for nodes common ancestor and then calls
Range::compareBoundaryPoints, which in turn calls comparePositionsInDOMTree from
EditingUtilities.
But Position::compareTo() just calls comparePositionsInDOMTree.

Maybe common ancestor check is a some kind of optimisation for early return?
How do you think, can we safely skip this check?
It seems that comparePositionsInDOMTree will try to find a commonAncestor and
return 0 if it doesn't exist.

[yosin_UTC9, 2015/10/23 01:52:50]

I have no idea when we have a range which start and end have no common ancestor.
One of start and end is orphaned?
If we're not sure, having ASSERT(commonAncestor(start, end)) is enough.

[Andrey Kraynov, 2015/10/30 18:24:25]

Done.

+        endRootContainer = endRootContainer->parentNode();
+
+    while (startRootContainer->parentNode())
+        startRootContainer = startRootContainer->parentNode();
+
+    return startRootContainer == endRootContainer
+        && (range.startPosition().compareTo(range.endPosition()) <= 0);
+}
+
+template <typename Strategy>
+static EphemeralRangeTemplate<Strategy> findStringBetweenPositions(const String& target, const EphemeralRangeTemplate<Strategy>& referenceRange, FindOptions options)
+{
+    using EphemeralRangeT = EphemeralRangeTemplate<Strategy>;

[yosin_UTC9, 2015/10/22 02:28:27]

We don't have rule about "using alias". In our experience, it is hard to 
use consistent alias name across files and different alias names readers
confusing.

Since Blink doesn't limit length of line, at this time ;-), long original 
type name is preferred.

[Andrey Kraynov, 2015/10/22 09:53:20]

OK, I'll revert these changes.
But sometimes it's not easy to read Blink code ;(

[yosin_UTC9, 2015/10/23 01:52:50]

You're not alone. I think so too.
I'm voting to use 80-char per line like Chromium.

[Andrey Kraynov, 2015/10/30 18:24:25]

Done.

+
+    EphemeralRangeT searchRange(referenceRange);
 
     bool forward = !(options & Backwards);
 
     while (true) {
-        EphemeralRangeTemplate<Strategy> resultRange = findPlainText(searchRange, target, options);
+        const EphemeralRangeT& resultRange = findPlainText(searchRange, target, options);

[yosin_UTC9, 2015/10/22 02:28:27]

Remove |&|. I think returned object lives on stack or registers.

         if (resultRange.isCollapsed())
-            return nullptr;
+            return EphemeralRangeT();
 
-        RefPtrWillBeRawPtr<Range> rangeObject = Range::create(resultRange.document(), toPositionInDOMTree(resultRange.startPosition()), toPositionInDOMTree(resultRange.endPosition()));
-        if (!rangeObject->collapsed())
-            return rangeObject.release();
+        if (rangeBoundariesAreValid(resultRange))
+            return resultRange;
 
         // Found text spans over multiple TreeScopes. Since it's impossible to
         // return such section as a Range, we skip this match and seek for the
@@ -1205,72 +1224,74 @@ static PassRefPtrWillBeRawPtr<Range> findStringBetweenPositions(const String& ta
         if (forward) {
             // TODO(yosin) We should use |PositionMoveType::Character|
             // for |nextPositionOf()|.
-            searchRange = EphemeralRangeTemplate<Strategy>(nextPositionOf(resultRange.startPosition(), PositionMoveType::CodePoint), searchRange.endPosition());
+            searchRange = EphemeralRangeT(nextPositionOf(resultRange.startPosition(), PositionMoveType::CodePoint), searchRange.endPosition());
         } else {
             // TODO(yosin) We should use |PositionMoveType::Character|
             // for |previousPositionOf()|.
-            searchRange = EphemeralRangeTemplate<Strategy>(searchRange.startPosition(), previousPositionOf(resultRange.endPosition(), PositionMoveType::CodePoint));
+            searchRange = EphemeralRangeT(searchRange.startPosition(), previousPositionOf(resultRange.endPosition(), PositionMoveType::CodePoint));
         }
     }
 
     ASSERT_NOT_REACHED();
-    return nullptr;
+    return EphemeralRangeT();
 }
 
 template <typename Strategy>
-static PassRefPtrWillBeRawPtr<Range> findRangeOfStringAlgorithm(Document& document, const String& target, const EphemeralRangeTemplate<Strategy>& referenceRange, FindOptions options)
+static EphemeralRangeTemplate<Strategy> findRangeOfStringAlgorithm(const String& target, const EphemeralRangeTemplate<Strategy>& referenceRange, FindOptions options)
 {
+    using EphemeralRangeT = EphemeralRangeTemplate<Strategy>;
+
     if (target.isEmpty())
-        return nullptr;
+        return EphemeralRangeT();
 
     // Start from an edge of the reference range. Which edge is used depends on
     // whether we're searching forward or backward, and whether startInSelection
     // is set.
-    EphemeralRangeTemplate<Strategy> documentRange = EphemeralRangeTemplate<Strategy>::rangeOfContents(document);
-    EphemeralRangeTemplate<Strategy> searchRange(documentRange);
+    EphemeralRangeT documentRange = EphemeralRangeT::rangeOfContents(referenceRange.document());
+    EphemeralRangeT searchRange(documentRange);
 
     bool forward = !(options & Backwards);
     bool startInReferenceRange = false;
     if (referenceRange.isNotNull()) {
         startInReferenceRange = options & StartInSelection;
         if (forward && startInReferenceRange)
-            searchRange = EphemeralRangeTemplate<Strategy>(referenceRange.startPosition(), documentRange.endPosition());
+            searchRange = EphemeralRangeT(referenceRange.startPosition(), documentRange.endPosition());
         else if (forward)
-            searchRange = EphemeralRangeTemplate<Strategy>(referenceRange.endPosition(), documentRange.endPosition());
+            searchRange = EphemeralRangeT(referenceRange.endPosition(), documentRange.endPosition());
         else if (startInReferenceRange)
-            searchRange = EphemeralRangeTemplate<Strategy>(documentRange.startPosition(), referenceRange.endPosition());
+            searchRange = EphemeralRangeT(documentRange.startPosition(), referenceRange.endPosition());
         else
-            searchRange = EphemeralRangeTemplate<Strategy>(documentRange.startPosition(), referenceRange.startPosition());
+            searchRange = EphemeralRangeT(documentRange.startPosition(), referenceRange.startPosition());
     }
 
-    RefPtrWillBeRawPtr<Range> resultRange = findStringBetweenPositions(target, searchRange, options);
+    EphemeralRangeT resultRange = findStringBetweenPositions(target, searchRange, options);
 
     // If we started in the reference range and the found range exactly matches
     // the reference range, find again. Build a selection with the found range
     // to remove collapsed whitespace. Compare ranges instead of selection
     // objects to ignore the way that the current selection was made.
-    if (resultRange && startInReferenceRange && normalizeRange(EphemeralRangeTemplate<Strategy>(resultRange.get())) == referenceRange) {
+    if (!resultRange.isCollapsed() && startInReferenceRange && normalizeRange(resultRange) == referenceRange) {
         if (forward)
-            searchRange = EphemeralRangeTemplate<Strategy>(fromPositionInDOMTree<Strategy>(resultRange->endPosition()), searchRange.endPosition());
+            searchRange = EphemeralRangeT(resultRange.endPosition(), searchRange.endPosition());
         else
-            searchRange = EphemeralRangeTemplate<Strategy>(searchRange.startPosition(), fromPositionInDOMTree<Strategy>(resultRange->startPosition()));
+            searchRange = EphemeralRangeT(searchRange.startPosition(), resultRange.startPosition());
         resultRange = findStringBetweenPositions(target, searchRange, options);
     }
 
-    if (!resultRange && options & WrapAround)
+    if (resultRange.isCollapsed() && options & WrapAround)
         return findStringBetweenPositions(target, documentRange, options);
 
-    return resultRange.release();
+    return resultRange;
 }
 
-PassRefPtrWillBeRawPtr<Range> Editor::findRangeOfString(const String& target, const EphemeralRange& reference, FindOptions options)
+EphemeralRange Editor::findRangeOfString(const String& target, const EphemeralRange& reference, FindOptions options)
 {
-    return findRangeOfStringAlgorithm<EditingStrategy>(*frame().document(), target, reference, options);
+    return findRangeOfStringAlgorithm<EditingStrategy>(target, reference, options);
 }
 
-PassRefPtrWillBeRawPtr<Range> Editor::findRangeOfString(const String& target, const EphemeralRangeInComposedTree& reference, FindOptions options)
+EphemeralRangeInComposedTree Editor::findRangeOfString(const String& target, const EphemeralRangeInComposedTree& reference, FindOptions options)
 {
-    return findRangeOfStringAlgorithm<EditingInComposedTreeStrategy>(*frame().document(), target, reference, options);
+    return findRangeOfStringAlgorithm<EditingInComposedTreeStrategy>(target, reference, options);
 }
 
 void Editor::setMarkedTextMatchesAreHighlighted(bool flag)