Refactor LayoutSelection::SetSelection()

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 718f97acdc5a45c50f7af4ec522ec518368d968b..6adbf18f95322daf6720f6966c544ede3eab3591 100644
--- a/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
@@ -130,70 +130,42 @@ static inline LayoutObject* GetNextOrPrevLayoutObjectBasedOnDirection(
   return next;
 }
 
-void LayoutSelection::SetSelection(
-    LayoutObject* start,
-    int start_pos,
-    LayoutObject* end,
-    int end_pos,
-    SelectionPaintInvalidationMode block_paint_invalidation_mode) {
-  // This code makes no assumptions as to if the layout tree is up to date or
-  // not and will not try to update it. Currently clearSelection calls this
-  // (intentionally) without updating the layout tree as it doesn't care.
-  // Other callers may want to force recalc style before calling this.
-
-  // Make sure both our start and end objects are defined.
-  // Check www.msnbc.com and try clicking around to find the case where this
-  // happened.
-  if ((start && !end) || (end && !start))
-    return;
-
-  // Just return if the selection hasn't changed.
-  if (selection_start_ == start && selection_start_pos_ == start_pos &&
-      selection_end_ == end && selection_end_pos_ == end_pos)
-    return;
-
-  // Record the old selected objects. These will be used later when we compare
-  // against the new selected objects.
-  int old_start_pos = selection_start_pos_;
-  int old_end_pos = selection_end_pos_;
-
-  // Objects each have a single selection rect to examine.
-  typedef HashMap<LayoutObject*, SelectionState> SelectedObjectMap;
-  SelectedObjectMap old_selected_objects;
-  // FIXME: |newSelectedObjects| doesn't really need to store the
-  // SelectionState, it's just more convenient to have it use the same data
-  // structure as |oldSelectedObjects|.
-  SelectedObjectMap new_selected_objects;
-
-  // 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.
-  typedef HashMap<LayoutBlock*, SelectionState> SelectedBlockMap;
-  SelectedBlockMap old_selected_blocks;
-  // FIXME: |newSelectedBlocks| doesn't really need to store the SelectionState,
-  // it's just more convenient to have it use the same data structure as
-  // |oldSelectedBlocks|.
-  SelectedBlockMap new_selected_blocks;
-
-  LayoutObject* os = selection_start_;
-  LayoutObject* stop =
-      LayoutObjectAfterPosition(selection_end_, selection_end_pos_);
+// Objects each have a single selection rect to examine.
+typedef HashMap<LayoutObject*, SelectionState> SelectedObjectMap;
+// 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.
+typedef HashMap<LayoutBlock*, SelectionState> SelectedBlockMap;
+
+static std::tuple<SelectedObjectMap, SelectedBlockMap> CollectSelectedMap(
+    LayoutObject* const selection_start,
+    LayoutObject* const selection_end,
+    int selection_end_pos,
+    LayoutSelection::SelectionPaintInvalidationMode
+        block_paint_invalidation_mode =
+            LayoutSelection::kPaintInvalidationNewXOROld) {
+  SelectedObjectMap selected_objects;
+  SelectedBlockMap selected_blocks;
+  LayoutObject* layout_object = selection_start;
+  LayoutObject* const stop =
+      LayoutObjectAfterPosition(selection_end, selection_end_pos);
   bool exploring_backwards = false;
-  bool continue_exploring = os && (os != stop);
+  bool continue_exploring = layout_object && (layout_object != stop);
   while (continue_exploring) {
-    if ((os->CanBeSelectionLeaf() || os == selection_start_ ||
-         os == selection_end_) &&
-        os->GetSelectionState() != SelectionNone) {
+    if ((layout_object->CanBeSelectionLeaf() ||
+         layout_object == selection_start || layout_object == selection_end) &&
+        layout_object->GetSelectionState() != SelectionNone) {
       // Blocks are responsible for painting line gaps and margin gaps.  They
       // must be examined as well.
-      old_selected_objects.Set(os, os->GetSelectionState());
-      if (block_paint_invalidation_mode == kPaintInvalidationNewXOROld) {
-        LayoutBlock* cb = os->ContainingBlock();
+      selected_objects.Set(layout_object, layout_object->GetSelectionState());
+      if (block_paint_invalidation_mode ==
+          LayoutSelection::kPaintInvalidationNewXOROld) {
+        LayoutBlock* cb = layout_object->ContainingBlock();
         while (cb && !cb->IsLayoutView()) {
           SelectedBlockMap::AddResult result =
-              old_selected_blocks.insert(cb, cb->GetSelectionState());
+              selected_blocks.insert(cb, cb->GetSelectionState());
           if (!result.is_new_entry)
             break;
           cb = cb->ContainingBlock();
@@ -201,22 +173,18 @@ void LayoutSelection::SetSelection(
       }
     }
 
-    os = GetNextOrPrevLayoutObjectBasedOnDirection(os, stop, continue_exploring,
-                                                   exploring_backwards);
+    layout_object = GetNextOrPrevLayoutObjectBasedOnDirection(
+        layout_object, stop, continue_exploring, exploring_backwards);
   }
+  return std::forward_as_tuple(selected_objects, selected_blocks);
+}
 
-  // Now clear the selection.
-  SelectedObjectMap::iterator old_objects_end = old_selected_objects.end();
-  for (SelectedObjectMap::iterator i = old_selected_objects.begin();
-       i != old_objects_end; ++i)
-    i->key->SetSelectionStateIfNeeded(SelectionNone);
-
-  // set selection start and end
-  selection_start_ = start;
-  selection_start_pos_ = start_pos;
-  selection_end_ = end;
-  selection_end_pos_ = end_pos;
+static void SetSelectionNone(SelectedObjectMap map) {
+  for (auto iterator : map)

[yosin_UTC9, 2017/04/27 09:50:29]

nit: s/auto/const auto&/
nit: s/iterator/key_value/ or another good name. "iterator" is usually abbrev to
"it"
and used as:
  for (auto it = old_selected_blocks.begin(); it != old_selected_blocks.end();
++it)

[yoichio, 2017/04/28 02:16:25]

Done.

+    iterator.key->SetSelectionStateIfNeeded(SelectionNone);
+}
 
+static void MarkSelection(LayoutObject* start, LayoutObject* end, int end_pos) {
   // Update the selection status of all objects between m_selectionStart and
   // m_selectionEnd
   if (start && start == end) {
@@ -228,51 +196,32 @@ void LayoutSelection::SetSelection(
       end->SetSelectionStateIfNeeded(SelectionEnd);
   }
 
-  LayoutObject* o = start;
-  stop = LayoutObjectAfterPosition(end, end_pos);
-
-  while (o && o != stop) {
-    if (o != start && o != end && o->CanBeSelectionLeaf())
-      o->SetSelectionStateIfNeeded(SelectionInside);
-    o = o->NextInPreOrder();
+  LayoutObject* layout_object = start;
+  LayoutObject* const stop = LayoutObjectAfterPosition(end, end_pos);
+  while (layout_object && layout_object != stop) {
+    if (layout_object != start && layout_object != end &&
+        layout_object->CanBeSelectionLeaf())
+      layout_object->SetSelectionStateIfNeeded(SelectionInside);
+    layout_object = layout_object->NextInPreOrder();
   }
+}
 
-  // Now that the selection state has been updated for the new objects, walk
-  // them again and put them in the new objects list.
-  o = start;
-  exploring_backwards = false;
-  continue_exploring = o && (o != stop);
-  while (continue_exploring) {
-    if ((o->CanBeSelectionLeaf() || o == start || o == end) &&
-        o->GetSelectionState() != SelectionNone) {
-      new_selected_objects.Set(o, o->GetSelectionState());
-      LayoutBlock* cb = o->ContainingBlock();
-      while (cb && !cb->IsLayoutView()) {
-        SelectedBlockMap::AddResult result =
-            new_selected_blocks.insert(cb, cb->GetSelectionState());
-        if (!result.is_new_entry)
-          break;
-        cb = cb->ContainingBlock();
-      }
-    }
-
-    o = GetNextOrPrevLayoutObjectBasedOnDirection(o, stop, continue_exploring,
-                                                  exploring_backwards);
-  }
-
-  // TODO(yoichio): DCHECK(frame_selection_->,,,->GetFrameView());
-  if (!frame_selection_->GetDocument().GetLayoutView()->GetFrameView())
-    return;
-
+static void InvalidateLayoutObjects(LayoutObject* start,
+                                    bool is_start_pos_changed,
+                                    LayoutObject* end,
+                                    bool is_end_pos_changed,
+                                    SelectedObjectMap& old_selected_objects,
+                                    SelectedBlockMap& old_selected_blocks,
+                                    SelectedObjectMap& new_selected_objects,
+                                    SelectedBlockMap& new_selected_blocks) {
   // Have any of the old selected objects changed compared to the new selection?
-  for (SelectedObjectMap::iterator i = old_selected_objects.begin();
-       i != old_objects_end; ++i) {
-    LayoutObject* obj = i->key;
+  for (auto iterator : old_selected_objects) {
+    LayoutObject* obj = iterator.key;
     SelectionState new_selection_state = obj->GetSelectionState();
-    SelectionState old_selection_state = i->value;
+    SelectionState old_selection_state = iterator.value;
     if (new_selection_state != old_selection_state ||
-        (selection_start_ == obj && old_start_pos != selection_start_pos_) ||
-        (selection_end_ == obj && old_end_pos != selection_end_pos_)) {
+        (start == obj && is_start_pos_changed) ||
+        (end == obj && is_end_pos_changed)) {
       obj->SetShouldInvalidateSelection();
       new_selected_objects.erase(obj);
     }
@@ -280,18 +229,14 @@ void LayoutSelection::SetSelection(
 
   // Any new objects that remain were not found in the old objects dict, and so
   // they need to be updated.
-  SelectedObjectMap::iterator new_objects_end = new_selected_objects.end();
-  for (SelectedObjectMap::iterator i = new_selected_objects.begin();
-       i != new_objects_end; ++i)
-    i->key->SetShouldInvalidateSelection();
+  for (auto iterator : new_selected_objects)

[yosin_UTC9, 2017/04/27 09:50:29]

nit: s/auto/const auto&/
nit: s/iterator/key_value/ or another good name. "iterator" is usually abbrev to
"it"
and used as:
  for (auto it = old_selected_blocks.begin(); it != old_selected_blocks.end();
++it)

[yoichio, 2017/04/28 02:16:25]

Done.

+    iterator.key->SetShouldInvalidateSelection();
 
   // Have any of the old blocks changed?
-  SelectedBlockMap::iterator old_blocks_end = old_selected_blocks.end();
-  for (SelectedBlockMap::iterator i = old_selected_blocks.begin();
-       i != old_blocks_end; ++i) {
-    LayoutBlock* block = i->key;
+  for (auto iterator : old_selected_blocks) {

[yosin_UTC9, 2017/04/27 09:50:29]

nit: s/auto/const auto&/
nit: s/iterator/key_value/ or another good name. "iterator" is usually abbrev to
"it"
and used as:
  for (auto it = old_selected_blocks.begin(); it != old_selected_blocks.end();
++it)

[yoichio, 2017/04/28 02:16:25]

Done.

+    LayoutBlock* block = iterator.key;
     SelectionState new_selection_state = block->GetSelectionState();
-    SelectionState old_selection_state = i->value;
+    SelectionState old_selection_state = iterator.value;
     if (new_selection_state != old_selection_state) {
       block->SetShouldInvalidateSelection();
       new_selected_blocks.erase(block);
@@ -300,10 +245,71 @@ void LayoutSelection::SetSelection(
 
   // Any new blocks that remain were not found in the old blocks dict, and so
   // they need to be updated.
-  SelectedBlockMap::iterator new_blocks_end = new_selected_blocks.end();
-  for (SelectedBlockMap::iterator i = new_selected_blocks.begin();
-       i != new_blocks_end; ++i)
-    i->key->SetShouldInvalidateSelection();
+  for (auto iterator : new_selected_blocks)
+    iterator.key->SetShouldInvalidateSelection();
+}
+
+void LayoutSelection::SetSelection(
+    LayoutObject* start,
+    int start_pos,
+    LayoutObject* end,
+    int end_pos,
+    SelectionPaintInvalidationMode block_paint_invalidation_mode) {
+  // This code makes no assumptions as to if the layout tree is up to date or
+  // not and will not try to update it. Currently clearSelection calls this
+  // (intentionally) without updating the layout tree as it doesn't care.
+  // Other callers may want to force recalc style before calling this.
+
+  // Make sure both our start and end objects are defined.
+  // Check www.msnbc.com and try clicking around to find the case where this
+  // happened.
+  if ((start && !end) || (end && !start))
+    return;
+
+  // Just return if the selection hasn't changed.
+  if (selection_start_ == start && selection_start_pos_ == start_pos &&
+      selection_end_ == end && selection_end_pos_ == end_pos)
+    return;
+
+  SelectedObjectMap old_selected_objects;
+  SelectedBlockMap old_selected_blocks;
+  std::tie(old_selected_objects, old_selected_blocks) =
+      CollectSelectedMap(selection_start_, selection_end_, selection_end_pos_,
+                         block_paint_invalidation_mode);
+  SetSelectionNone(old_selected_objects);
+
+  MarkSelection(start, end, end_pos);
+
+  // Now that the selection state has been updated for the new objects, walk
+  // them again and put them in the new objects list.
+  // FIXME: |new_selected_objects| doesn't really need to store the
+  // SelectionState, it's just more convenient to have it use the same data
+  // structure as |old_selected_objects|.
+  SelectedObjectMap new_selected_objects;
+  // FIXME: |new_selected_blocks| doesn't really need to store the
+  // SelectionState, it's just more convenient to have it use the same data
+  // structure as |old_selected_blocks|.
+  SelectedBlockMap new_selected_blocks;
+  std::tie(new_selected_objects, new_selected_blocks) =
+      CollectSelectedMap(start, end, end_pos);
+
+  // TODO(yoichio): DCHECK(frame_selection_->,,,->GetFrameView());
+  if (!frame_selection_->GetDocument().GetLayoutView()->GetFrameView())

[yosin_UTC9, 2017/04/27 09:50:29]

We should move this if-statement to beginning of this function.
Or at least after L281 MarkSelection()

[yoichio, 2017/04/28 02:16:25]

Done.

+    return;
+
+  // Record the old selected offsets. These will be used later when we compare
+  // against the new selected offsets.
+  const bool is_start_pos_changed = selection_start_pos_ != start_pos;
+  const bool is_end_pos_changed = selection_end_pos_ != end_pos;
+  // set selection start and end
+  selection_start_ = start;
+  selection_start_pos_ = start_pos;
+  selection_end_ = end;
+  selection_end_pos_ = end_pos;
+  InvalidateLayoutObjects(selection_start_, is_start_pos_changed,
+                          selection_end_, is_end_pos_changed,
+                          old_selected_objects, old_selected_blocks,
+                          new_selected_objects, new_selected_blocks);
 }
 
 void LayoutSelection::SelectionStartEnd(int& start_pos, int& end_pos) {