Fix multiple problems with omnibox text handling across focus changes.

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

Index: chrome/browser/ui/views/omnibox/omnibox_view_views.cc
===================================================================
--- chrome/browser/ui/views/omnibox/omnibox_view_views.cc	(revision 224205)
+++ chrome/browser/ui/views/omnibox/omnibox_view_views.cc	(working copy)
@@ -68,20 +68,24 @@
   static const char kKey[];
 
   OmniboxState(const OmniboxEditModel::State& model_state,
-               const gfx::SelectionModel& selection_model);
+               const gfx::Range& selection,
+               const gfx::Range& saved_selection_for_focus_change);
   virtual ~OmniboxState();
 
   const OmniboxEditModel::State model_state;
-  const gfx::SelectionModel selection_model;
+  const gfx::Range selection;

[msw, 2013/09/20 01:52:39]

Can you explain here why two selection ranges are needed, and perhaps rename
this one to something more descriptive? I suppose this affords us new
functionality where the current selection is disambiguated from the selection
saved from an OnBlur, so you can now do the following: (1) Make an omnibox
selection. (2) Focus away (3) Switch tabs (4) Switch back to the previous tab
(5) Restore omnibox focus with tab-to-focus (6) The prior selection from #1 will
be restored (where #6 wasn't previously possible).

[Peter Kasting, 2013/09/21 00:36:46]

Yes.  Added a description of that case.

+  const gfx::Range saved_selection_for_focus_change;
 };
 
 // static
 const char OmniboxState::kKey[] = "OmniboxState";
 
 OmniboxState::OmniboxState(const OmniboxEditModel::State& model_state,
-                           const gfx::SelectionModel& selection_model)
+                           const gfx::Range& selection,
+                           const gfx::Range& saved_selection_for_focus_change)
     : model_state(model_state),
-      selection_model(selection_model) {
+      selection(selection),
+      saved_selection_for_focus_change(saved_selection_for_focus_change) {
 }
 
 OmniboxState::~OmniboxState() {}
@@ -129,6 +133,7 @@
     : OmniboxView(profile, controller, command_updater),
       popup_window_mode_(popup_window_mode),
       security_level_(ToolbarModel::NONE),
+      saved_selection_for_focus_change_(gfx::Range::InvalidRange()),
       ime_composing_before_change_(false),
       delete_at_end_pressed_(false),
       location_bar_view_(location_bar),
@@ -186,6 +191,10 @@
   views::Textfield::OnGestureEvent(event);
   if (!HasFocus() && event->type() == ui::ET_GESTURE_TAP_DOWN) {
     select_all_on_gesture_tap_ = true;
+
+    // Don't restore saved selection, it will just screw up our interaction
+    // with this edit.

[msw, 2013/09/20 01:52:39]

nit: What is "this edit"? Do you mean the gesture event handling?
Try to explain what's happening rather than eluding to bad things.

[Peter Kasting, 2013/09/21 00:36:46]

Done.

+    saved_selection_for_focus_change_ = gfx::Range::InvalidRange();
     return;
   }
   if (select_all_on_gesture_tap_ && event->type() == ui::ET_GESTURE_TAP)
@@ -202,12 +211,17 @@
   select_all_on_mouse_release_ =
       (event.IsOnlyLeftMouseButton() || event.IsOnlyRightMouseButton()) &&
       (!HasFocus() || (model()->focus_state() == OMNIBOX_FOCUS_INVISIBLE));
-  // Restore caret visibility whenever the user clicks in the omnibox in a way
-  // that would give it focus.  We must handle this case separately here because
-  // if the omnibox currently has invisible focus, the mouse event won't trigger
-  // either SetFocus() or OmniboxEditModel::OnSetFocus().
-  if (select_all_on_mouse_release_)
+  if (select_all_on_mouse_release_) {
+    // Restore caret visibility whenever the user clicks in the omnibox in a way
+    // that would give it focus.  We must handle this case separately here
+    // because if the omnibox currently has invisible focus, the mouse event
+    // won't trigger either SetFocus() or OmniboxEditModel::OnSetFocus().
     model()->SetCaretVisibility(true);
+
+    // Don't restore saved selection, it will just screw up our interaction
+    // with this edit.

[msw, 2013/09/20 01:52:39]

nit: ditto to comment clarity and value.

[Peter Kasting, 2013/09/21 00:36:46]

Done.

+    saved_selection_for_focus_change_ = gfx::Range::InvalidRange();
+  }
   return views::Textfield::OnMousePressed(event);
 }
 
