Simplify LayoutSelection::CalcSelection()

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights
  * reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 91 matching lines @@
 
   current_ = nullptr;
   return *this;
 }
 
 LayoutSelection::LayoutSelection(FrameSelection& frame_selection)
     : frame_selection_(&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();
+static VisibleSelectionInFlatTree CalcSelection(
+    const FrameSelection& frame_selection) {
+  const VisibleSelectionInFlatTree& original_selection =
+      frame_selection.ComputeVisibleSelectionInFlatTree();
 
-  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();
-  }
+  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;
 
-  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();
+  // 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
 // left, right, or in between lines and blocks.
 // In order to get the visual rect right, we have to examine left, middle, and
 // right rects individually, since otherwise the union of those rects might
 // remain the same even when changes have occurred.
 using SelectedBlockMap = HashMap<LayoutBlock*, SelectionState>;
 struct SelectedMap {
   STACK_ALLOCATED();
@@ skipping 141 matching lines @@
   paint_range_ = SelectionPaintRange();
 }
 
 static SelectionPaintRange CalcSelectionPaintRange(
     const FrameSelection& frame_selection) {
   const SelectionInDOMTree& selection_in_dom =
       frame_selection.GetSelectionInDOMTree();
   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();
 
   DCHECK(!selection.IsNone());
   const PositionInFlatTree start_pos = selection.Start();
   const PositionInFlatTree end_pos = selection.End();
   DCHECK_LE(start_pos, end_pos);
   LayoutObject* start_layout_object = start_pos.AnchorNode()->GetLayoutObject();
   LayoutObject* end_layout_object = end_pos.AnchorNode()->GetLayoutObject();
   DCHECK(start_layout_object);
@@ skipping 67 matching lines @@
 
     runner->SetShouldInvalidateSelection();
   }
 }
 
 DEFINE_TRACE(LayoutSelection) {
   visitor->Trace(frame_selection_);
 }
 
 }  // namespace blink