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..86832bab059aae8d78a5c0bba4b873bef5e1d7fb 100644
--- a/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/LayoutSelection.cpp
@@ -186,6 +186,25 @@ static SelectedMap CollectSelectedMap(
   return selected_map;
 }
 
+// Update the selection status of all LayoutObjects between |start| and |end|.
+static void SetSelectionState(LayoutObject* start,
+                              LayoutObject* end,
+                              int end_pos) {
+  if (start && start == end)

[yosin_UTC9, 2017/05/16 01:38:14]

Oops, sorry my comment was wrong, according to layout test failure.
This if-statement block should be:

if (start && start == end) {
  ...
} else {
  if (start)
   start->SetSelectionState(...)
  if (end)
   end->SetSelectionState(...)
}

When |start| is |nullptr|, also implies |end| is |nullptr|, this
function does't nothing.

I can't imagine |start && !end| or |!start && end| cases, could you
try?


So, we can write:
static void SetSelectionState(LayoutObject* start, ...) {
  if (!start) {
    DCHECK_EQ(end, nullptr);
    return;
  }

  DCHECK(end) << start;
  
  if (start == end) {
    start->StartSelectionIfNeeded(SelectionBoth);
    return;
  }

  start->SetSelectionStateIfNeeded(SelectionStart);
  end->SetSelectionStateIfNeeded(SelectionEnd);

  ...
}

[yoichio, 2017/05/16 01:48:07]

This code can be increased but I want this CL only to extract rather than detail
clean up.

+    start->SetSelectionStateIfNeeded(SelectionBoth);
+  else if (start)
+    start->SetSelectionStateIfNeeded(SelectionStart);
+  else if (end)
+    end->SetSelectionStateIfNeeded(SelectionEnd);
+
+  LayoutObject* const stop = LayoutObjectAfterPosition(end, end_pos);
+  for (LayoutObject* runner = start; runner && runner != stop;
+       runner = runner->NextInPreOrder()) {
+    if (runner != start && runner != end && runner->CanBeSelectionLeaf())

[yosin_UTC9, 2017/05/16 01:38:14]

nit: better to use early continue style.

if (runner == start || runner == end || !runner->CanBeSelectionLeaf())
  continue;
runner->SetSelectionStateIfNeeded(SelectionInSide)

If we initialize |runner| as |runner = runner->nextInPreorder()|, we don't
need to check |runner == start|.

BTW, it seems we can use |end| as loop condition |runner != end| instead of
|runner != stop|.

[yoichio, 2017/05/16 01:48:07]

Acknowledged.

+      runner->SetSelectionStateIfNeeded(SelectionInside);
+  }
+}
+
 void LayoutSelection::SetSelection(
     LayoutObject* start,
     int start_pos,
@@ -232,25 +251,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();
-  }
+  SetSelectionState(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.