Fix multiple problems with omnibox text handling across focus changes.

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

 // Copyright (c) 2012 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 "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 50 matching lines @@
 #include "ui/compositor/layer.h"
 #endif
 
 namespace {
 
 // Stores omnibox state for each tab.
 struct OmniboxState : public base::SupportsUserData::Data {
   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() {}
 
 // We'd like to set the text input type to TEXT_INPUT_TYPE_URL, because this
 // triggers URL-specific layout in software keyboards, e.g. adding top-level "/"
 // and ".com" keys for English.  However, this also causes IMEs to default to
 // Latin character mode, which makes entering search queries difficult for IME
 // users.  Therefore, we try to guess whether an IME will be used based on the
 // application language, and set the input type accordingly.
@@ skipping 27 matching lines @@
 OmniboxViewViews::OmniboxViewViews(OmniboxEditController* controller,
                                    Profile* profile,
                                    CommandUpdater* command_updater,
                                    bool popup_window_mode,
                                    LocationBarView* location_bar,
                                    const gfx::FontList& font_list,
                                    int font_y_offset)
     : 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),
       ime_candidate_window_open_(false),
       select_all_on_mouse_release_(false),
       select_all_on_gesture_tap_(false) {
   RemoveBorder();
   set_id(VIEW_ID_OMNIBOX);
   SetFontList(font_list);
   SetVerticalMargins(font_y_offset, 0);
@@ skipping 37 matching lines @@
 // OmniboxViewViews, views::Textfield implementation:
 
 const char* OmniboxViewViews::GetClassName() const {
   return kViewClassName;
 }
 
 void OmniboxViewViews::OnGestureEvent(ui::GestureEvent* event) {
   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)
     SelectAll(false);
   select_all_on_gesture_tap_ = false;
 }
 
 void OmniboxViewViews::GetAccessibleState(ui::AccessibleViewState* state) {
   location_bar_view_->GetAccessibleState(state);
   state->role = ui::AccessibilityTypes::ROLE_TEXT;
 }
 
 bool OmniboxViewViews::OnMousePressed(const ui::MouseEvent& event) {
   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);
 }
 
 bool OmniboxViewViews::OnMouseDragged(const ui::MouseEvent& event) {
   select_all_on_mouse_release_ = false;
   return views::Textfield::OnMouseDragged(event);
 }
 
 void OmniboxViewViews::OnMouseReleased(const ui::MouseEvent& event) {
   views::Textfield::OnMouseReleased(event);
@@ skipping 101 matching lines @@
 
   return Textfield::SkipDefaultKeyEventProcessing(event);
 }
 
 void OmniboxViewViews::OnFocus() {
   views::Textfield::OnFocus();
   // TODO(oshima): Get control key state.
   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;
 #if defined(USE_AURA)
   views::Widget* widget = GetWidget();
   if (widget) {
     aura::client::FocusClient* client =
         aura::client::GetFocusClient(widget->GetNativeView());
     if (client)
       native_view = client->GetFocusedWindow();
   }
 #endif
   model()->OnWillKillFocus(native_view);
   // Close the popup.
   CloseOmniboxPopup();
+
   // Tell the model to reset itself.
   model()->OnKillFocus();
   controller()->OnKillFocus();
 
   // Make sure the beginning of the text is visible.
   SelectRange(gfx::Range(0));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxViewViews, OmniboxView implementation:
 
 void OmniboxViewViews::SaveStateToTab(content::WebContents* tab) {
   DCHECK(tab);
 
   // We don't want to keep the IME status, so force quit the current
   // session here.  It may affect the selection status, so order is
   // also important.
   if (IsIMEComposing()) {
     GetTextInputClient()->ConfirmCompositionText();
     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) {
   security_level_ = controller()->GetToolbarModel()->GetSecurityLevel(false);
 
   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();
 }
 
 void OmniboxViewViews::Update() {
   const ToolbarModel::SecurityLevel old_security_level = security_level_;
   security_level_ = controller()->GetToolbarModel()->GetSecurityLevel(false);
   if (model()->UpdatePermanentText()) {
     // Something visibly changed.  Re-enable search term replacement.
@@ skipping 23 matching lines @@
   } else if (old_security_level != security_level_) {
     EmphasizeURLComponents();
   }
 }
 
 string16 OmniboxViewViews::GetText() const {
   // TODO(oshima): IME support
   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,
                                                 bool notify_text_changed) {
   const gfx::Range range(caret_pos, caret_pos);
   SetTextAndSelectedRange(text, range);
 
   if (update_popup)
     UpdatePopup();
 
   if (notify_text_changed)
     TextChanged();
 }
 
 void OmniboxViewViews::SetForcedQuery() {
   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));
 }
 
 bool OmniboxViewViews::IsSelectAll() const {
   // TODO(oshima): IME support.
   return text() == GetSelectedText();
 }
 
 bool OmniboxViewViews::DeleteAtEndPressed() {
   return delete_at_end_pressed_;
 }
 
 void OmniboxViewViews::GetSelectionBounds(string16::size_type* start,
                                           string16::size_type* end) const {
   const gfx::Range range = GetSelectedRange();
   *start = static_cast<size_t>(range.start());
   *end = static_cast<size_t>(range.end());
 }
 
 void OmniboxViewViews::SelectAll(bool reversed) {
   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())
     return;
 
   // Hide the inline autocompletion for IME users.
   location_bar_view_->SetImeInlineAutocompletion(string16());
 
   // Prevent inline autocomplete when the caret isn't at the end of the text,
   // and during IME composition editing unless
@@ skipping 428 matching lines @@
   const string16 text(GetClipboardText());
   if (!text.empty()) {
     // Record this paste, so we can do different behavior.
     model()->on_paste();
     // Force a Paste operation to trigger the text_changed code in
     // OnAfterPossibleChange(), even if identical contents are pasted.
     text_before_change_.clear();
     InsertOrReplaceText(text);
   }
 }