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

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

 /*
  * Copyright (C) 2004, 2005, 2006, 2007, 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
@@ skipping 39 matching lines @@
                                      const VisibleSelection& selection,
                                      LayoutUnit xPosForVerticalArrowNavigation)
     : m_frame(const_cast<LocalFrame*>(&frame)),
       m_selection(selection),
       m_xPosForVerticalArrowNavigation(xPosForVerticalArrowNavigation) {}
 
 SelectionModifier::SelectionModifier(const LocalFrame& frame,
                                      const VisibleSelection& selection)
     : SelectionModifier(frame, selection, NoXPosForVerticalArrowNavigation()) {}
 
-TextDirection SelectionModifier::directionOfEnclosingBlock() const {
-  return blink::directionOfEnclosingBlock(m_selection.extent());
+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::directionOfSelection() const {
+TextDirection SelectionModifier::directionOfEnclosingBlock() const {
+  return directionOfEnclosingBlockOf(m_selection.extent());
+}
+
+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())
     endBox = computeInlineBoxPosition(endPosition).inlineBox;
   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;
+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);
+}
 
-  Position start = m_selection.start();
-  Position end = m_selection.end();
-
-  bool baseIsStart = true;
-
-  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 {
   Settings* settings = frame() ? frame()->settings() : nullptr;
   if (settings && settings->getEditingBehaviorType() == EditingMacBehavior)
     return isGetStart ? m_selection.visibleStart() : m_selection.visibleEnd();
   // Linux and Windows always extend selections from the extent endpoint.
   // FIXME: VisibleSelection should be fixed to ensure as an invariant that
   // base/extent always point to the same nodes as start/end, but which points
   // to which depends on the value of isBaseFirst. Then this can be changed
@@ skipping 447 matching lines @@
   selection->setExtent(newStart);
 }
 
 bool SelectionModifier::modify(EAlteration alter,
                                SelectionDirection direction,
                                TextGranularity granularity) {
   DCHECK(!frame()->document()->needsLayoutTreeUpdate());
   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();
   VisiblePosition position;
   switch (direction) {
     case DirectionRight:
       if (alter == FrameSelection::AlterationMove)
         position = modifyMovingRight(granularity);
       else
         position = modifyExtendingRight(granularity);
@@ skipping 107 matching lines @@
 bool SelectionModifier::modifyWithPageGranularity(EAlteration alter,
                                                   unsigned verticalDistance,
                                                   VerticalDirection direction) {
   if (!verticalDistance)
     return false;
 
   DCHECK(!frame()->document()->needsLayoutTreeUpdate());
   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;
   switch (alter) {
     case FrameSelection::AlterationMove:
       pos = createVisiblePosition(direction == FrameSelection::DirectionUp
                                       ? m_selection.start()
                                       : m_selection.end(),
                                   m_selection.affinity());
       xPos = lineDirectionPointForBlockDirectionNavigation(
@@ skipping 126 matching lines @@
     x = lineDirectionPointForBlockDirectionNavigationOf(visiblePosition);
     m_xPosForVerticalArrowNavigation = x;
   } else {
     x = m_xPosForVerticalArrowNavigation;
   }
 
   return x;
 }
 
 }  // namespace blink