Fixing ESC in instant-extended.

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 <string>
 
 #include "base/auto_reset.h"
 #include "base/format_macros.h"
@@ skipping 202 matching lines @@
 
     case INSTANT_COMPLETE_REPLACE: {
       const bool save_original_selection = !has_temporary_text_;
       view_->SetInstantSuggestion(string16());
       has_temporary_text_ = true;
       is_temporary_text_set_by_instant_ = true;
       // Instant suggestions are never a keyword.
       keyword_ = string16();
       is_keyword_hint_ = false;
       view_->OnTemporaryTextMaybeChanged(suggestion.text,
-                                         save_original_selection);
+                                         save_original_selection, true);
       break;
     }
   }
 }
 
 bool OmniboxEditModel::CommitSuggestedText(bool skip_inline_autocomplete) {
   if (!controller_->GetInstant())
     return false;
 
   const string16 suggestion = view_->GetInstantSuggestion();
@@ skipping 464 matching lines @@
     StartAutocomplete(false, true);
 
   // Ensure the current selection is saved before showing keyword mode
   // so that moving to another line and then reverting the text will restore
   // the current state properly.
   bool save_original_selection = !has_temporary_text_;
   has_temporary_text_ = true;
   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;
 }
 
 void OmniboxEditModel::ClearKeyword(const string16& visible_text) {
   autocomplete_controller_->Stop(false);
   ClearPopupKeywordMode();
 
   const string16 window_text(keyword_ + visible_text);
@@ skipping 69 matching lines @@
 void OmniboxEditModel::OnKillFocus() {
   // TODO(samarth): determine if it is safe to move the call to
   // OmniboxFocusChanged() from OnWillKillFocus() to here, which would let us
   // just call SetFocusState() to handle the state change.
   focus_state_ = OMNIBOX_FOCUS_NONE;
   control_key_state_ = UP;
   paste_state_ = NONE;
 }
 
 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_) {
       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.
@@ skipping 104 matching lines @@
       // Arrowing around the popup cancels control-enter.
       control_key_state_ = DOWN_WITH_CHANGE;
       // Now things are a bit screwy: the desired_tld has changed, but if we
       // update the popup, the new order of entries won't match the old, so the
       // user's selection gets screwy; and if we don't update the popup, and the
       // user reverts, then the selected item will be as if control is still
       // pressed, even though maybe it isn't any more.  There is no obvious
       // right answer here :(
     }
     view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(text),
-                                       save_original_selection);
+                                       save_original_selection, true);
     return;
   }
 
   bool call_controller_onchanged = true;
   inline_autocomplete_text_ = text;
 
   if (keyword_state_changed && KeywordIsSelected()) {
     // If we reach here, the user most likely entered keyword mode by inserting
     // a space between a keyword name and a search string (as pressing space or
     // tab after the keyword name alone would have been be handled in
@@ skipping 238 matching lines @@
   }
 }
 
 void OmniboxEditModel::RevertTemporaryText(bool revert_popup) {
   // The user typed something, then selected a different item.  Restore the
   // text they typed and change back to the default item.
   // NOTE: This purposefully does not reset paste_state_.
   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();
 }
 
 bool OmniboxEditModel::MaybeAcceptKeywordBySpace(const string16& new_text) {
   size_t keyword_length = new_text.length() - 1;
   return (paste_state_ == NONE) && is_keyword_hint_ && !keyword_.empty() &&
       inline_autocomplete_text_.empty() &&
       (keyword_.length() == keyword_length) &&
@@ skipping 149 matching lines @@
     instant->OmniboxFocusChanged(state, reason, NULL);
 
   // 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();
 }