Fixing ESC in instant-extended.

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 1e1c3fc06acc3106c88bd1add494cd9eba160727..056ea890ea4b584bf865db0ed8e4cca2fc123980 100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -220,7 +220,7 @@ void OmniboxEditModel::SetInstantSuggestion(
       keyword_ = string16();
       is_keyword_hint_ = false;
       view_->OnTemporaryTextMaybeChanged(suggestion.text,
-                                         save_original_selection);
+                                         save_original_selection, true);
       break;
     }
   }
@@ -705,7 +705,7 @@ bool OmniboxEditModel::AcceptKeyword() {
   is_temporary_text_set_by_instant_ = false;
   view_->OnTemporaryTextMaybeChanged(
       DisplayTextFromUserText(CurrentMatch().fill_into_edit),
-      save_original_selection);
+      save_original_selection, true);
 
   content::RecordAction(UserMetricsAction("AcceptedKeywordHint"));
   return true;
@@ -795,7 +795,7 @@ void OmniboxEditModel::OnKillFocus() {
 }
 
 bool OmniboxEditModel::OnEscapeKeyPressed() {
-  if (has_temporary_text_ && !is_temporary_text_set_by_instant_) {
+  if (has_temporary_text_) {
     AutocompleteMatch match;
     InfoForCurrentSelection(&match, NULL);
     if (match.destination_url != original_url_) {
@@ -920,7 +920,7 @@ void OmniboxEditModel::OnPopupDataChanged(
       // right answer here :(
     }
     view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(text),
-                                       save_original_selection);
+                                       save_original_selection, true);
     return;
   }
 
@@ -1179,6 +1179,14 @@ void OmniboxEditModel::RevertTemporaryText(bool revert_popup) {
   just_deleted_text_ = false;
   has_temporary_text_ = false;
   is_temporary_text_set_by_instant_ = false;
+
+  InstantController* instant = controller_->GetInstant();
+  if (instant) {
+    // Update the view text, do not notify the model in order not to trigger an
+    // unwanted call to instant->Update().
+    view_->OnTemporaryTextMaybeChanged(user_text_, false, false);
+    instant->OnCancel(user_text_);

[sreeram, 2013/02/12 18:12:29]

It took me some time to figure out what's going on here. I think this warrants a
more detailed comment. Something like:

"""
Normally, popup_->ResetToDefaultMatch() will cause the view text to be updated.
In Instant Extended mode however, the popup_ is not used, so it won't do
anything. So, update the view ourselves. Even if Instant is not in extended mode
(i.e., it's enabled in non-extended mode, or disabled altogether), this is okay
to do, since the call to popup_->ResetToDefaultMatch() will just override
whatever we do here.

The two "false" arguments make sure that our shenanigans don't cause any
previously saved selection to be erased nor OnChanged() to be called.
"""

Which raises the questions:
1. Why not use view_->SetWindowTextAndCaretPos() instead of
OnTemporaryTextMaybeChanged() here (and then do an early return)?
2. Maybe do this only if "is_temporary_text_set_by_instant_" was true in the
first place?

[beaudoin, 2013/02/12 20:27:30]

Now using your comment.
1) --> discussed over chat, leaving it like this.
2) I've gated that code on the original value of
is_temporary_text_set_by_instant_

+  }
   if (revert_popup)
     popup_->ResetToDefaultMatch();
   view_->OnRevertTemporaryText();