Refactor 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 112 matching lines @@
     exploring_backwards = !next && (next != stop);
     if (exploring_backwards) {
       next = stop->PreviousInPreOrder();
       continue_exploring = next && !next->IsLayoutView();
     }
   }
 
   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.
+// Objects each have a single selection rect to examine.
+typedef HashMap<LayoutObject*, SelectionState> SelectedObjectMap;

[yosin_UTC9, 2017/04/28 03:21:51]

nit: better to use |using|

[yoichio, 2017/04/28 08:00:33]

Done.

+// 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;

[yosin_UTC9, 2017/04/28 03:21:51]

nit: better to use |using|

[yoichio, 2017/04/28 08:00:33]

Done.

 
-  // 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_);
+static std::tuple<SelectedObjectMap, SelectedBlockMap> CollectSelectedMap(

[yosin_UTC9, 2017/04/28 03:21:51]

nit: it is better to use |std::pair<>| since it has only two members.

[yoichio, 2017/04/28 08:00:34]

Done.

+    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();
         }
       }
     }
 
-    os = GetNextOrPrevLayoutObjectBasedOnDirection(os, stop, continue_exploring,
-                                                   exploring_backwards);
+    layout_object = GetNextOrPrevLayoutObjectBasedOnDirection(
+        layout_object, stop, continue_exploring, exploring_backwards);
   }
+  return std::forward_as_tuple(selected_objects, selected_blocks);

[yoichio, 2017/04/28 02:16:26]

Does this correctly move |selected_objects| and |selected_blocks|
 to caller w/o deep copy?

[yosin_UTC9, 2017/04/28 03:21:51]

|std::forward_as_tuple()| is used for function argument[1].

We should write:
return std::move(std::make_pair(selection_objects, selected_blocks));

[1] http://en.cppreference.com/w/cpp/utility/tuple/forward_as_tuple

[yoichio, 2017/04/28 08:00:33]

Acknowledged.

+}
 
-  // 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);
+static void SetSelectionStateNone(const SelectedObjectMap& map) {
+  for (const auto& it : map)
+    it.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 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) {
     start->SetSelectionStateIfNeeded(SelectionBoth);
   } else {
     if (start)
       start->SetSelectionStateIfNeeded(SelectionStart);
     if (end)
       end->SetSelectionStateIfNeeded(SelectionEnd);
   }
 
-  LayoutObject* o = start;
-  stop = LayoutObjectAfterPosition(end, end_pos);
+  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();
+  }
+}
 
-  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);
-  }
-
-  // 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 SelectedObjectMap& old_selected_objects,
+    const SelectedBlockMap& old_selected_blocks,
+    SelectedObjectMap& new_selected_objects,
+    SelectedBlockMap& new_selected_blocks) {
   // 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;
+  for (const auto& it : old_selected_objects) {
+    LayoutObject* obj = it.key;
     SelectionState new_selection_state = obj->GetSelectionState();
-    SelectionState old_selection_state = i->value;
+    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);
     }
   }
 
   // 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;
+  for (const auto& it : old_selected_blocks) {
+    LayoutBlock* block = it.key;

[yosin_UTC9, 2017/04/28 03:21:51]

nit: s/LayoutBlock*/LayoutBlock* const/
Is it better to use |LayoutBlock&|?

[yoichio, 2017/04/28 08:00:33]

Done.

     SelectionState new_selection_state = block->GetSelectionState();
-    SelectionState old_selection_state = i->value;
+    SelectionState old_selection_state = it.value;

[yosin_UTC9, 2017/04/28 03:21:51]

nit: s/SelectionState/const SelectionState/

[yoichio, 2017/04/28 08:00:33]

Done.

     if (new_selection_state != old_selection_state) {
       block->SetShouldInvalidateSelection();
       new_selected_blocks.erase(block);
     }
   }
 
   // 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,
+    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;
+
+  SelectedObjectMap old_selected_objects;
+  SelectedBlockMap old_selected_blocks;
+  std::tie(old_selected_objects, old_selected_blocks) =

[yosin_UTC9, 2017/04/28 03:21:51]

I think it is better to introduce |struct SelectedMap { SelectedObjectMap
objects; SelectedBlockMap blocks; }|
Rather than destructuing tuple.

[yoichio, 2017/04/28 08:00:33]

Done.

+      CollectSelectedMap(selection_start_, selection_end_, selection_end_pos_,
+                         block_paint_invalidation_mode);
+  SetSelectionStateNone(old_selected_objects);
+
+  SetSelectionState(start, end, end_pos);
+
+  // TODO(yoichio): DCHECK(frame_selection_->,,,->GetFrameView());
+  if (!frame_selection_->GetDocument().GetLayoutView()->GetFrameView())
+    return;
+
+  // 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_objects| doesn't really need to store the
+  // SelectionState, it's just more convenient to have it use the same data
+  // structure as |old_selected_objects|.
+  SelectedObjectMap new_selected_objects;
+  // FIXME: |new_selected_blocks| doesn't really need to store the
+  // SelectionState, it's just more convenient to have it use the same data
+  // structure as |old_selected_blocks|.
+  SelectedBlockMap new_selected_blocks;
+  std::tie(new_selected_objects, new_selected_blocks) =
+      CollectSelectedMap(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;
+  InvalidateLayoutObjects(selection_start_, is_start_pos_changed,
+                          selection_end_, is_end_pos_changed,
+                          old_selected_objects, old_selected_blocks,
+                          new_selected_objects, new_selected_blocks);
 }
 
 void LayoutSelection::SelectionStartEnd(int& start_pos, int& end_pos) {
   Commit();
   start_pos = selection_start_pos_;
   end_pos = selection_end_pos_;
 }
 
 void LayoutSelection::ClearSelection() {
   // For querying Layer::compositingState()
@@ skipping 130 matching lines @@
 
     o->SetShouldInvalidateSelection();
   }
 }
 
 DEFINE_TRACE(LayoutSelection) {
   visitor->Trace(frame_selection_);
 }
 
 }  // namespace blink