Fix a variety of omnibox bugs around zerosuggest, the escape key, or both.

chrome/browser/ui/omnibox/omnibox_edit_model.cc

 // Copyright 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/omnibox/omnibox_edit_model.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 445 matching lines @@
                                          base::string16* text,
                                          GURL* url,
                                          bool* write_url) {
   *write_url = false;
 
   // Do not adjust if selection did not start at the beginning of the field, or
   // if the URL was omitted.
   if ((sel_min != 0) || controller_->GetToolbarModel()->WouldReplaceURL())
     return;
 
-  if (!user_input_in_progress_ && is_all_selected) {
-    // The user selected all the text and has not edited it. Use the url as the
-    // text so that if the scheme was stripped it's added back, and the url
-    // is unescaped (we escape parts of the url for display).
+  // Check whether the user is trying to copy the current page's URL by
+  // selecting the whole thing without editing it.
+  //
+  // This is complicated by ZeroSuggest.  When ZeroSuggest is active, the user
+  // may be selecting different items and thus changing the address bar text,
+  // even though !user_input_in_progress_; and the permanent URL may change
+  // without updating the visible text, just like when user input is in
+  // progress.  In these cases, we don't want to copy the underlying URL, we
+  // want to copy what the user actually sees.  However, if we simply never do
+  // this block when !popup_model()->IsOpen(), then just clicking into the
+  // address bar and trying to copy will always bypass this block on pages that
+  // trigger ZeroSuggest, which is too conservative.  Instead, in the
+  // ZeroSuggest case, we check that (a) the user hasn't selected one of the
+  // other suggestions, and (b) the selected text is still the same as the
+  // permanent text.  ((b) probably implies (a), but it doesn't hurt to be
+  // sure.)  If these hold, then it's safe to copy the underlying URL.
+  if (!user_input_in_progress_ && is_all_selected &&
+      (!popup_model() || !popup_model()->IsOpen() ||
+       ((popup_model()->selected_line() == 0) && (*text == permanent_text_)))) {
+    // It's safe to copy the underlying URL.  These lines ensure that if the
+    // scheme was stripped it's added back, and the URL is unescaped (we escape
+    // parts of it for display).
     *url = PermanentURL();
     *text = base::UTF8ToUTF16(url->spec());
     *write_url = true;
     return;
   }
 
   // We can't use CurrentTextIsURL() or GetDataForURLExport() because right now
   // the user is probably holding down control to cause the copy, which will
   // screw up our calculation of the desired_tld.
   AutocompleteMatch match;
@@ skipping 498 matching lines @@
     delegate_->OnInputStateChanged();
 }
 
 void OmniboxEditModel::OnKillFocus() {
   SetFocusState(OMNIBOX_FOCUS_NONE, OMNIBOX_FOCUS_CHANGE_EXPLICIT);
   focus_source_ = INVALID;
   control_key_state_ = UP;
   paste_state_ = NONE;
 }
 
+bool OmniboxEditModel::WillHandleEscapeKey() const {
+  return user_input_in_progress_ ||
+      (has_temporary_text_ &&
+       (CurrentMatch(NULL).destination_url != original_url_));
+}
+
 bool OmniboxEditModel::OnEscapeKeyPressed() {
-  const AutocompleteMatch& match = CurrentMatch(NULL);
-  if (has_temporary_text_) {
-    if (match.destination_url != original_url_) {
-      RevertTemporaryText(true);
-      return true;
-    }
+  if (has_temporary_text_ &&
+      (CurrentMatch(NULL).destination_url != original_url_)) {
+    RevertTemporaryText(true);
+    return true;
   }
 
   // We do not clear the pending entry from the omnibox when a load is first
-  // stopped.  If the user presses Escape while stopped, we clear it.
+  // stopped.  If the user presses Escape while stopped, whether editing or not,
+  // we clear it.
   if (delegate_->CurrentPageExists() && !delegate_->IsLoading()) {
     delegate_->GetNavigationController().DiscardNonCommittedEntries();
     view_->Update();
   }
 
-  // If the user wasn't editing, but merely had focus in the edit, allow <esc>
-  // to be processed as an accelerator, so it can still be used to stop a load.
-  // When the permanent text isn't all selected we still fall through to the
-  // SelectAll() call below so users can arrow around in the text and then hit
-  // <esc> to quickly replace all the text; this matches IE.
-  const bool has_zero_suggest_match = match.provider &&
-      (match.provider->type() == AutocompleteProvider::TYPE_ZERO_SUGGEST);
-  if (!has_zero_suggest_match && !user_input_in_progress_ &&
-      view_->IsSelectAll())
-    return false;
-
   if (!user_text_.empty()) {
     UMA_HISTOGRAM_ENUMERATION(kOmniboxUserTextClearedHistogram,
                               OMNIBOX_USER_TEXT_CLEARED_WITH_ESCAPE,
                               OMNIBOX_USER_TEXT_CLEARED_NUM_OF_ITEMS);
   }
 
+  // Unconditionally revert/select all.  This ensures any popup, whether due to
+  // normal editing or zerosuggest, is closed, and the full text is selected.

[H Fung, 2015/04/07 00:38:23]

ZeroSuggest to be consistent with earlier comments?

[Peter Kasting, 2015/04/07 02:18:16]

Done.

+  // This latter allows the user to hit escape to quickly select all for ease of

[H Fung, 2015/04/07 00:38:23]

This->The (I'm assuming you're referring to select all)

By replacement, you mean typing will clear the omnibox?

[Peter Kasting, 2015/04/07 02:18:16]

I tweaked the comment slightly to read better.
Right, eliminating the omnibox text and replacing it with something different.

+  // replacement, and matches other browsers.
+  bool user_input_was_in_progress = user_input_in_progress_;
   view_->RevertAll();
   view_->SelectAll(true);
-  return true;
+
+  // If the user was in the midst of editing, don't cancel any underlying page
+  // load.  This doesn't match IE or Firefox, but seems more correct.  Note that
+  // we do allow the page load to be stopped in the case where zerosuggest was
+  // visible; this is so that it's still possible to focus the address bar and
+  // hit escape once to stop a load even if the address being loaded triggers
+  // the zerosuggest popup.
+  return user_input_was_in_progress;
 }
 
 void OmniboxEditModel::OnControlKeyChanged(bool pressed) {
   if (pressed == (control_key_state_ == UP))
     control_key_state_ = pressed ? DOWN_WITHOUT_CHANGE : UP;
 }
 
 void OmniboxEditModel::OnPaste() {
   UMA_HISTOGRAM_COUNTS("Omnibox.Paste", 1);
   paste_state_ = PASTING;
@@ skipping 435 matching lines @@
   // Update state and notify view if the omnibox has focus and the caret
   // visibility changed.
   const bool was_caret_visible = is_caret_visible();
   focus_state_ = state;
   if (focus_state_ != OMNIBOX_FOCUS_NONE &&
       is_caret_visible() != was_caret_visible)
     view_->ApplyCaretVisibility();
 
   delegate_->OnFocusChanged(focus_state_, reason);
 }