Omnibox refactor. OmniboxController now holds an AutocompleteMatch.

chrome/browser/ui/omnibox/omnibox_controller.cc

Index: chrome/browser/ui/omnibox/omnibox_controller.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_controller.cc b/chrome/browser/ui/omnibox/omnibox_controller.cc
index 8d04055b2a2602f4e99b15e1d20858d3a69193fc..12617783caa93c93f8ff06bc393dbd234d69b384 100644
--- a/chrome/browser/ui/omnibox/omnibox_controller.cc
+++ b/chrome/browser/ui/omnibox/omnibox_controller.cc
@@ -15,6 +15,8 @@
 #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"
@@ -25,6 +27,21 @@
 
 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)
@@ -76,36 +93,40 @@ void OmniboxController::OnResultChanged(bool default_match_changed) {
   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());
@@ -149,6 +170,75 @@ bool OmniboxController::DoInstant(const AutocompleteMatch& match,
       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)
@@ -200,3 +290,24 @@ bool OmniboxController::UseVerbatimInstant(bool just_deleted_text) const {
 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);
+}