Make SelecitonModifier class not to use VisibleSelection::setBase() and setExtent()

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

Index: third_party/WebKit/Source/core/editing/SelectionModifier.cpp
diff --git a/third_party/WebKit/Source/core/editing/SelectionModifier.cpp b/third_party/WebKit/Source/core/editing/SelectionModifier.cpp
index 3e9af85fc85c7cf74c1537e57aacfc8928ef7995..1de1ba0d75375b280221647633e78d847769bbce 100644
--- a/third_party/WebKit/Source/core/editing/SelectionModifier.cpp
+++ b/third_party/WebKit/Source/core/editing/SelectionModifier.cpp
@@ -57,17 +57,21 @@ SelectionModifier::SelectionModifier(const LocalFrame& frame,
                                      const VisibleSelection& selection)
     : SelectionModifier(frame, selection, NoXPosForVerticalArrowNavigation()) {}
 
+static TextDirection directionOfEnclosingBlockOf(const Position& position) {

[tkent, 2017/02/21 23:34:20]

What's the benefit of this function?

[yosin_UTC9, 2017/02/22 05:11:05]

Oops, this function is useless.
it may be introduced during rewriting to try not using "blink::".

Removing...

+  return directionOfEnclosingBlock(position);
+}
+
 TextDirection SelectionModifier::directionOfEnclosingBlock() const {
-  return blink::directionOfEnclosingBlock(m_selection.extent());
+  return directionOfEnclosingBlockOf(m_selection.extent());
 }
 
-TextDirection SelectionModifier::directionOfSelection() const {
+static TextDirection directionOf(const VisibleSelection& visibleSelection) {
   InlineBox* startBox = nullptr;
   InlineBox* endBox = nullptr;
   // Cache the VisiblePositions because visibleStart() and visibleEnd()
   // can cause layout, which has the potential to invalidate lineboxes.
-  VisiblePosition startPosition = m_selection.visibleStart();
-  VisiblePosition endPosition = m_selection.visibleEnd();
+  const VisiblePosition startPosition = visibleSelection.visibleStart();
+  const VisiblePosition endPosition = visibleSelection.visibleEnd();
   if (startPosition.isNotNull())
     startBox = computeInlineBoxPosition(startPosition).inlineBox;
   if (endPosition.isNotNull())
@@ -75,57 +79,46 @@ TextDirection SelectionModifier::directionOfSelection() const {
   if (startBox && endBox && startBox->direction() == endBox->direction())
     return startBox->direction();
 
-  return directionOfEnclosingBlock();
+  return directionOfEnclosingBlock(visibleSelection.extent());
 }
 
-void SelectionModifier::willBeModified(EAlteration alter,
-                                       SelectionDirection direction) {
-  if (alter != FrameSelection::AlterationExtend)
-    return;
-
-  Position start = m_selection.start();
-  Position end = m_selection.end();
-
-  bool baseIsStart = true;
+TextDirection SelectionModifier::directionOfSelection() const {

[tkent, 2017/02/21 23:34:20]

What's the benefit of this function?

[Xiaocheng, 2017/02/22 01:06:19]

This function seems necessary as it's called by
SelectionModifier::ModifyMovingLeft/Right.

[tkent, 2017/02/22 01:10:20]

Oh, right.  This is an existing function, and this CL updates the
implementation.  I was confused that this was a new function.

[yosin_UTC9, 2017/02/22 05:11:05]

Thanks xiaochengh@ for explanation.

+  return directionOf(m_selection);
+}
 
-  if (m_selection.isDirectional()) {
+static bool isBaseStart(const VisibleSelection& visibleSelection,
+                        SelectionDirection direction) {
+  if (visibleSelection.isDirectional()) {
     // Make base and extent match start and end so we extend the user-visible
     // selection. This only matters for cases where base and extend point to
     // different positions than start and end (e.g. after a double-click to
     // select a word).
-    if (m_selection.isBaseFirst())
-      baseIsStart = true;
-    else
-      baseIsStart = false;
-  } else {
-    switch (direction) {
-      case DirectionRight:
-        if (directionOfSelection() == TextDirection::kLtr)
-          baseIsStart = true;
-        else
-          baseIsStart = false;
-        break;
-      case DirectionForward:
-        baseIsStart = true;
-        break;
-      case DirectionLeft:
-        if (directionOfSelection() == TextDirection::kLtr)
-          baseIsStart = false;
-        else
-          baseIsStart = true;
-        break;
-      case DirectionBackward:
-        baseIsStart = false;
-        break;
-    }
+    return visibleSelection.isBaseFirst();
   }
-  if (baseIsStart) {
-    m_selection.setBase(start);
-    m_selection.setExtent(end);
-  } else {
-    m_selection.setBase(end);
-    m_selection.setExtent(start);
+  switch (direction) {
+    case DirectionRight:
+      return directionOf(visibleSelection) == TextDirection::kLtr;
+    case DirectionForward:
+      return true;
+    case DirectionLeft:
+      return directionOf(visibleSelection) != TextDirection::kLtr;
+    case DirectionBackward:
+      return false;
   }
+  NOTREACHED() << "We should handle " << direction;
+  return true;
+}
+
+static SelectionInDOMTree prepareForExtend(

[tkent, 2017/02/21 23:34:20]

nit: prepareForExtend ->prepareForExtent, prepareForExtension, or
prepareToExtend

However, I feel |prepare| isn't a proper word for this function. 
createExtendingSelectionInDOMTree() or something?

[yosin_UTC9, 2017/02/22 05:11:05]

Done. Rename to |prepareToExtendSeelction()|

+    const VisibleSelection& visibleSelection,
+    SelectionDirection direction) {
+  if (visibleSelection.start().isNull())
+    return visibleSelection.asSelection();
+  const bool baseIsStart = isBaseStart(visibleSelection, direction);
+  return SelectionInDOMTree::Builder(visibleSelection.asSelection())
+      .collapse(baseIsStart ? visibleSelection.start() : visibleSelection.end())
+      .extend(baseIsStart ? visibleSelection.end() : visibleSelection.start())
+      .build();
 }
 
 VisiblePosition SelectionModifier::positionForPlatform(bool isGetStart) const {
@@ -593,7 +586,10 @@ bool SelectionModifier::modify(EAlteration alter,
   DocumentLifecycle::DisallowTransitionScope disallowTransition(
       frame()->document()->lifecycle());
 
-  willBeModified(alter, direction);
+  if (alter == FrameSelection::AlterationExtend) {
+    m_selection =
+        createVisibleSelection(prepareForExtend(m_selection, direction));
+  }
 
   bool wasRange = m_selection.isRange();
   VisiblePosition originalStartPosition = m_selection.visibleStart();
@@ -721,9 +717,12 @@ bool SelectionModifier::modifyWithPageGranularity(EAlteration alter,
   DocumentLifecycle::DisallowTransitionScope disallowTransition(
       frame()->document()->lifecycle());
 
-  willBeModified(alter, direction == FrameSelection::DirectionUp
-                            ? DirectionBackward
-                            : DirectionForward);
+  if (alter == FrameSelection::AlterationExtend) {
+    m_selection = createVisibleSelection(
+        prepareForExtend(m_selection, direction == FrameSelection::DirectionUp
+                                          ? DirectionBackward
+                                          : DirectionForward));
+  }
 
   VisiblePosition pos;
   LayoutUnit xPos;