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 cd5cc26b30d141b990834a5611edc212cd91e2f3..e5f75e13700f78e5b27fed17e954272555692b3d 100644
--- a/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
@@ -126,70 +126,43 @@ 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.
+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>;
+using SelectedMap = std::pair<SelectedObjectMap, SelectedBlockMap>;
+
+static SelectedMap 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();
@@ -197,22 +170,21 @@ void LayoutSelection::SetSelection(
       }
     }
 
-    os = GetNextOrPrevLayoutObjectBasedOnDirection(os, stop, continue_exploring,
-                                                   exploring_backwards);
+    layout_object = GetNextOrPrevLayoutObjectBasedOnDirection(
+        layout_object, stop, continue_exploring, exploring_backwards);
   }
+  return std::make_pair(std::move(selected_objects),
+                        std::move(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 SetSelectionStateNone(const SelectedObjectMap& map) {
+  for (const auto& it : map)
+    it.key->SetSelectionStateIfNeeded(SelectionNone);
+}
 
+static void SetSelectionState(LayoutObject* start,
+                              LayoutObject* end,
+                              int end_pos) {
   // Update the selection status of all objects between m_selectionStart and
   // m_selectionEnd
   if (start && start == end) {
@@ -224,51 +196,35 @@ 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();
-  }
-
-  // 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);
+  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();
   }
+}
 
-  // 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,
+                                    const SelectedMap& old_selected_map,
+                                    SelectedMap& new_selected_map) {

[yosin_UTC9, 2017/05/10 07:00:57]

s/SelectedMap&/SelectedMap*/
We use reference for output parameter.

+  const SelectedObjectMap& old_selected_objects = old_selected_map.first;
+  const SelectedBlockMap& old_selected_blocks = old_selected_map.second;
+  SelectedObjectMap& new_selected_objects = new_selected_map.first;
+  SelectedBlockMap& new_selected_blocks = new_selected_map.second;
 
   // 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;
-    SelectionState new_selection_state = obj->GetSelectionState();
-    SelectionState old_selection_state = i->value;
+  for (const auto& it : old_selected_objects) {
+    LayoutObject* const obj = it.key;
+    const SelectionState new_selection_state = obj->GetSelectionState();
+    const SelectionState old_selection_state = it.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);
     }
@@ -276,18 +232,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 (const auto& it : new_selected_objects)
+    it.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;
-    SelectionState new_selection_state = block->GetSelectionState();
-    SelectionState old_selection_state = i->value;
+  for (const auto& it : old_selected_blocks) {
+    LayoutBlock* const block = it.key;
+    const SelectionState new_selection_state = block->GetSelectionState();
+    const SelectionState old_selection_state = it.value;
     if (new_selection_state != old_selection_state) {
       block->SetShouldInvalidateSelection();
       new_selected_blocks.erase(block);
@@ -296,10 +248,62 @@ 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 (const auto& it : new_selected_blocks)
+    it.key->SetShouldInvalidateSelection();
+}
+
+void LayoutSelection::SetSelection(
+    LayoutObject* start,

[yosin_UTC9, 2017/05/10 07:00:57]

Introducing |struct LayoutObjectWithOffset| before this patch is easier to read?

+    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;
+
+  SelectedMap old_selected_map =
+      CollectSelectedMap(selection_start_, selection_end_, selection_end_pos_,
+                         block_paint_invalidation_mode);
+  SetSelectionStateNone(old_selected_map.first);
+
+  SetSelectionState(start, end, end_pos);
+
+  // 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;
+
+  // 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_map| doesn't really need to store the
+  // SelectionState, it's just more convenient to have it use the same data
+  // structure as |old_selected_map|.
+  SelectedMap new_selected_map = CollectSelectedMap(start, end, end_pos);
+  // TODO(yoichio): DCHECK(frame_selection_->,,,->GetFrameView());
+  if (!frame_selection_->GetDocument().GetLayoutView()->GetFrameView())

[yosin_UTC9, 2017/05/10 07:00:57]

Can we move this if-statement at start of function?
Since this function doesn't destroy FrameView.

+    return;
+
+  InvalidateLayoutObjects(selection_start_, is_start_pos_changed,

[yosin_UTC9, 2017/05/10 07:00:57]

It is better to pass old selection_{start,end}_pos_ instead of bool's.

+                          selection_end_, is_end_pos_changed, old_selected_map,
+                          new_selected_map);

[yosin_UTC9, 2017/05/10 07:00:57]

Could you move |SelectedMap new_selected_map = CollectSelectedMap(start, end,
end_pos);|
into  InvalidateLayoutObjects()?
Since, we don't use |new_selected_map| after here.

Also, we don't need to pass |new_selected_map|.

 }
 
 std::pair<int, int> LayoutSelection::SelectionStartEnd() {