Return EphemeralRange from Editor::findRangeOfString.

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 1127 matching lines @@
         ASSERT(frame().document());
         ApplyStyleCommand::create(*frame().document(), blockStyle.get(), editingAction)->apply();
     }
 
     // Set the remaining style as the typing style.
     frame().selection().setTypingStyle(typingStyle);
 }
 
 bool Editor::findString(const String& target, FindOptions options)
 {
-    VisibleSelection selection = frame().selection().selection();
+    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));
 
-    // 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.

-
-    if (!resultRange)
+    if (resultRange.isCollapsed())
         return false;
 
-    frame().selection().setSelection(VisibleSelection(EphemeralRange(resultRange.get())));
+    frame().selection().setSelection(VisibleSelection(resultRange));
     frame().selection().revealSelection();
     return true;
 }
 
 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;
 
     nextMatch->firstNode()->layoutObject()->scrollRectToVisible(LayoutRect(nextMatch->boundingBox()),
         ScrollAlignment::alignCenterIfNeeded, ScrollAlignment::alignCenterIfNeeded, UserScroll);
 
     return nextMatch.release();
 }
 
 PassRefPtrWillBeRawPtr<Range> Editor::findStringAndScrollToVisible(const String& target, Range* range, FindOptions options)
 {
     if (RuntimeEnabledFeatures::selectionForComposedTreeEnabled())
         return findStringAndScrollToVisibleAlgorithm<EditingInComposedTreeStrategy>(*this, target, EphemeralRangeInComposedTree(range), options);
     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
         // next occurrence.
         // TODO(yosin) Handle this case.
         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)
 {
     if (flag == m_areMarkedTextMatchesHighlighted)
         return;
 
     m_areMarkedTextMatchesHighlighted = flag;
     frame().document()->markers().repaintMarkers(DocumentMarker::TextMatch);
 }
@@ skipping 17 matching lines @@
 }
 
 DEFINE_TRACE(Editor)
 {
     visitor->trace(m_frame);
     visitor->trace(m_lastEditCommand);
     visitor->trace(m_mark);
 }
 
 } // namespace blink