Implementing directional selection strategy in Blink.

Source/core/editing/FrameSelection.cpp

Index: Source/core/editing/FrameSelection.cpp
diff --git a/Source/core/editing/FrameSelection.cpp b/Source/core/editing/FrameSelection.cpp
index 0cb682f643ce648cd3916e4093ffbb688e47085f..98fffaf70a05642d9f0f956543fa44b36cd105bf 100644
--- a/Source/core/editing/FrameSelection.cpp
+++ b/Source/core/editing/FrameSelection.cpp
@@ -93,6 +93,126 @@ static inline bool shouldAlwaysUseDirectionalSelection(LocalFrame* frame)
     return !frame || frame->editor().behavior().shouldConsiderSelectionAsDirectional();
 }
 
+class GranularityStrategy {
+public:
+    virtual ~GranularityStrategy() { };
+    virtual void Clear();
+
+    // Calculates and returns the new selection based on the updated user selection extent |extentPosition| and the granularity strategy.
+    virtual VisibleSelection updateExtent(const VisiblePosition& extentPosition, const VisibleSelection&);
+protected:
+    GranularityStrategy() { };
+};
+
+class DefaultGranularityStrategy : public GranularityStrategy {
+public:
+    DefaultGranularityStrategy() { };
+    ~DefaultGranularityStrategy() override { };
+
+    void Clear() override { };
+    VisibleSelection updateExtent(const VisiblePosition& extentPosition, const VisibleSelection& selection) override
+    {
+        return VisibleSelection(selection.visibleBase(), extentPosition);
+    };
+};
+
+// "Expand by word, shrink by character" selection strategy.
+// Uses character granularity when selection is shrinking. If the selection is expanding,
+// granularity doesn't change until a word boundary is passed, after which the granularity
+// switches to "word".
+class DirectionGranularityStrategy : public GranularityStrategy {
+public:
+    DirectionGranularityStrategy();
+    ~DirectionGranularityStrategy() override { };
+
+    void Clear() override;
+    VisibleSelection updateExtent(const VisiblePosition&, const VisibleSelection&) override;
+private:
+    // Returns the next word boundary starting from |pos|. |direction| specifies the direction
+    // in which to search for the next bound. nextIfOnBound controls whether |pos| or the next boundary
+    // is returned when |pos| is located exactly on word boundary.
+    VisiblePosition nextWordBound(const VisiblePosition& /*pos*/, int /*direction*/, bool /*nextIfOnBound*/);

[yosin_UTC9, 2015/04/16 04:11:26]

nit: Could you use enum for |nextIfOnBound|? In Blink, we prefer enum over bool
for parameter.

[yosin_UTC9, 2015/04/16 04:11:26]

nit: It is better to use |SelectionDirection|, defined in "VisibleSelection.h",
rather than |int|.

[mfomitchev1, 2015/04/17 00:13:50]

Done.

[mfomitchev1, 2015/04/17 00:13:51]

I created a local ESearchDirection instead. SelectionDirection ahs 4 values, and
we only need two (DirectionForward, DirectionBackward).
DirectionRight/DirectionLeft are also non-trivial to support, since we don't
have RTL information readily available from the strategy object. I think
supporting them would make the code more convoluted.

+
+    // Current selection granularity being used
+    TextGranularity m_granularity;
+    // Set to true if the selection was shrunk (without changing relative base/extent order)
+    // as a result of the most recent updateExtent call.
+    bool m_lastMoveShrunkSelection;
+};
+
+DirectionGranularityStrategy::DirectionGranularityStrategy()
+    : m_granularity(CharacterGranularity)
+    , m_lastMoveShrunkSelection(false) { }
+
+void DirectionGranularityStrategy::Clear()
+{
+    m_granularity = CharacterGranularity;
+    m_lastMoveShrunkSelection = false;
+}
+
+VisiblePosition DirectionGranularityStrategy::nextWordBound(
+    const VisiblePosition& pos,
+    int direction,
+    bool nextIfOnBound)
+{
+    return direction > 0 ? endOfWord(pos, nextIfOnBound ? RightWordIfOnBoundary : LeftWordIfOnBoundary)
+        : startOfWord(pos, nextIfOnBound ? LeftWordIfOnBoundary : RightWordIfOnBoundary);
+}
+
+VisibleSelection DirectionGranularityStrategy::updateExtent(const VisiblePosition& extentPosition, const VisibleSelection& selection)
+{
+    if (extentPosition == selection.visibleExtent())
+        return selection;
+
+    const VisiblePosition& base = selection.visibleBase();

[yosin_UTC9, 2015/04/16 04:11:26]

nit: It is better to use |VisiblePostion| as value rather than reference, since
|VisibleSelection::visibleBase()| returns |VisiblePosition| as value.

[mfomitchev1, 2015/04/17 00:13:51]

Done.

+    const VisiblePosition& oldExtentWithGranularity = selection.isBaseFirst() ? selection.visibleEnd() : selection.visibleStart();
+
+    int extentBaseOrder = comparePositions(extentPosition, base);
+    int oldExtentBaseOrder = comparePositions(oldExtentWithGranularity, base);
+    ASSERT(extentBaseOrder != 0);

[yosin_UTC9, 2015/04/16 04:11:26]

I'm not sure this assertion is always satisfied.

[mfomitchev1, 2015/04/17 00:13:51]

I think it is - we short-circuit in FrameSelection::moveRangeSelectionExtent if
base == extentPosition. That said, the algorithm should work fine regardless, so
I have removed the ASSERT.

+
+    bool extentBaseOrderSwitched = extentBaseOrder * oldExtentBaseOrder < 0;

[yosin_UTC9, 2015/04/16 04:11:26]

This is too smart conditional expression. Could you write explicitly?

[mfomitchev1, 2015/04/17 00:13:51]

Done.

+
+    // Determine the boundary of the 'current word', i.e. the boundary extending beyond which
+    // should change the granularity to WordGranularity.
+    // If the last move has shrunk the selection and is now exactly on the word boundary -
+    // we need to take the next bound as the bound of the "current word".
+    VisiblePosition currentWordBoundary =
+        nextWordBound(oldExtentWithGranularity, oldExtentBaseOrder, m_lastMoveShrunkSelection);
+
+    bool thisMoveShrunkSelection = extentBaseOrder * comparePositions(extentPosition, selection.visibleExtent()) < 0;

[yosin_UTC9, 2015/04/16 04:11:26]

ditto as L174.

[mfomitchev1, 2015/04/17 00:13:50]

Done.

+    // If the extent-base order was switched, then the selection is now expanding in a different
+    // direction than before. Therefore we need to calculate the boundary of the 'current word'
+    // in this new direction in order to be able to tell if the selection expanded beyond it.
+    if (extentBaseOrderSwitched) {
+        currentWordBoundary = nextWordBound(base, extentBaseOrder, true);
+        m_granularity = CharacterGranularity;
+        // When the base/extent order switches it doesn't count as shrinking selection.
+        thisMoveShrunkSelection = false;
+    }
+
+    bool expandedBeyondWordBoundary = extentBaseOrder > 0 ? comparePositions(extentPosition, currentWordBoundary) > 0

[yosin_UTC9, 2015/04/16 04:11:26]

This conditional expression looks complex. It is better to use if-statement or
introducing named function.

[mfomitchev1, 2015/04/17 00:13:51]

Done.

+        : comparePositions(extentPosition, currentWordBoundary) < 0;
+    if (expandedBeyondWordBoundary) {
+        m_granularity = WordGranularity;
+    } else if (thisMoveShrunkSelection) {
+        m_granularity = CharacterGranularity;
+        m_lastMoveShrunkSelection = true;
+    }
+
+    m_lastMoveShrunkSelection = thisMoveShrunkSelection;
+    VisibleSelection newSelection = selection;
+    newSelection.setExtent(extentPosition);
+    if (m_granularity == WordGranularity) {
+        if (extentBaseOrder > 0)
+            newSelection.setEndRespectingGranularity(m_granularity, LeftWordIfOnBoundary);
+        else
+            newSelection.setStartRespectingGranularity(m_granularity, RightWordIfOnBoundary);
+    }
+
+    return newSelection;
+}
+
 FrameSelection::FrameSelection(LocalFrame* frame)
     : m_frame(frame)
     , m_xPosForVerticalArrowNavigation(NoXPosForVerticalArrowNavigation())
