Clean up of SelectionModel

ui/gfx/render_text.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/render_text.h"
 
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
 #include "base/logging.h"
@@ skipping 396 matching lines @@
 
 const Rect& RenderText::GetUpdatedCursorBounds() {
   UpdateCachedBoundsAndOffset();
   return cursor_bounds_;
 }
 
 size_t RenderText::GetIndexOfNextGrapheme(size_t position) {
   return IndexOfAdjacentGrapheme(position, true);
 }
 
+SelectionModel RenderText::GetSelectionModelForSelectionStart() {
+  size_t selection_start = GetSelectionStart();
+  size_t selection_end = GetCursorPosition();
+  if (selection_start < selection_end)
+    return SelectionModel(selection_start,

[msw, 2011/09/29 02:20:21]

You can use the single-argument ctor here for the same result:
return SelectionModel(selection_start);
But it's fine with me if you'd rather leave it explicit.

[xji, 2011/10/03 23:18:57]

left is as explicit.

+                          selection_start,
+                          SelectionModel::LEADING);
+  else if (selection_start > selection_end)
+    return SelectionModel(selection_start,
+                          GetIndexOfPreviousGrapheme(selection_start),
+                          SelectionModel::TRAILING);
+  return selection_model_;
+}
+
 RenderText::RenderText()
     : text_(),
       selection_model_(),
       cursor_bounds_(),
       cursor_visible_(false),
       insert_mode_(true),
       composition_range_(ui::Range::InvalidRange()),
       style_ranges_(),
       default_style_(),
       display_rect_(),
@@ skipping 133 matching lines @@
   Point p(point.Add(display_rect().origin()));
   p = p.Add(GetUpdatedDisplayOffset());
   if (base::i18n::IsRTL())
     p.Offset(display_rect().width() - GetStringWidth() - 1, 0);
   return p;
 }
 
 void RenderText::SetSelectionModel(const SelectionModel& selection_model) {
   DCHECK_LE(selection_model.selection_start(), text().length());
   selection_model_.set_selection_start(selection_model.selection_start());
-  DCHECK_LE(selection_model.selection_end(), text().length());
-  selection_model_.set_selection_end(selection_model.selection_end());
-  DCHECK_LT(selection_model.caret_pos(),
-            std::max(text().length(), static_cast<size_t>(1)));
-  selection_model_.set_caret_pos(selection_model.caret_pos());
-  selection_model_.set_caret_placement(selection_model.caret_placement());
+  size_t selection_end = selection_model.selection_end();

[msw, 2011/09/29 02:20:21]

Can you add a RenderTextTest for this new behavior? It'd be good to test a
couple simple cases and one or two with complex script where the previous
grapheme isn't pos-1.

[xji, 2011/10/03 23:18:57]

this function is reverted.

+  DCHECK_LE(selection_end, text().length());
+  selection_model_.set_selection_end(selection_end);
+  if (selection_model.caret_placement() ==
+      gfx::SelectionModel::TRAILING_OF_PREVIOUS_GRAPHEME) {
+    selection_model_.set_caret_pos(GetIndexOfPreviousGrapheme(selection_end));

[msw, 2011/09/29 02:20:21]

You need to check if GetIndexOfPreviousGrapheme returns the same position as its
argument (when there is no previous grapheme), and in that case we set the caret
position to leading... see how it's done in MoveCursorTo(size_t, bool) directly
below.

Maybe we need a warning on GetIndexOfPreviousGrapheme about that edge case,
since this isn't the first time it's come up.

+    selection_model_.set_caret_placement(gfx::SelectionModel::TRAILING);
+  } else {
+    DCHECK_LT(selection_model.caret_pos(),
+              std::max(text().length(), static_cast<size_t>(1)));
+    selection_model_.set_caret_pos(selection_model.caret_pos());
+    selection_model_.set_caret_placement(selection_model.caret_placement());
+  }
 
   cached_bounds_and_offset_valid_ = false;
 }
 
 void RenderText::MoveCursorTo(size_t position, bool select) {
   size_t cursor = std::min(position, text().length());
   size_t caret_pos = GetIndexOfPreviousGrapheme(cursor);
   SelectionModel::CaretPlacement placement = (caret_pos == cursor) ?
       SelectionModel::LEADING : SelectionModel::TRAILING;
   size_t selection_start = select ? GetSelectionStart() : cursor;
@@ skipping 29 matching lines @@
     // TODO(xji): have similar problem as above when overflow character is a
     // LTR character.
     //
     // Pan to show the cursor when it overflows to the left.
     delta_offset = display_rect_.x() - cursor_bounds_.x();
   }
   display_offset_.Offset(delta_offset, 0);
   cursor_bounds_.Offset(delta_offset, 0);
 }
 
-SelectionModel RenderText::GetSelectionModelForSelectionStart() {
-  size_t selection_start = GetSelectionStart();
-  size_t selection_end = GetCursorPosition();
-  if (selection_start < selection_end)
-    return SelectionModel(selection_start,
-                          selection_start,
-                          SelectionModel::LEADING);
-  else if (selection_start > selection_end)
-    return SelectionModel(selection_start,
-                          GetIndexOfPreviousGrapheme(selection_start),
-                          SelectionModel::TRAILING);
-  return selection_model_;
-}
-
 }  // namespace gfx