Introduce MarkSetSelectionState() 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 8112f8c98c307d38b375972dfef6f7b0a132e798..23508f405ddf20a407ecb91f30b3a42679cc36cc 100644
--- a/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
@@ -186,6 +186,30 @@ static SelectedMap CollectSelectedMap(
   return selected_map;
 }
 
+static void MarkSetSelectionState(LayoutObject* start,

[yosin_UTC9, 2017/05/15 08:30:05]

nit: s/MarkSetSelectionState/MarkSelectionState/

No need to have "Set"

[yoichio, 2017/05/15 09:12:22]

Done.

+                                  LayoutObject* end,
+                                  int end_pos) {
+  // Update the selection status of all objects between m_selectionStart and

[yosin_UTC9, 2017/05/15 08:30:04]

nit: s/status/state/
nit: s/m_selectionStart/|start|/

[yoichio, 2017/05/15 09:12:21]

Done.

+  // m_selectionEnd

[yosin_UTC9, 2017/05/15 08:30:05]

nit: s/m_selecitonEnd/|end|/

This function doesn't refer |m_selectionStart| and |m_selectionEnd|

[yoichio, 2017/05/15 09:12:22]

Done.

+  if (start && start == end) {
+    start->SetSelectionStateIfNeeded(SelectionBoth);
+  } else {

[yosin_UTC9, 2017/05/15 08:30:05]

nit: better to use else-if to shallow indentation

if (start && start == end)
  start->SetSel...
else if (start)
  start->...
else if (end)
  end->....

[yoichio, 2017/05/15 09:12:22]

Done.

+    if (start)
+      start->SetSelectionStateIfNeeded(SelectionStart);
+    if (end)
+      end->SetSelectionStateIfNeeded(SelectionEnd);
+  }
+
+  LayoutObject* o = start;

[yosin_UTC9, 2017/05/15 08:30:05]

nit: s/o/runner/ or something
Please avoid to use one letter variable name.

[yoichio, 2017/05/15 09:12:21]

Done.

+  LayoutObject* const stop = LayoutObjectAfterPosition(end, end_pos);
+
+  while (o && o != stop) {

[yosin_UTC9, 2017/05/15 08:30:04]

nit: It is better to use for:

for (LayoutObject* runner = start; 
     start && start != stop; 
     runner = runner->NextInPreOrder() {
 ...
}

[yoichio, 2017/05/15 09:12:22]

Done.

+    if (o != start && o != end && o->CanBeSelectionLeaf())
+      o->SetSelectionStateIfNeeded(SelectionInside);
+    o = o->NextInPreOrder();
+  }
+}
+
 void LayoutSelection::SetSelection(
     LayoutObject* start,
     int start_pos,
@@ -232,25 +256,7 @@ void LayoutSelection::SetSelection(
   selection_end_ = end;
   selection_end_pos_ = end_pos;
 
-  // Update the selection status of all objects between m_selectionStart and
-  // m_selectionEnd
-  if (start && start == end) {
-    start->SetSelectionStateIfNeeded(SelectionBoth);
-  } else {
-    if (start)
-      start->SetSelectionStateIfNeeded(SelectionStart);
-    if (end)
-      end->SetSelectionStateIfNeeded(SelectionEnd);
-  }
-
-  LayoutObject* o = start;
-  LayoutObject* const stop = LayoutObjectAfterPosition(end, end_pos);
-
-  while (o && o != stop) {
-    if (o != start && o != end && o->CanBeSelectionLeaf())
-      o->SetSelectionStateIfNeeded(SelectionInside);
-    o = o->NextInPreOrder();
-  }
+  MarkSetSelectionState(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.