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..30619399518f21269b3d0ba33037eea213b8b07c 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, false);

[Peter Kasting, 2013/02/11 19:54:54]

Why false here?

[Peter Kasting, 2013/02/11 23:53:09]

Didn't answer this.

[beaudoin, 2013/02/12 00:24:37]

I've switched it back to true. I thought using false would let us get rid of an
awkward check we have in DoInstant but it reintroduced an old bug...

       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,8 +1179,15 @@ void OmniboxEditModel::RevertTemporaryText(bool revert_popup) {
   just_deleted_text_ = false;
   has_temporary_text_ = false;
   is_temporary_text_set_by_instant_ = false;
-  if (revert_popup)
+
+  InstantController* instant = controller_->GetInstant();
+  if (instant) {
+    // Update the view text, but ensure

[Peter Kasting, 2013/02/11 19:54:54]

Comment deadends

[beaudoin, 2013/02/11 23:38:22]

Done.

[Peter Kasting, 2013/02/11 23:53:09]

No, it wasn't.

[beaudoin, 2013/02/12 00:24:37]

Sorry. Fell through the cracks. Done now, for real.

+    view_->OnTemporaryTextMaybeChanged(user_text_, false, false);
+    instant->OnCancel();
+  } else if (revert_popup) {
     popup_->ResetToDefaultMatch();
+  }
   view_->OnRevertTemporaryText();
 }
 
@@ -1238,11 +1245,6 @@ bool OmniboxEditModel::DoInstant(const AutocompleteMatch& match) {
   if (!instant || in_revert_)
     return false;
 
-  // Don't call Update() if the change is a result of a
-  // INSTANT_COMPLETE_REPLACE instant suggestion.
-  if (has_temporary_text_ && is_temporary_text_set_by_instant_)
-    return false;

[Alexei Svitkine (slow), 2013/02/11 17:54:43]

On my CL that added this block, Sreeram expressed concern that the logic in
OnChanged() should still run (hence why this if statement wasn't added to
OnChanged(), which was the case in an earlier patchset of mine).

Please double check with him that it makes sense not to run the rest of the
OnChanged() logic in the case where you're passing false now. 

[beaudoin, 2013/02/11 23:38:22]

See comment in main thread.

[beaudoin, 2013/02/12 00:24:37]

After some testing it reintroduced the bug I originally fixed, so I went back to
your fix.

-
   // The two pieces of text we want to send Instant, viz., what the user has
   // typed, and the full omnibox text including any inline autocompletion.
   string16 user_text = has_temporary_text_ ?