Introduce CollectSelectedMap() in 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 260debb79c233e2b803f07f406d5504fdbb8bb40..8c7d0316bf4734362e51e948d390afc489074738 100644
--- a/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
@@ -133,7 +133,59 @@ using SelectedObjectMap = HashMap<LayoutObject*, SelectionState>;
 // 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>;
+struct SelectedMap {
+  STACK_ALLOCATED();
+
+  SelectedMap() {}
+  SelectedMap(SelectedMap&& other) {
+    swap(first, other.first);

[yosin_UTC9, 2017/05/12 02:09:45]

nit: s/swap/std::swap/
or
|first = std::move(other.first)|

to avoid temporary variable in |std::swap()|, it is better to use |std::moved()|

[yoichio, 2017/05/12 04:37:39]

Done.

+    swap(second, other.second);

[yosin_UTC9, 2017/05/12 02:09:45]

nit: s/swap/std::swap/
second = std::move(other.second)

to avoid temporary variable in |std::swap()|, it is better to use |std::moved()|

[yoichio, 2017/05/12 04:37:39]

Done.

+  }
+  SelectedObjectMap first;

[yosin_UTC9, 2017/05/12 02:09:45]

nit: s/first/object_map/ or another instead of |first|.
Because |first| doesn't mean anything.

[yoichio, 2017/05/12 04:37:39]

Done.

+  SelectedBlockMap second;

[yosin_UTC9, 2017/05/12 02:09:45]

nit: s/second/block_map/ or another instead of |second|.
Because |second| doesn't mean anything.

[yoichio, 2017/05/12 04:37:39]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SelectedMap);
+};
+
+static SelectedMap CollectSelectedMap(
+    LayoutObject* selection_start,
+    LayoutObject* selection_end,
+    int selection_end_pos,
+    LayoutSelection::SelectionPaintInvalidationMode
+        block_paint_invalidation_mode =
+            LayoutSelection::kPaintInvalidationNewXOROld) {
+  SelectedMap selected_map;
+  LayoutObject* os = selection_start;

[yosin_UTC9, 2017/05/12 02:09:45]

nit: s/os/runner/ or another meaning full name.

[yoichio, 2017/05/12 04:37:39]

Done.

+  LayoutObject* stop =
+      LayoutObjectAfterPosition(selection_end, selection_end_pos);
+  bool exploring_backwards = false;
+  bool continue_exploring = os && (os != stop);
+  while (continue_exploring) {
+    if ((os->CanBeSelectionLeaf() || os == selection_start ||
+         os == selection_end) &&
+        os->GetSelectionState() != SelectionNone) {
+      // Blocks are responsible for painting line gaps and margin gaps.  They
+      // must be examined as well.
+      selected_map.first.Set(os, os->GetSelectionState());
+      if (block_paint_invalidation_mode ==
+          LayoutSelection::kPaintInvalidationNewXOROld) {
+        LayoutBlock* cb = os->ContainingBlock();

[yosin_UTC9, 2017/05/12 02:09:45]

nit: s/cb/containing_block/

[yoichio, 2017/05/12 04:37:39]

Done.

+        while (cb && !cb->IsLayoutView()) {
+          SelectedBlockMap::AddResult result =
+              selected_map.second.insert(cb, cb->GetSelectionState());
+          if (!result.is_new_entry)
+            break;
+          cb = cb->ContainingBlock();
+        }
+      }
+    }
+
+    os = GetNextOrPrevLayoutObjectBasedOnDirection(os, stop, continue_exploring,
+                                                   exploring_backwards);
+  }
+  return selected_map;
+}
 
 void LayoutSelection::SetSelection(
     LayoutObject* start,
@@ -164,34 +216,9 @@ void LayoutSelection::SetSelection(
   int old_start_pos = selection_start_pos_;
   int old_end_pos = selection_end_pos_;
 
-  SelectedMap old_selected_map;
-  LayoutObject* os = selection_start_;
-  LayoutObject* stop =
-      LayoutObjectAfterPosition(selection_end_, selection_end_pos_);
-  bool exploring_backwards = false;
-  bool continue_exploring = os && (os != stop);
-  while (continue_exploring) {
-    if ((os->CanBeSelectionLeaf() || os == selection_start_ ||
-         os == selection_end_) &&
-        os->GetSelectionState() != SelectionNone) {
-      // Blocks are responsible for painting line gaps and margin gaps.  They
-      // must be examined as well.
-      old_selected_map.first.Set(os, os->GetSelectionState());
-      if (block_paint_invalidation_mode == kPaintInvalidationNewXOROld) {
-        LayoutBlock* cb = os->ContainingBlock();
-        while (cb && !cb->IsLayoutView()) {
-          SelectedBlockMap::AddResult result =
-              old_selected_map.second.insert(cb, cb->GetSelectionState());
-          if (!result.is_new_entry)
-            break;
-          cb = cb->ContainingBlock();
-        }
-      }
-    }
-
-    os = GetNextOrPrevLayoutObjectBasedOnDirection(os, stop, continue_exploring,
-                                                   exploring_backwards);
-  }
+  SelectedMap old_selected_map =
+      CollectSelectedMap(selection_start_, selection_end_, selection_end_pos_,
+                         block_paint_invalidation_mode);
 
   // Now clear the selection.
   SelectedObjectMap::iterator old_objects_end = old_selected_map.first.end();
@@ -217,7 +244,7 @@ void LayoutSelection::SetSelection(
   }
 
   LayoutObject* o = start;
-  stop = LayoutObjectAfterPosition(end, end_pos);
+  LayoutObject* stop = LayoutObjectAfterPosition(end, end_pos);

[yosin_UTC9, 2017/05/12 02:09:45]

Can we make |LayoutObject* stop|?

[yoichio, 2017/05/12 04:37:39]

Done.

 
   while (o && o != stop) {
     if (o != start && o != end && o->CanBeSelectionLeaf())
@@ -227,30 +254,10 @@ void LayoutSelection::SetSelection(
 
   // 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
+  // TODO(editing-dev): |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;
-  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_map.first.Set(o, o->GetSelectionState());
-      LayoutBlock* cb = o->ContainingBlock();
-      while (cb && !cb->IsLayoutView()) {
-        SelectedBlockMap::AddResult result =
-            new_selected_map.second.insert(cb, cb->GetSelectionState());
-        if (!result.is_new_entry)
-          break;
-        cb = cb->ContainingBlock();
-      }
-    }
-
-    o = GetNextOrPrevLayoutObjectBasedOnDirection(o, stop, continue_exploring,
-                                                  exploring_backwards);
-  }
+  SelectedMap new_selected_map = CollectSelectedMap(start, end, end_pos);
 
   // Have any of the old selected objects changed compared to the new selection?
   for (SelectedObjectMap::iterator i = old_selected_map.first.begin();