Implementing directional selection strategy in Blink.

Source/core/editing/FrameSelection.cpp

 /*
  * Copyright (C) 2004, 2008, 2009, 2010 Apple 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:
  * 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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 75 matching lines @@
 static inline LayoutUnit NoXPosForVerticalArrowNavigation()
 {
     return LayoutUnit::min();
 }
 
 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())
     , m_observingVisibleSelection(false)
     , m_granularity(CharacterGranularity)
     , m_caretBlinkTimer(this, &FrameSelection::caretBlinkTimerFired)
     , m_caretRectDirty(true)
     , m_shouldPaintCaret(true)
     , m_isCaretBlinkingSuspended(false)
     , m_focused(frame && frame->page() && frame->page()->focusController().focusedFrame() == frame)
@@ skipping 109 matching lines @@
 
     newSelection.setIsDirectional(isDirectional); // Adjusting base and extent will make newSelection always directional
     if (m_selection == newSelection)
         return;
 
     setSelection(newSelection, granularity);
 }
 
 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);
 
     if (!m_frame) {
         m_selection = s;
         return;
     }
 
     // <http://bugs.webkit.org/show_bug.cgi?id=23464>: Infinite recursion at FrameSelection::setSelection
     // if document->frame() == m_frame we can get into an infinite loop
@@ skipping 926 matching lines @@
         m_xPosForVerticalArrowNavigation = x;
     } else
         x = m_xPosForVerticalArrowNavigation;
 
     return x;
 }
 
 void FrameSelection::clear()
 {
     m_granularity = CharacterGranularity;
+    granularityStrategy()->Clear();
     setSelection(VisibleSelection());
 }
 
 void FrameSelection::prepareForDestruction()
 {
     m_granularity = CharacterGranularity;
 
     m_caretBlinkTimer.stop();
 
     LayoutView* view = m_frame->contentRenderer();
@@ skipping 720 matching lines @@
         String text = plainText(start.deepEquivalent(), end.deepEquivalent());
         if (!text.isEmpty() && !isSeparator(text.characterStartingAt(0))) {
             setSelection(VisibleSelection(start, end), WordGranularity);
             return true;
         }
     }
 
     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())
+    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
+        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;
 
-    const VisiblePosition basePosition = m_selection.isBaseFirst() ? m_selection.visibleStart() : m_selection.visibleEnd();
-    VisibleSelection newSelection(basePosition, extentPosition);
-    if (newSelection.isBaseFirst())
-        newSelection.setEndRespectingGranularity(granularity);
-    else
-        newSelection.setStartRespectingGranularity(granularity);
-    if (!newSelection.isRange())
-        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)
 {
     VisibleSelection newSelection(basePosition, extentPosition);
     newSelection.expandUsingGranularity(granularity);
 
     if (newSelection.isNone())
         return;
 
     setSelection(newSelection, granularity);
 }
 
 }
 
 #ifndef NDEBUG
 
 void showTree(const blink::FrameSelection& sel)
 {
     sel.showTreeForThis();
 }
 
 void showTree(const blink::FrameSelection* sel)
 {
     if (sel)
         sel->showTreeForThis();
 }
 
 #endif