Introduce MarkSetSelectionState() in LayoutSelection::SetSelection()

third_party/WebKit/Source/core/editing/LayoutSelection.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights
  * reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 168 matching lines @@
         }
       }
     }
 
     runner = GetNextOrPrevLayoutObjectBasedOnDirection(
         runner, stop, continue_exploring, exploring_backwards);
   }
   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,
     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.
@@ skipping 26 matching lines @@
   for (SelectedObjectMap::iterator i = old_selected_map.object_map.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;
 
-  // 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.
   // 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 =
       CollectSelectedMap(start, end, end_pos, kPaintInvalidationNewXOROld);
 
   // Have any of the old selected objects changed compared to the new selection?
@@ skipping 170 matching lines @@
 
     o->SetShouldInvalidateSelection();
   }
 }
 
 DEFINE_TRACE(LayoutSelection) {
   visitor->Trace(frame_selection_);
 }
 
 }  // namespace blink