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

chrome/browser/ui/omnibox/omnibox_edit_model.cc

Index: chrome/browser/ui/omnibox/omnibox_edit_model.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
index b5bb65d2ac1709ca96816ba0484b95acb017ab02..141488f43327b8533a0b1ff946f3dcea4ec90636 100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -463,10 +463,28 @@ void OmniboxEditModel::AdjustTextForCopy(int sel_min,
   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;
@@ -985,42 +1003,48 @@ void OmniboxEditModel::OnKillFocus() {
   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) {