Simplify handling of BiDi cursor movement

ui/views/controls/textfield/textfield_views_model.cc

Index: ui/views/controls/textfield/textfield_views_model.cc
diff --git a/ui/views/controls/textfield/textfield_views_model.cc b/ui/views/controls/textfield/textfield_views_model.cc
index d0fc8e01aec4c841e6429b8b3b7241661718158a..b73d68e4d5874289fc43f90a1d469091f3349d77 100644
--- a/ui/views/controls/textfield/textfield_views_model.cc
+++ b/ui/views/controls/textfield/textfield_views_model.cc
@@ -345,7 +345,8 @@ bool TextfieldViewsModel::Delete() {
     size_t cursor_position = GetCursorPosition();
     size_t next_grapheme_index = render_text_->IndexOfAdjacentGrapheme(
         cursor_position, gfx::CURSOR_FORWARD);
-    ExecuteAndRecordDelete(cursor_position, next_grapheme_index, true);
+    ExecuteAndRecordDelete(ui::Range(cursor_position, next_grapheme_index),
+                           true);
     return true;
   }
   return false;
@@ -363,14 +364,15 @@ bool TextfieldViewsModel::Backspace() {
   }
   if (GetCursorPosition() > 0) {
     size_t cursor_position = GetCursorPosition();
-    ExecuteAndRecordDelete(cursor_position, cursor_position - 1, true);
+    ExecuteAndRecordDelete(ui::Range(cursor_position, cursor_position - 1),

[msw, 2012/02/16 00:09:21]

Hmm, shouldn't the range end be IndexOfAdjacentGrapheme(cursor_position,
gfx::CURSOR_BACKWARD) corresponding with the behavior in
TextfieldViewsModel::Delete?

[benrg, 2012/02/16 09:20:55]

I think the idea is that Backspace should remove combining diacritics one by
one, while Delete should remove the whole grapheme with all diacritics. This
code should check for surrogate pairs -- I have a pending change that fixes
that. It's possible that it should do other special things that I don't know
about.

+                           true);
     return true;
   }
   return false;
 }
 
 size_t TextfieldViewsModel::GetCursorPosition() const {
-  return render_text_->GetCursorPosition();
+  return render_text_->cursor_position();
 }
 
 void TextfieldViewsModel::MoveCursor(gfx::BreakType break_type,
@@ -381,20 +383,18 @@ void TextfieldViewsModel::MoveCursor(gfx::BreakType break_type,
   render_text_->MoveCursor(break_type, direction, select);
 }
 
-bool TextfieldViewsModel::MoveCursorTo(const gfx::SelectionModel& selection) {
+bool TextfieldViewsModel::MoveCursorTo(const gfx::SelectionModel& model) {
   if (HasCompositionText()) {
     ConfirmCompositionText();
     // ConfirmCompositionText() updates cursor position. Need to reflect it in
     // the SelectionModel parameter of MoveCursorTo().
-    if (render_text_->GetSelectionStart() != selection.selection_end())
-      return render_text_->SelectRange(ui::Range(
-          render_text_->GetSelectionStart(), selection.selection_end()));
-    gfx::SelectionModel sel(selection.selection_end(),
-                            selection.caret_pos(),
-                            selection.caret_placement());
-    return render_text_->MoveCursorTo(sel);
+    ui::Range range(render_text_->selection().start(), model.caret_pos());
+    if (!range.is_empty())
+      return render_text_->SelectRange(range);

[msw, 2012/02/16 00:09:21]

nit: Optionally set the range on a SelectionModel and always call MoveCursorTo.

[benrg, 2012/02/16 09:20:55]

That would probably make sense. As I said elsewhere, I started to overhaul this
code and eventually decided to leave it for another CL. It seems weird (probably
a bug?) that this method behaves as though Shift is held down when there's
composition text.

+    return render_text_->MoveCursorTo(
+        gfx::SelectionModel(model.caret_pos(), model.caret_affinity()));
   }
-  return render_text_->MoveCursorTo(selection);
+  return render_text_->MoveCursorTo(model);
 }
 
 bool TextfieldViewsModel::MoveCursorTo(const gfx::Point& point, bool select) {
@@ -404,13 +404,12 @@ bool TextfieldViewsModel::MoveCursorTo(const gfx::Point& point, bool select) {
 }
 
 string16 TextfieldViewsModel::GetSelectedText() const {
-  return GetText().substr(render_text_->MinOfSelection(),
-      (render_text_->MaxOfSelection() - render_text_->MinOfSelection()));
+  return GetText().substr(render_text_->selection().GetMin(),
+                          render_text_->selection().length());
 }
 
 void TextfieldViewsModel::GetSelectedRange(ui::Range* range) const {

[msw, 2012/02/16 00:09:21]

Should this convenience function be deprecated?
NativeTextfieldViews::GetSelectedRange could just assign
GetRenderText()->selection(); to its |range| arg.

[benrg, 2012/02/16 09:20:55]

Seems simple. Done.

-  range->set_start(render_text_->GetSelectionStart());
-  range->set_end(render_text_->GetCursorPosition());
+  *range = render_text_->selection();
 }
 
 void TextfieldViewsModel::SelectRange(const ui::Range& range) {
@@ -506,8 +505,8 @@ bool TextfieldViewsModel::Cut() {
     // than beginning, unlike Delete/Backspace.
     // TODO(oshima): Change Delete/Backspace to use DeleteSelection,
     // update DeleteEdit and remove this trick.
-    render_text_->SelectRange(ui::Range(render_text_->GetCursorPosition(),
-                                        render_text_->GetSelectionStart()));
+    const ui::Range& selection = render_text_->selection();
+    render_text_->SelectRange(ui::Range(selection.end(), selection.start()));
     DeleteSelection();
     return true;
   }
@@ -535,14 +534,13 @@ bool TextfieldViewsModel::Paste() {
 }
 
 bool TextfieldViewsModel::HasSelection() const {
-  return !render_text_->EmptySelection();
+  return !render_text_->selection().is_empty();
 }
 
 void TextfieldViewsModel::DeleteSelection() {
   DCHECK(!HasCompositionText());
   DCHECK(HasSelection());
-  ExecuteAndRecordDelete(render_text_->GetSelectionStart(),
-                         render_text_->GetCursorPosition(), false);
+  ExecuteAndRecordDelete(render_text_->selection(), false);
 }
 
 void TextfieldViewsModel::DeleteSelectionAndInsertTextAt(
@@ -657,10 +655,10 @@ void TextfieldViewsModel::ReplaceTextInternal(const string16& text,
     // with |text|. So, need to find the index of next grapheme first.
     size_t next =
         render_text_->IndexOfAdjacentGrapheme(cursor, gfx::CURSOR_FORWARD);
-    if (next == model.selection_end())
+    if (next == model.caret_pos())
       render_text_->MoveCursorTo(model);
     else
-      render_text_->SelectRange(ui::Range(next, model.selection_end()));
+      render_text_->SelectRange(ui::Range(next, model.caret_pos()));
   }
   // Edit history is recorded in InsertText.
   InsertTextInternal(text, mergeable);
@@ -686,13 +684,11 @@ void TextfieldViewsModel::ClearRedoHistory() {
   edit_history_.erase(delete_start, edit_history_.end());
 }
 
-void TextfieldViewsModel::ExecuteAndRecordDelete(size_t from,
-                                                 size_t to,
+void TextfieldViewsModel::ExecuteAndRecordDelete(ui::Range range,
                                                  bool mergeable) {
-  size_t old_text_start = std::min(from, to);
-  const string16 text = GetText().substr(old_text_start,
-      std::abs(static_cast<long>(from - to)));
-  bool backward = from > to;
+  size_t old_text_start = range.GetMin();
+  const string16 text = GetText().substr(old_text_start, range.length());
+  bool backward = range.is_reversed();
   Edit* edit = new DeleteEdit(mergeable, text, old_text_start, backward);
   bool delete_edit = AddOrMergeEditHistory(edit);
   edit->Redo(this);
@@ -702,7 +698,7 @@ void TextfieldViewsModel::ExecuteAndRecordDelete(size_t from,
 
 void TextfieldViewsModel::ExecuteAndRecordReplaceSelection(
     MergeType merge_type, const string16& new_text) {
-  size_t new_text_start = render_text_->MinOfSelection();
+  size_t new_text_start = render_text_->selection().GetMin();
   size_t new_cursor_pos = new_text_start + new_text.length();
   ExecuteAndRecordReplace(merge_type,
                           GetCursorPosition(),
@@ -716,9 +712,8 @@ void TextfieldViewsModel::ExecuteAndRecordReplace(MergeType merge_type,
                                                   size_t new_cursor_pos,
                                                   const string16& new_text,
                                                   size_t new_text_start) {
-  size_t old_text_start = render_text_->MinOfSelection();
-  bool backward =
-      render_text_->GetSelectionStart() > render_text_->GetCursorPosition();
+  size_t old_text_start = render_text_->selection().GetMin();
+  bool backward = render_text_->selection().is_reversed();
   Edit* edit = new ReplaceEdit(merge_type,
                                GetSelectedText(),
                                old_cursor_pos,