Improving direction-based selection strategy.

Source/core/editing/GranularityStrategy.h

Index: Source/core/editing/GranularityStrategy.h
diff --git a/Source/core/editing/GranularityStrategy.h b/Source/core/editing/GranularityStrategy.h
index 9d31bc84eb0ad41f48f7a04ea2fa49f2e994b75c..18f21c78829fbd541c6a83ed89f1d78a902cf3fb 100644
--- a/Source/core/editing/GranularityStrategy.h
+++ b/Source/core/editing/GranularityStrategy.h
@@ -16,9 +16,9 @@ public:
     virtual SelectionStrategy GetType() const = 0;
     virtual void Clear() = 0;
 
-    // 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&) = 0;
+    // Calculates and returns the new selection based on the updated extent
+    // location in contents-space coordinates.
+    virtual VisibleSelection updateExtent(const IntPoint&, LocalFrame*) = 0;
 
 protected:
     GranularityStrategy();
@@ -33,13 +33,41 @@ public:
     // GranularityStrategy:
     SelectionStrategy GetType() const final;
     void Clear() final;
-    VisibleSelection updateExtent(const VisiblePosition& extentPosition, const VisibleSelection&) final;
+    VisibleSelection updateExtent(const IntPoint&, LocalFrame*) final;
 };
 
 // "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".
+// In word granularity, the word is not selected until the extent passes the
+// middle of the word. If the selection is shrunk when there is a distance
+// between the extent and the selection end on extent's side of the base, then
+// this distance is applied to the extent as an offset for the purpose of
+// determining the selection bound. This prevents the selection edge from
+// "jumping" back to extent position when it is shrunk after extending. This
+// offset can be reduced or set to 0 if the extent is moved in the opposite
+// direction.
+//
+// Example:
+// ^ marks base, | marks extent passed in updateExtent, > marks selection end:
+// Lorem ip^sum|> dolor sit amet, consectetur
+//
+// Move extent over the middle of "dolor", granularity should change to word
+// granularity and the selection end should jump to the end of the word.
+// Lorem ip^sum dolo|r> sit amet, consectetur
+//
+// Move extent back one character. Granularity changes to "character". The
+// selection end should move back one character as well. Note an offset between
+// the extent and the selection end.
+// Lorem ip^sum dol|o>r sit amet, consectetur
+//
+// Move extent forward one character. The offset is reduced to 0. Selection end
+// doesn't change.
+// Lorem ip^sum dolo|>r sit amet, consectetur
+//
+// Move forward one character. End moves with extent in character granularity.
+// Lorem ip^sum dolor|> sit amet, consectetur
 class DirectionGranularityStrategy final : public GranularityStrategy {
 public:
     DirectionGranularityStrategy();
@@ -48,23 +76,57 @@ public:
     // GranularityStrategy:
     SelectionStrategy GetType() const final;
     void Clear() final;
-    VisibleSelection updateExtent(const VisiblePosition&, const VisibleSelection&) final;
+    VisibleSelection updateExtent(const IntPoint&, LocalFrame*) final;
 
 private:
+    enum class StrategyState {
+        // Starting state.
+        // Selection was cleared and there were no extent updates since then.
+        // One an update is performed, the strategy goes into the Expanding
+        // state unless the upate shrinks the selection without changing
+        // relative base/extent order, in which case the strategy goes into the
+        // Shrinking state.
+        Cleared,
+        // Last time the selection was changed by updateExtent - it was expanded
+        // or the relative base/extent order was changed.
+        Expanding,
+        // Last time the selection was changed by updateExtent - it was shrunk
+        // (without changing relative base/extent order).
+        Shrinking
+    };
     enum class BoundAdjust {CurrentPosIfOnBound, NextBoundIfOnBound};
     enum class SearchDirection {SearchBackwards, SearchForward};
 
+    // We use the bottom-left corner of the caret rect to represent the
+    // location of a VisiblePosition. This way locations corresponding to
+    // VisiblePositions on the same line will all have the same y coordinate
+    // unless the text is transformed.
+    static IntPoint positionLocation(const VisiblePosition& vp) { return vp.absoluteCaretBounds().minXMaxYCorner(); }

[leviw_travelin_and_unemployed, 2015/06/03 22:29:48]

Why not put this in the cpp file?

[mfomitchev, 2015/06/05 17:38:39]

Ok. I've put this and a couple of other methods in an anonymous namespace in the
cpp file. This is the pattern typically used in chromium in cases when a method
doesn't use any of the class's state.

+
+    // Order is specified using the same contract as comparePositions.
+    bool arePositionsInSpecifiedOrder(const VisiblePosition& vp1, const VisiblePosition& vp2, int specifiedOrder);

[leviw_travelin_and_unemployed, 2015/06/03 22:29:48]

Nit: you don't need the 'vp1' and 'vp2' names here.

[mfomitchev, 2015/06/05 17:38:39]

Done.

+
     // 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*/, SearchDirection /*direction*/, BoundAdjust /*nextIfOnBound*/);
 
-    // Current selection granularity being used
+    StrategyState m_state;
+
+    // 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;
+
+    // Horizontal offset in pixels in content space applied to the extent point.

[leviw_travelin_and_unemployed, 2015/06/03 22:29:48]

You use "content space" repeatedly in this CL, but generally it seems like
you're using what in blink we refer to as absolute coordinates (hence
absoluteCaretBounds() and LayoutObject::localToAbsolute()).

[mfomitchev, 2015/06/05 17:38:39]

Done..
I was trying to use the terminology established here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

+    int m_offset;
+
+    // Vector defining location of the extent point relative to the location of
+    // extent's VisiblePosition.
+    // The start of the vector is the bottom-left of the caret rect of the

[leviw_travelin_and_unemployed, 2015/06/03 22:29:48]

A couple things:
- Again I don't think you should call this a vector.
- Saying this has a start and end makes it sound like there's both a start and
and offset in this, but really it's just a width and height. This comment seems
misleading to me.

[mfomitchev, 2015/06/03 22:36:55]

If I call it Euclidean vector, or geometric vector, or spatial vector, would
that be okay? And then say initial point and terminal point instead of start and
end?

[mfomitchev, 2015/06/05 17:38:39]

Done.

+    // VisiblePosition corresponding to the selection extent. The end of
+    // the vector is the extent point (from the last updateExtent call) shifted
+    // horizontally by |m_offset| pixels.
+    IntSize m_subPositionCorrection;
 };
 
 } // namespace blink