Omnibox refactor. OmniboxController now holds an AutocompleteMatch.

chrome/browser/ui/omnibox/omnibox_controller.cc

 // Copyright 2013 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_controller.h"
 
 #include "base/metrics/histogram.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/search_provider.h"
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/predictors/autocomplete_action_predictor.h"
 #include "chrome/browser/prerender/prerender_field_trial.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_manager_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
+#include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/omnibox/omnibox_edit_controller.h"
 #include "chrome/browser/ui/omnibox/omnibox_edit_model.h"
 #include "chrome/browser/ui/omnibox/omnibox_popup_model.h"
 #include "chrome/browser/ui/omnibox/omnibox_popup_view.h"
 #include "chrome/browser/ui/search/instant_controller.h"
 #include "extensions/common/constants.h"
 #include "ui/gfx/rect.h"
 
 using predictors::AutocompleteActionPredictor;
 
+namespace {
+
+string16 GetDefaultSearchProviderKeyword(Profile* profile) {
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile);
+  if (template_url_service) {
+    TemplateURL* template_url =
+        template_url_service->GetDefaultSearchProvider();
+    if (template_url)
+      return template_url->keyword();
+  }
+  return string16();
+}
+
+}  // namespace
 
 OmniboxController::OmniboxController(OmniboxEditModel* omnibox_edit_model,
                                      Profile* profile)
     : omnibox_edit_model_(omnibox_edit_model),
       profile_(profile) {
   autocomplete_controller_.reset(new AutocompleteController(profile, this,
       chrome::IsInstantExtendedAPIEnabled() ?
           AutocompleteClassifier::kInstantExtendedOmniboxProviders :
           AutocompleteClassifier::kDefaultOmniboxProviders));
 }
