Clean up of SelectionModel

ui/gfx/render_text.cc

Index: ui/gfx/render_text.cc
===================================================================
--- ui/gfx/render_text.cc	(revision 103192)
+++ ui/gfx/render_text.cc	(working copy)
@@ -414,6 +414,20 @@
   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_(),
@@ -567,12 +581,19 @@
 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;
 }
@@ -622,18 +643,4 @@
   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