@@ -329,19 +343,16 @@
   model()->OnSetFocus(false);
   // Don't call controller()->OnSetFocus, this view has already acquired focus.
 
-  // Restore a valid saved selection on tab-to-focus.
-  if (location_bar_view_->GetWebContents() && !select_all_on_mouse_release_) {
-    const OmniboxState* state = static_cast<OmniboxState*>(
-        location_bar_view_->GetWebContents()->GetUserData(&OmniboxState::kKey));
-    if (state)
-      SelectSelectionModel(state->selection_model);
+  // Restore saved selection if available.

[msw, 2013/09/20 01:52:39]

nit: consider "Restore the saved selection from OnBlur if it is valid."

[Peter Kasting, 2013/09/21 00:36:46]

Done.

+  if (saved_selection_for_focus_change_.IsValid()) {
+    SelectRange(saved_selection_for_focus_change_);
+    saved_selection_for_focus_change_ = gfx::Range::InvalidRange();
   }
 }
 
 void OmniboxViewViews::OnBlur() {
-  // Save the selection to restore on tab-to-focus.
-  if (location_bar_view_->GetWebContents())
-    SaveStateToTab(location_bar_view_->GetWebContents());
+  // Save the user's existing selection to restore it later.
+  saved_selection_for_focus_change_ = GetSelectedRange();
 
   views::Textfield::OnBlur();
   gfx::NativeView native_view = NULL;
@@ -357,6 +368,7 @@
   model()->OnWillKillFocus(native_view);
   // Close the popup.
   CloseOmniboxPopup();
+
   // Tell the model to reset itself.
   model()->OnKillFocus();
   controller()->OnKillFocus();
@@ -379,10 +391,11 @@
     GetInputMethod()->CancelComposition(this);
   }
 
-  // NOTE: GetStateForTabSwitch may affect GetSelection, so order is important.
+  // NOTE: GetStateForTabSwitch() may affect GetSelectedRange(), so order is
+  // important.
   OmniboxEditModel::State state = model()->GetStateForTabSwitch();
-  const gfx::SelectionModel selection = GetSelectionModel();
-  tab->SetUserData(OmniboxState::kKey, new OmniboxState(state, selection));
+  tab->SetUserData(OmniboxState::kKey, new OmniboxState(
+      state, GetSelectedRange(), saved_selection_for_focus_change_));
 }
 
 void OmniboxViewViews::OnTabChanged(const content::WebContents* web_contents) {
@@ -391,8 +404,10 @@
   const OmniboxState* state = static_cast<OmniboxState*>(
       web_contents->GetUserData(&OmniboxState::kKey));
   model()->RestoreState(state ? &state->model_state : NULL);
-  if (state)
-    SelectSelectionModel(state->selection_model);
+  if (state) {
+    SelectRange(state->selection);
+    saved_selection_for_focus_change_ = state->saved_selection_for_focus_change;
+  }
 
   // TODO(msw|oshima): Consider saving/restoring edit history.
   ClearEditHistory();
@@ -436,6 +451,13 @@
   return text();
 }
 
+void OmniboxViewViews::SetUserText(const string16& text,
+                                   const string16& display_text,
+                                   bool update_popup) {
+  saved_selection_for_focus_change_ = gfx::Range::InvalidRange();
+  OmniboxView::SetUserText(text, display_text, update_popup);
+}
+
 void OmniboxViewViews::SetWindowTextAndCaretPos(const string16& text,
                                                 size_t caret_pos,
                                                 bool update_popup,
@@ -454,7 +476,7 @@
   const string16 current_text(text());
   const size_t start = current_text.find_first_not_of(kWhitespaceUTF16);
   if (start == string16::npos || (current_text[start] != '?'))
-    SetUserText(ASCIIToUTF16("?"));
+    OmniboxView::SetUserText(ASCIIToUTF16("?"));

[msw, 2013/09/20 01:52:39]

Yuck, an overload! (no action item...)

[Peter Kasting, 2013/09/21 00:36:46]

Yeah.  I find this intensely annoying :(

   else
     SelectRange(gfx::Range(current_text.size(), start + 1));
 }
@@ -479,6 +501,11 @@
   views::Textfield::SelectAll(reversed);
 }
 
+void OmniboxViewViews::RevertAll() {
+  saved_selection_for_focus_change_ = gfx::Range::InvalidRange();
+  OmniboxView::RevertAll();
+}
+
 void OmniboxViewViews::UpdatePopup() {
   model()->SetInputInProgress(true);
   if (!model()->has_focus())