@@ -222,11 +342,14 @@ void FrameSelection::setNonDirectionalSelectionIfNeeded(const VisibleSelection&
 
 void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity)
 {
+    if ((options & FrameSelection::DoNotClearStrategy) == 0)
+        granularityStrategy()->Clear();
     bool closeTyping = options & CloseTyping;
     bool shouldClearTypingStyle = options & ClearTypingStyle;
     EUserTriggered userTriggered = selectionOptionsToUserTriggered(options);
 
     VisibleSelection s = validateSelection(newSelection);
+
     if (shouldAlwaysUseDirectionalSelection(m_frame))
         s.setIsDirectional(true);
 
@@ -1173,6 +1296,7 @@ LayoutUnit FrameSelection::lineDirectionPointForBlockDirectionNavigation(EPositi
 void FrameSelection::clear()
 {
     m_granularity = CharacterGranularity;
+    granularityStrategy()->Clear();
     setSelection(VisibleSelection());
 }
 
@@ -1913,21 +2037,33 @@ bool FrameSelection::selectWordAroundPosition(const VisiblePosition& position)
     return false;
 }
 
-void FrameSelection::moveRangeSelectionExtent(const VisiblePosition& extentPosition, TextGranularity granularity)
+GranularityStrategy* const FrameSelection::granularityStrategy()

[yosin_UTC9, 2015/04/16 04:11:26]

nit: omit |const|

[mfomitchev1, 2015/04/17 00:13:51]

Done.

 {
-    if (isNone())
-        return;
-
-    const VisiblePosition basePosition = m_selection.isBaseFirst() ? m_selection.visibleStart() : m_selection.visibleEnd();
-    VisibleSelection newSelection(basePosition, extentPosition);
-    if (newSelection.isBaseFirst())
-        newSelection.setEndRespectingGranularity(granularity);
+    if (m_granularityStrategy)
+        return m_granularityStrategy.get();
+    // We do lazy initalization for m_granularityStrategy, because if we initialize it
+    // right in the constructor - the correct settings may not be set yet.

[Rick Byers, 2015/04/16 19:09:48]

and you're sure granularityStrategy won't ever be called too early?  Is it worth
at least adding an ASSERT above to ensure the type you're returning still
matches the current value of the setting (verifying nobody will try to change
the setting for the lifetime of the frame)?  Or do you just want to support the
strategy changing with the setting?

[mfomitchev1, 2015/04/16 19:33:00]

Yeah, that's a good point. I don't really understand the lifecycle here very
well, so I am not sure. I think it's best to just re-init m_granularityStrategy
to the proper type here if there's a discrepancy... Also, we don't need to init
m_granularityStrategy for Clear() and SetSelection(), so I can make it so
granularityStrategy() is only used by moveRangeSelectionExtent, which should
decrease the likelihood of this called too soon. Does this sound good?

[Rick Byers, 2015/04/16 19:35:10]

Sure, sounds good.

[mfomitchev1, 2015/04/17 00:13:51]

Done.

+    Settings* settings = m_frame ? m_frame->settings() : 0;
+    if (settings && settings->selectionStrategy() == StrategyDirection)
+        m_granularityStrategy = adoptPtr(new DirectionGranularityStrategy());
     else
-        newSelection.setStartRespectingGranularity(granularity);
-    if (!newSelection.isRange())
+        m_granularityStrategy = adoptPtr(new DefaultGranularityStrategy());
+    return m_granularityStrategy.get();
+}
+
+void FrameSelection::moveRangeSelectionExtent(const VisiblePosition& extentPosition)
+{
+    const VisiblePosition base = m_selection.visibleBase();
+
+    if (isNone() || base == extentPosition)
         return;
 
-    setSelection(newSelection, FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle | UserTriggered, FrameSelection::AlignCursorOnScrollIfNeeded, granularity);
+    VisibleSelection newSelection = granularityStrategy()->updateExtent(extentPosition, selection());
+    setSelection(
+        newSelection,
+        FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle | FrameSelection::DoNotClearStrategy | UserTriggered,
+        FrameSelection::AlignCursorOnScrollIfNeeded,
+        CharacterGranularity);
 }
 
 void FrameSelection::moveRangeSelection(const VisiblePosition& basePosition, const VisiblePosition& extentPosition, TextGranularity granularity)