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, 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;
     }
   }
 }
 
 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;
-  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();
 }
 
 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) &&
       IsSpaceCharForAcceptingKeyword(new_text[keyword_length]) &&
       !new_text.compare(0, keyword_length, keyword_, 0, keyword_length) &&
@@ skipping 37 matching lines @@
         controller_->GetWebContents())->
             OmniboxEditModelChanged(user_input_in_progress_, !in_revert_);
   }
 }
 
 bool OmniboxEditModel::DoInstant(const AutocompleteMatch& match) {
   InstantController* instant = controller_->GetInstant();
   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_ ?
       match.fill_into_edit : DisplayTextFromUserText(user_text_);
   string16 full_text = view_->GetText();
 
   // Remove "?" if we're in forced query mode.
   AutocompleteInput::RemoveForcedQueryStringIfNecessary(
       autocomplete_controller_->input().type(), &user_text);
   AutocompleteInput::RemoveForcedQueryStringIfNecessary(
@@ skipping 85 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();
 }