Use Instant suggested match type for Instant temporary text.

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 104 matching lines @@
                                    OmniboxEditController* controller,
                                    Profile* profile)
     : view_(view),
       popup_(NULL),
       controller_(controller),
       focus_state_(OMNIBOX_FOCUS_NONE),
       user_input_in_progress_(false),
       just_deleted_text_(false),
       has_temporary_text_(false),
       is_temporary_text_set_by_instant_(false),
+      is_instant_temporary_text_a_search_query_(false),
       paste_state_(NONE),
       control_key_state_(UP),
       is_keyword_hint_(false),
       profile_(profile),
       in_revert_(false),
       in_escape_handler_(false),
       allow_exact_keyword_match_(false) {
   // Use a restricted subset of the autocomplete providers if we're using the
   // Instant Extended API, as it doesn't support them all.
   autocomplete_controller_.reset(new AutocompleteController(profile, this,
@@ skipping 77 matching lines @@
 GURL OmniboxEditModel::PermanentURL() {
   return URLFixerUpper::FixupURL(UTF16ToUTF8(permanent_text_), std::string());
 }
 
 void OmniboxEditModel::SetUserText(const string16& text) {
   SetInputInProgress(true);
   InternalSetUserText(text);
   paste_state_ = NONE;
   has_temporary_text_ = false;
   is_temporary_text_set_by_instant_ = false;
+  is_instant_temporary_text_a_search_query_ = false;
 }
 
 void OmniboxEditModel::FinalizeInstantQuery(const string16& input_text,
                                             const InstantSuggestion& suggestion,
                                             bool skip_inline_autocomplete) {
   if (skip_inline_autocomplete) {
     const string16 final_text = input_text + suggestion.text;
     view_->OnBeforePossibleChange();
     view_->SetWindowTextAndCaretPos(final_text, final_text.length(), false,
         false);
@@ skipping 12 matching lines @@
   switch (suggestion.behavior) {
     case INSTANT_COMPLETE_NOW:
       view_->SetInstantSuggestion(string16());
       if (!suggestion.text.empty())
         FinalizeInstantQuery(view_->GetText(), suggestion, false);
       break;
 
     case INSTANT_COMPLETE_NEVER: {
       DCHECK_EQ(INSTANT_SUGGESTION_SEARCH, suggestion.type);
       view_->SetInstantSuggestion(suggestion.text);
-      SearchProvider* search_provider =
-          autocomplete_controller_->search_provider();
-      if (search_provider)

[Mark P, 2013/04/09 23:59:11]

I know this shouldn't happen in normal use.  Nevertheless, I'm curious: why did
you bother reminding this guard?

[sreeram, 2013/04/11 20:37:54]

It shouldn't happen at all, whether in normal use or in unit tests, so that's
why I removed it. When I added the code in GetInfoForCurrentText() to call
search_provider()->GetSearchWhatYouTypedMatch(), I knew I didn't need the guard,
so I went looking to see where else we had such a guard that isn't necessary.

In contrast, the guard _is_ needed in FinalizeInstantQuery(), as the comment
there says, because of unit tests.

-        search_provider->ClearInstantSuggestion();
+      autocomplete_controller_->search_provider()->ClearInstantSuggestion();
       break;
     }
 
     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;
+      is_instant_temporary_text_a_search_query_ =
+          suggestion.type == INSTANT_SUGGESTION_SEARCH;
       // Instant suggestions are never a keyword.
       keyword_ = string16();
       is_keyword_hint_ = false;
       view_->OnTemporaryTextMaybeChanged(suggestion.text,
                                          save_original_selection, true);
       break;
     }
   }
 }
 
@@ skipping 194 matching lines @@
 }
 
 void OmniboxEditModel::Revert() {
   SetInputInProgress(false);
   paste_state_ = NONE;
   InternalSetUserText(string16());
   keyword_.clear();
   is_keyword_hint_ = false;
   has_temporary_text_ = false;
   is_temporary_text_set_by_instant_ = false;
+  is_instant_temporary_text_a_search_query_ = false;
   view_->SetWindowTextAndCaretPos(permanent_text_,
                                   has_focus() ? permanent_text_.length() : 0,
                                   false, true);
   AutocompleteActionPredictor* action_predictor =
       AutocompleteActionPredictorFactory::GetForProfile(profile_);
   if (action_predictor)
     action_predictor->ClearTransitionalMatches();
 }
 
 void OmniboxEditModel::StartAutocomplete(
@@ skipping 260 matching lines @@
     popup_->SetSelectedLineState(OmniboxPopupModel::KEYWORD);
   else
     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;
+  is_instant_temporary_text_a_search_query_ = false;
   view_->OnTemporaryTextMaybeChanged(
       DisplayTextFromUserText(CurrentMatch().fill_into_edit),
       save_original_selection, true);
 
   content::RecordAction(UserMetricsAction("AcceptedKeywordHint"));
   return true;
 }
 
 void OmniboxEditModel::ClearKeyword(const string16& visible_text) {
   autocomplete_controller_->Stop(false);
@@ skipping 116 matching lines @@
   // Don't change anything unless the key state is actually toggling.
   if (pressed == (control_key_state_ == UP)) {
     ControlKeyState old_state = control_key_state_;
     control_key_state_ = pressed ? DOWN_WITHOUT_CHANGE : UP;
     if ((control_key_state_ == DOWN_WITHOUT_CHANGE) && has_temporary_text_) {
       // Arrowing down and then hitting control accepts the temporary text as
       // the input text.
       InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
       has_temporary_text_ = false;
       is_temporary_text_set_by_instant_ = false;
+      is_instant_temporary_text_a_search_query_ = false;
     }
     if ((old_state != DOWN_WITH_CHANGE) && popup_->IsOpen()) {
       // Autocomplete history provider results may change, so refresh the
       // popup.  This will force user_input_in_progress_ to true, but if the
       // popup is open, that should have already been the case.
       view_->UpdatePopup();
     }
   }
 }
 
@@ skipping 48 matching lines @@
     DCHECK(!keyword_.empty() || !is_keyword_hint_);
   }
 
   // Handle changes to temporary text.
   if (destination_for_temporary_text_change != NULL) {
     const bool save_original_selection = !has_temporary_text_;
     if (save_original_selection) {
       // Save the original selection and URL so it can be reverted later.
       has_temporary_text_ = true;
       is_temporary_text_set_by_instant_ = false;
+      is_instant_temporary_text_a_search_query_ = false;
       original_url_ = *destination_for_temporary_text_change;
       inline_autocomplete_text_.clear();
     }
     if (control_key_state_ == DOWN_WITHOUT_CHANGE) {
       // 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
@@ skipping 85 matching lines @@
     return false;
   }
 
   // If the user text has not changed, we do not want to change the model's
   // state associated with the text.  Otherwise, we can get surprising behavior
   // where the autocompleted text unexpectedly reappears, e.g. crbug.com/55983
   if (user_text_changed) {
     InternalSetUserText(UserTextFromDisplayText(new_text));
     has_temporary_text_ = false;
     is_temporary_text_set_by_instant_ = false;
+    is_instant_temporary_text_a_search_query_ = false;
 
     // Track when the user has deleted text so we won't allow inline
     // autocomplete.
     just_deleted_text_ = just_deleted_text;
 
     if (user_input_in_progress_ && user_text_.empty()) {
       // Log cases where the user started editing and then subsequently cleared
       // all the text.  Note that this explicitly doesn't catch cases like
       // "hit ctrl-l to select whole edit contents, then hit backspace", because
       // in such cases, |user_input_in_progress| won't be true here.
@@ skipping 73 matching lines @@
   }
 
   if (popup_->IsOpen()) {
     OnPopupBoundsChanged(popup_->view()->GetTargetBounds());
   } else if (was_open) {
     // Accepts the temporary text as the user text, because it makes little
     // sense to have temporary text when the popup is closed.
     InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
     has_temporary_text_ = false;
     is_temporary_text_set_by_instant_ = false;
+    is_instant_temporary_text_a_search_query_ = false;
     OnPopupBoundsChanged(gfx::Rect());
     delegate_->NotifySearchTabHelper(user_input_in_progress_, !in_revert_);
   }
 
   InstantController* instant = controller_->GetInstant();
   if (instant && !in_revert_)
     instant->HandleAutocompleteResults(*autocomplete_controller_->providers());
 }
 
 bool OmniboxEditModel::query_in_progress() const {
@@ skipping 47 matching lines @@
     CHECK(popup_->selected_line() < result.size());
     *match = result.match_at(popup_->selected_line());
   }
   if (alternate_nav_url && popup_->manually_selected_match().empty())
     *alternate_nav_url = result.alternate_nav_url();
 }
 
 void OmniboxEditModel::GetInfoForCurrentText(AutocompleteMatch* match,
                                              GURL* alternate_nav_url) const {
   // If there's temporary text and it has been set by Instant, we won't find it
-  // in the popup model, so classify the text anew.
-  if ((popup_->IsOpen() || query_in_progress()) &&
-      !is_temporary_text_set_by_instant_) {
+  // in the popup model, so create the match based on the type Instant told us
+  // (SWYT for queries and UWYT for URLs). We do this instead of classifying the
+  // text ourselves because the text may look like a URL, but Instant may expect
+  // it to be a search (e.g.: a query for "amazon.com").
+  if (is_temporary_text_set_by_instant_) {
+    const string16& text = view_->GetText();
+    if (is_instant_temporary_text_a_search_query_) {
+      autocomplete_controller_->search_provider()->GetSearchWhatYouTypedMatch(
+          text, text, TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, false, match);

[Mark P, 2013/04/09 23:59:11]

Are there cases when the match you want to provide isn't identical to the input
string?  For instance, normal usage strips the ? from the beginning of omnibox
inputs.  Do you need to do that here too?

[Mark P, 2013/04/09 23:59:11]

Isn't using TemplateURLRef::NO_SUGGESTIONS_AVAILABLE a lie?  Can't there be
suggestions available here?

[sreeram, 2013/04/11 20:37:54]

Not needed. Even if you type a query starting with "?", and you arrow up/down,
the Instant suggested text (i.e., |text| here) won't have a "?".

[sreeram, 2013/04/11 20:48:58]

You're right. Good catch.

When we are using the local fallback Instant overlay (i.e., not the
Google-provided overlay), the SearchProvider will retrieve suggestions
(http://crbug.com/225245) and therefore we should set this correctly. Working on
it...

+    } else {
+      AutocompleteInput input(text, string16::npos, string16(), GURL(), false,

[Mark P, 2013/04/09 23:59:11]

what's wrong with passing the actual current URL here?

[Mark P, 2013/04/09 23:59:11]

I'm pretty sure it's not used, but I think you might as well set
prevent_inline_autocomplete to true.  It seems more appropriate.

[sreeram, 2013/04/11 20:37:54]

Nothing wrong. I've added a comment. Please see if that is satisfactory. If not,
I'll add the current URL.

[sreeram, 2013/04/11 20:37:54]

As above, I've added a comment as to why I'm mostly passing in "default" values.
Passing "true" makes it look as though it's required for a non-obvious reason,
so let me know if the comment isn't sufficient.

+                              false, false, AutocompleteInput::BEST_MATCH);
+      *match = HistoryURLProvider::SuggestExactInput(
+        autocomplete_controller_->history_url_provider(), input, false);

[Mark P, 2013/04/09 23:59:11]

Is it correct to always trim the http?  Maybe it should depend on what the input
looks like?

[sreeram, 2013/04/11 20:37:54]

The false indicates to NOT trim the http. The trimming only affects the
"contents", i.e., what would be displayed if this match were to be displayed in
a dropdown, for example. I've added a comment explaining that it actually won't
be used for such purposes. Let me know if that doesn't work for you.

+    }
+  } else if (popup_->IsOpen() || query_in_progress()) {
     InfoForCurrentSelection(match, alternate_nav_url);
   } else {
     AutocompleteClassifierFactory::GetForProfile(profile_)->Classify(
         UserTextFromDisplayText(view_->GetText()), KeywordIsSelected(), true,
         match, alternate_nav_url);
   }
 }
 
 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_.
   bool notify_instant = is_temporary_text_set_by_instant_;
   just_deleted_text_ = false;
   has_temporary_text_ = false;
   is_temporary_text_set_by_instant_ = false;
+  is_instant_temporary_text_a_search_query_ = false;
 
   InstantController* instant = controller_->GetInstant();
   if (instant && notify_instant) {
     // 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.
     //
@@ skipping 50 matching lines @@
   return !keyword.empty() && autocomplete_controller_->keyword_provider() &&
       !autocomplete_controller_->keyword_provider()->
           GetKeywordForText(keyword).empty();
 }
 
 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
+  // Don't call Update() if the change is the result of an
   // INSTANT_COMPLETE_REPLACE instant suggestion.
-  if (has_temporary_text_ && is_temporary_text_set_by_instant_)
+  if (is_temporary_text_set_by_instant_)
     return false;
 
   // 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(
@@ skipping 69 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();
 }