Simplify LayoutSelection::CalcSelection()

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

Index: third_party/WebKit/Source/core/editing/LayoutSelection.cpp
diff --git a/third_party/WebKit/Source/core/editing/LayoutSelection.cpp b/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
index 1264e56f10343f4c7234bbb57c73e22940cbff85..5b6cfb5a0df98a0018f8893850d18b0cf513b192 100644
--- a/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
@@ -109,60 +109,28 @@ LayoutSelection::LayoutSelection(FrameSelection& frame_selection)
       has_pending_selection_(false),
       paint_range_(SelectionPaintRange()) {}
 
-static SelectionInFlatTree CalcSelection(
-    const VisibleSelectionInFlatTree& original_selection,
-    bool should_show_blok_cursor) {
-  const PositionInFlatTree& start = original_selection.Start();
-  const PositionInFlatTree& end = original_selection.End();
-  SelectionType selection_type = original_selection.GetSelectionType();
-  const TextAffinity affinity = original_selection.Affinity();
-
-  bool paint_block_cursor =
-      should_show_blok_cursor &&
-      selection_type == SelectionType::kCaretSelection &&
-      !IsLogicalEndOfLine(CreateVisiblePosition(end, affinity));
-  if (EnclosingTextControl(start.ComputeContainerNode())) {
-    // TODO(yosin) We should use |PositionMoveType::CodePoint| to avoid
-    // ending paint at middle of character.
-    PositionInFlatTree end_position =
-        paint_block_cursor ? NextPositionOf(original_selection.Extent(),
-                                            PositionMoveType::kCodeUnit)
-                           : end;
-    return SelectionInFlatTree::Builder()
-        .SetBaseAndExtent(start, end_position)
-        .Build();
-  }
+static VisibleSelectionInFlatTree CalcSelection(
+    const FrameSelection& frame_selection) {
+  const VisibleSelectionInFlatTree& original_selection =
+      frame_selection.ComputeVisibleSelectionInFlatTree();
 
-  const VisiblePositionInFlatTree& visible_start = CreateVisiblePosition(
-      start, selection_type == SelectionType::kRangeSelection
-                 ? TextAffinity::kDownstream
-                 : affinity);
-  if (visible_start.IsNull())
-    return SelectionInFlatTree();
-  if (paint_block_cursor) {
-    const VisiblePositionInFlatTree visible_extent = NextPositionOf(
-        CreateVisiblePosition(end, affinity), kCanSkipOverEditingBoundary);
-    if (visible_extent.IsNull())
-      return SelectionInFlatTree();
-    SelectionInFlatTree::Builder builder;
-    builder.Collapse(visible_start.ToPositionWithAffinity());
-    builder.Extend(visible_extent.DeepEquivalent());
-    return builder.Build();
-  }
-  const VisiblePositionInFlatTree visible_end = CreateVisiblePosition(
-      end, selection_type == SelectionType::kRangeSelection
-               ? TextAffinity::kUpstream
-               : affinity);
-  if (visible_end.IsNull())
-    return SelectionInFlatTree();
-  SelectionInFlatTree::Builder builder;
-  builder.Collapse(visible_start.ToPositionWithAffinity());
-  builder.Extend(visible_end.DeepEquivalent());
-  return builder.Build();
+  const bool paint_block_cursor =

[yosin_UTC9, 2017/06/23 03:53:50]

Could you move this expression into function?

[yoichio, 2017/06/23 04:27:15]

I don't understand. This expression is used only here and 
 enough short to understand.

[yosin_UTC9, 2017/06/23 04:46:11]

It is noisy for reading code focusing CalcSelection().
In that time, we don't care about details of ShouldShowBlockCursor().
Multiple lines of conditional expression is good candidate of introducing
function.
ShouldShowBlockCursor() could be written as

bool ShouldShowBlockCursor(const FrameSelection& frame_selection, const
VisibleSelectionInFlatTree& visible_selection) {
  if (!frame_selection.ShouldShowBLockCursor())
    return false;
  if (!visible_selection.IsCaret())
    return false;
  return !IsLogicalEndOfLine(CreateVisiblePosition(visible_selection.Start(),
visible_selection.Affinity()));
}

[yoichio, 2017/06/23 05:01:01]

O.K. Done.

+      frame_selection.ShouldShowBlockCursor() &&
+      original_selection.GetSelectionType() == SelectionType::kCaretSelection &&

[yosin_UTC9, 2017/06/23 04:46:11]

original_selection.IsCaret()

+      !IsLogicalEndOfLine(CreateVisiblePosition(original_selection.End(),
+                                                original_selection.Affinity()));
+  if (!paint_block_cursor)
+    return original_selection;
+
+  // TODO(editing-dev): We should consider BIDI.

[yosin_UTC9, 2017/06/23 03:53:50]

Could you explain more about this?
NextPositionOf() can work bidi, it returns logical next position.

[yoichio, 2017/06/23 04:27:15]

Understood. This works already.

+  const PositionInFlatTree end_position = NextPositionOf(
+      original_selection.Extent(), PositionMoveType::kGraphemeCluster);
+  return CreateVisibleSelection(
+      SelectionInFlatTree::Builder()
+          .SetBaseAndExtent(original_selection.Start(), end_position)
+          .Build());
 }
 
-
-
 // Objects each have a single selection rect to examine.
 using SelectedObjectMap = HashMap<LayoutObject*, SelectionState>;
 // Blocks contain selected objects and fill gaps between them, either on the
@@ -324,17 +292,7 @@ static SelectionPaintRange CalcSelectionPaintRange(
   if (selection_in_dom.IsNone())
     return SelectionPaintRange();
 
-  const VisibleSelectionInFlatTree& original_selection =
-      frame_selection.ComputeVisibleSelectionInFlatTree();
-  // Construct a new VisibleSolution, since visibleSelection() is not
-  // necessarily valid, and the following steps assume a valid selection. See
-  // <https://bugs.webkit.org/show_bug.cgi?id=69563> and
-  // <rdar://problem/10232866>.
-  const SelectionInFlatTree& new_selection = CalcSelection(
-      original_selection, frame_selection.ShouldShowBlockCursor());
-  const VisibleSelectionInFlatTree& selection =
-      CreateVisibleSelection(new_selection);
-
+  const VisibleSelectionInFlatTree& selection = CalcSelection(frame_selection);
   if (!selection.IsRange() || frame_selection.IsHidden())
     return SelectionPaintRange();