@@ skipping 31 matching lines @@
       AutocompleteInput::ALL_MATCHES));
 }
 
 void OmniboxController::OnResultChanged(bool default_match_changed) {
   // TODO(beaudoin): There should be no need to access the popup when using
   // instant extended, remove this reference.
   const bool was_open = popup_->IsOpen();
   if (default_match_changed) {
     // The default match has changed, we need to let the OmniboxEditModel know
     // about new inline autocomplete text (blue highlight).
-    string16 inline_autocomplete_text;
-    string16 keyword;
-    bool is_keyword_hint = false;
     const AutocompleteResult& result = this->result();
     const AutocompleteResult::const_iterator match(result.default_match());
     if (match != result.end()) {
+      current_match_ = *match;
+      // The |fill_into_edit| we get may not match what we have in the view at
+      // that time. We're only interested in the inline_autocomplete part, so
+      // update this here.
+      current_match_.fill_into_edit = omnibox_edit_model_->user_text();

[Peter Kasting, 2013/06/11 22:42:55]

I confess, I have no idea what you're doing here.

[beaudoin, 2013/06/14 21:39:38]

This is the "blue text" completion path. In an ideal world, we could just set
fill_into_edit into the textbox, selecting everything after the
inline_autocomplete_offset. Unfortunately, the prefix of match->fill_into_edit
may be totally different from what is currently in the textbox in some
situations. I've spotted a bunch of cases but don't remember them all. Off the
top of my head:
- fill_into_edit can have a "www." prefix even if you don't type it
- fill_into_edit can use spaces where you use "%20"

In short: fill_into_edit is what you'd like to place in the text field if the
user arrowed on that match.

Here we want a different fill_into_edit: we want what the user has typed until
now + whatever blue completion this AutocompleteMatch says we should use. Hence
the current code.

[Peter Kasting, 2013/06/15 00:29:21]

It seems like the reason we need to change this is because the OmniboxEditModel
is eventually going to use this |fill_into_edit| text to set its inline
autocompletion.

This suggests to me that perhaps instead of getting a |fill_into_edit| from an
AutocompleteMatch to compute the inline autocompletion, we should just calculate
a string16 "here's your inline autocompletion text" and then ultimately supply
that to the OmniboxEditModel.

Maybe this dovetails with your idea of moving away from an AutocompleteMatch
holding the "current match" details and towards some other struct.  It also
might not make sense to do in this change if it's too involved or too tied to
that larger change.

Incidentally, I have considered at times making this change even in
AutocompleteMatch -- instead of a size_t |inline_autocomplete_offset|, we'd just
have a string16 |inline_autocompletion|, and providers would set that.  It'd
probably be clearer to understand.

[beaudoin, 2013/06/17 17:22:33]

Yes, I think it would make it easier to understand and simplify this code to
have an |inline_autocompletion| field in AutocompleteMatch. I'll mark it as a
TODO.

Looking ahead a little: if we use a second AutocompleteMatch to hold the
suggestion coming from instant then this |inline_autocompletion| could hold the
gray text. (ie. it's rendered in gray because it's coming from the
|alternate_match|.

       if ((match->inline_autocomplete_offset != string16::npos) &&

[Peter Kasting, 2013/06/11 22:42:55]

Nit: First condition not necessary, see comments in omnibox_edit_model.cc

[beaudoin, 2013/06/14 21:39:38]

Done.

           (match->inline_autocomplete_offset <
            match->fill_into_edit.length())) {
-        inline_autocomplete_text =
-            match->fill_into_edit.substr(match->inline_autocomplete_offset);
+        current_match_.inline_autocomplete_offset =
+            current_match_.fill_into_edit.length();
+        current_match_.fill_into_edit += match->fill_into_edit.substr(
+            match->inline_autocomplete_offset);
+      } else {
+        current_match_.inline_autocomplete_offset = string16::npos;
       }
 
       if (!prerender::IsOmniboxEnabled(profile_))
         DoPreconnect(*match);
-
-      // We could prefetch the alternate nav URL, if any, but because there
-      // can be many of these as a user types an initial series of characters,
-      // the OS DNS cache could suffer eviction problems for minimal gain.
-
-      match->GetKeywordUIState(profile_, &keyword, &is_keyword_hint);
+      omnibox_edit_model_->OnCurrentMatchChanged(false);
+    } else {
+      InvalidateCurrentMatch();
+      popup_->OnResultChanged();
+      omnibox_edit_model_->OnPopupDataChanged(string16(), NULL, string16(),
+                                              false);
     }
-
-    popup_->OnResultChanged();
-    omnibox_edit_model_->OnPopupDataChanged(inline_autocomplete_text, NULL,
-                                            keyword, is_keyword_hint);
   } else {
     popup_->OnResultChanged();
   }
 
+  // TODO(beaudoin): This may no longer be needed now that instant classic is
+  // gone.
   if (popup_->IsOpen()) {
     // The popup size may have changed, let instant know.
     OnPopupBoundsChanged(popup_->view()->GetTargetBounds());
 
     InstantController* instant_controller = GetInstantController();
     if (instant_controller && !omnibox_edit_model_->in_revert()) {
       instant_controller->HandleAutocompleteResults(
           *autocomplete_controller_->providers(),
           autocomplete_controller_->result());
     }
@@ skipping 23 matching lines @@
   AutocompleteInput::RemoveForcedQueryStringIfNecessary(
       autocomplete_controller_->input().type(), &user_text);
   AutocompleteInput::RemoveForcedQueryStringIfNecessary(
       autocomplete_controller_->input().type(), &full_text);
   return instant_controller->Update(
       match, user_text, full_text, selection_start, selection_end,
       UseVerbatimInstant(just_deleted_text), user_input_in_progress,
       popup_->IsOpen(), in_escape_handler, keyword_is_selected);
 }
 
+void OmniboxController::FinalizeInstantQuery(
+    const string16& input_text,
+    const InstantSuggestion& suggestion) {
+  if (!popup_model()->result().empty()) {
+    // When a IME is active and a candidate window is open, we don't show
+    // the omnibox popup, though |result()| may be available.  Thus we check
+    // whether result().empty() or not instead of whether IsOpen() or not.
+    // We need the finalization of instant query when result() is available.

[Peter Kasting, 2013/06/11 22:42:55]

Nit: This last sentence isn't grammatical and I don't totally know what it
means.  (I am aware you're just moving this comment from elsewhere.)

[beaudoin, 2013/06/14 21:39:38]

Clarified, to the best of my understanding.

Done.

+    SearchProvider* search_provider =
+        autocomplete_controller_->search_provider();
+    // There may be no providers during testing; guard against that.
+    if (search_provider)
+      search_provider->FinalizeInstantQuery(input_text, suggestion);
+  }
+}
+
+void OmniboxController::SetInstantSuggestion(
+    const InstantSuggestion& suggestion) {
+  switch (suggestion.behavior) {
+

[Peter Kasting, 2013/06/11 22:42:55]

Nit: Extra newline

[beaudoin, 2013/06/14 21:39:38]

Done.

+    case INSTANT_COMPLETE_NOW:
+      // Set blue suggestion text.
+      // TODO(beaudoin): This currently go to the SearchProvider. Instead we

[Peter Kasting, 2013/06/11 22:42:55]

Nit: go -> goes

[beaudoin, 2013/06/14 21:39:38]

Done.

+      // should just create a valid current_match_ and call
+      // omnibox_edit_model_->OnCurrentMatchChanged. This way we can get rid of
+      // FinalizeInstantQuery entirely.
+      if (!suggestion.text.empty())
+        FinalizeInstantQuery(omnibox_edit_model_->GetViewText(), suggestion);
+      break;

[Peter Kasting, 2013/06/11 22:42:55]

Nit: Prefer return to break when they'd do the same thing

[beaudoin, 2013/06/14 21:39:38]

Done.

+
+    case INSTANT_COMPLETE_NEVER: {
+      DCHECK_EQ(INSTANT_SUGGESTION_SEARCH, suggestion.type);
+
+      // Set gray suggestion text.
+      // Remove "?" if we're in forced query mode.
+      string16 view_text = omnibox_edit_model_->GetViewText();
+      AutocompleteInput::RemoveForcedQueryStringIfNecessary(
+          autocomplete_controller_->input().type(), &view_text);

[beaudoin, 2013/06/06 14:15:00]

Sreeram: I'm using autocomplete_controller->input() above, but I'm not sure it's
always guaranteed to be valid since StartAutocomplete is called independently of
DoInstant. Think it's kosher?

[sreeram, 2013/06/06 23:32:23]

No, not kosher. The gray text is set asynchronously, so
autocomplete_controller_->input() may be invalid, say because the user unfocused
the omnibox and thus StopAutocomplete() was called. You'll have to find another
way.

Thanks for catching this yourself! I likely wouldn't have!

[beaudoin, 2013/06/14 21:39:38]

Done.

+      CreateAndSetInstantMatch(view_text, view_text,
+                               AutocompleteMatchType::SEARCH_SUGGEST);
+      current_match_.gray_suggestion = suggestion.text;
+
+      // TODO(beaudoin): The following should no longer be needed.
+      SearchProvider* search_provider =
+          autocomplete_controller_->search_provider();
+      if (search_provider)
+        search_provider->ClearInstantSuggestion();
+
+      omnibox_edit_model_->OnCurrentMatchChanged(false);
+      break;
+    }
+
+    case INSTANT_COMPLETE_REPLACE:
+      // Replace the entire omnibox text by the suggestion the user just arrowed
+      // into.

[Peter Kasting, 2013/06/11 22:42:55]

Nit: into -> to

[beaudoin, 2013/06/14 21:39:38]

Done.

+      CreateAndSetInstantMatch(suggestion.text, suggestion.text,
+                               suggestion.type == INSTANT_SUGGESTION_SEARCH ?
+                                   AutocompleteMatchType::SEARCH_SUGGEST :
+                                   AutocompleteMatchType::URL_WHAT_YOU_TYPED);
+
+      omnibox_edit_model_->OnCurrentMatchChanged(true);
+      break;
+  }
+}
+
+void OmniboxController::InvalidateCurrentMatch() {
+  current_match_ = AutocompleteMatch();
+}
+
 void OmniboxController::ClearPopupKeywordMode() const {
   if (popup_->IsOpen() &&
       popup_->selected_line_state() == OmniboxPopupModel::KEYWORD)
     popup_->SetSelectedLineState(OmniboxPopupModel::NORMAL);
 }
 
 void OmniboxController::DoPreconnect(const AutocompleteMatch& match) {
   if (!match.destination_url.SchemeIs(extensions::kExtensionScheme)) {
     // Warm up DNS Prefetch cache, or preconnect to a search service.
     UMA_HISTOGRAM_ENUMERATION("Autocomplete.MatchType", match.type,
@@ skipping 31 matching lines @@
   // We send the caret position to Instant (so it can determine #1 itself), and
   // we use a separated widget for displaying the Instant suggest (so it doesn't
   // interfere with #2). So, we don't need to care about the value of
   // input.prevent_inline_autocomplete() here.
   return just_deleted_text || popup_->selected_line() != 0;
 }
 
 InstantController* OmniboxController::GetInstantController() const {
   return omnibox_edit_model_->GetInstantController();
 }
+
+void OmniboxController::CreateAndSetInstantMatch(
+    string16 query_string,
+    string16 input_text,
+    AutocompleteMatchType::Type match_type) {
+  string16 keyword = GetDefaultSearchProviderKeyword(profile_);
+  if (keyword.empty())
+    return;  // CreateSearchSuggestion needs a keyword.
+
+  current_match_ = SearchProvider::CreateSearchSuggestion(
+      profile_,
+      NULL,                 // autocomplete_provider

[Peter Kasting, 2013/06/11 22:42:55]

Nit: I don't really like commenting particular args like this... I kind of feel
like, if the reader wants to know, they can just go look at the function
declaration.

Plus it results in super-long code here because every arg has to have its own
line.

I'd probably just condense this to a couple of lines.

[beaudoin, 2013/06/14 21:39:38]

Done.

+      AutocompleteInput(),
+      query_string,
+      input_text,
+      0,                    // relevance
+      match_type,
+      0,                    // accepted_suggestion
+      false,                // is_keyword
+      keyword);
+}