Omnibox - Warm Up PSuggest on Focus

components/omnibox/browser/search_provider.cc

Index: components/omnibox/browser/search_provider.cc
diff --git a/components/omnibox/browser/search_provider.cc b/components/omnibox/browser/search_provider.cc
index e01d3b350d4f4767b31b813692ff49f02eb262c0..96ee7dc79af189cca090affb57b7ecacb02278b2 100644
--- a/components/omnibox/browser/search_provider.cc
+++ b/components/omnibox/browser/search_provider.cc
@@ -12,6 +12,7 @@
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/feature_list.h"
 #include "base/i18n/break_iterator.h"
 #include "base/i18n/case_conversion.h"
 #include "base/json/json_string_value_serializer.h"
@@ -222,9 +223,11 @@ void SearchProvider::Start(const AutocompleteInput& input,
   matches_.clear();
   set_field_trial_triggered(false);
 
-  // Can't return search/suggest results for bogus input.
-  if (input.from_omnibox_focus() ||
-      input.type() == metrics::OmniboxInputType::INVALID) {
+  // Unless warming up the suggest server on focus, SearchProvider doesn't do
+  // do anything useful for on-focus inputs or empty inputs.  Exit early.
+  if (!base::FeatureList::IsEnabled(omnibox::kSearchProviderWarmUpOnFocus) &&
+      (input.from_omnibox_focus() ||
+       input.type() == metrics::OmniboxInputType::INVALID)) {
     Stop(true, false);
     return;
   }
@@ -267,7 +270,14 @@ void SearchProvider::Start(const AutocompleteInput& input,
 
   providers_.set(default_provider_keyword, keyword_provider_keyword);
 
-  if (input.text().empty()) {
+  if (input.from_omnibox_focus()) {
+    // Don't display any suggestions for on-focus requests.  Stop the requests
+    // here so that we can create a new request later in this flow (to warm-up
+    // the suggest server by alerting it that the user is likely about to start
+    // typing).

[Peter Kasting, 2017/03/01 03:00:35]

Can we have had requests in-flight before this?  Going out of focus should have
closed the dropdown, which should have stopped all providers, which should have
stopped all requests, no?

[Mark P, 2017/03/01 20:02:32]

Probably...?

But there are so many edge cases.  E.g., control-l to focus, dropdown does not
focus, click on web content, control-l to focus.  Do we cancel requests on loss
of focus even if the dropdown was not open?  Revised the comment; please the
StopSuggest() call.

[Peter Kasting, 2017/03/02 22:50:42]

Yes.  Otherwise, the dropdown could pop open later when the not-stopped requests
return results back.  So I think this is an invariant that we cannot have
requests in flight here.

I would aim to remove this and the accompanying comment bits.

[Mark P, 2017/03/03 22:12:24]

Removed it and removed comment.  Added DCHECK to verify your claim.

+    StopSuggest();
+    ClearAllResults();
+  } else if (input.text().empty()) {
     // User typed "?" alone.  Give them a placeholder result indicating what
     // this syntax does.
     if (default_provider) {
@@ -286,21 +296,24 @@ void SearchProvider::Start(const AutocompleteInput& input,
 
   input_ = input;
 
-  DoHistoryQuery(minimal_changes);
-  // Answers needs scored history results before any suggest query has been
-  // started, since the query for answer-bearing results needs additional
-  // prefetch information based on the highest-scored local history result.
-  ScoreHistoryResults(raw_default_history_results_,
-                      false,
-                      &transformed_default_history_results_);
-  ScoreHistoryResults(raw_keyword_history_results_,
-                      true,
-                      &transformed_keyword_history_results_);
-  prefetch_data_ = FindAnswersPrefetchData();
-
-  // Raw results are not needed any more.
-  raw_default_history_results_.clear();
-  raw_keyword_history_results_.clear();
+  // Don't search the query history database for on-focus inputs; this
+  // provider should not be opening the dropdown / giving results for on-focus
+  // inputs.

[Peter Kasting, 2017/03/01 03:00:35]

Nit: Maybe second clause should be "these inputs should only be used to warm up
the suggest server".

[Mark P, 2017/03/01 20:02:33]

Okay.  Done.

+  if (!input.from_omnibox_focus()) {
+    DoHistoryQuery(minimal_changes);
+    // Answers needs scored history results before any suggest query has been
+    // started, since the query for answer-bearing results needs additional
+    // prefetch information based on the highest-scored local history result.
+    ScoreHistoryResults(raw_default_history_results_, false,
+                        &transformed_default_history_results_);
+    ScoreHistoryResults(raw_keyword_history_results_, true,
+                        &transformed_keyword_history_results_);
+    prefetch_data_ = FindAnswersPrefetchData();
+
+    // Raw results are not needed any more.
+    raw_default_history_results_.clear();
+    raw_keyword_history_results_.clear();
+  }
 
   StartOrStopSuggestQuery(minimal_changes);
   UpdateMatches();
@@ -394,7 +407,8 @@ void SearchProvider::OnURLFetchComplete(const net::URLFetcher* source) {
   LogFetchComplete(request_succeeded, is_keyword);
 
   bool results_updated = false;
-  if (request_succeeded) {
+  // Ignore (i.e, don't display) any suggestions for on-focus inputs.

[Peter Kasting, 2017/03/01 03:00:35]

Nit: , -> .

Might want to say why we should ignore these.

[Mark P, 2017/03/01 20:02:33]

Did -> .,
Done, though I'm not sure how much light my new comment adds.

+  if (!input_.from_omnibox_focus() && request_succeeded) {
     std::unique_ptr<base::Value> data(
         SearchSuggestionParser::DeserializeJsonData(
             SearchSuggestionParser::ExtractJsonData(source)));
@@ -495,6 +509,14 @@ void SearchProvider::LogFetchComplete(bool success, bool is_keyword) {
 }
 
 void SearchProvider::UpdateMatches() {
+  // Because on-focus inputs display no suggestions, it's inappropriate to
+  // enforce inlineability and default match constraints (which is what the
+  // rest of this function does).
+  if (input_.from_omnibox_focus()) {
+    UpdateDone();

[Peter Kasting, 2017/03/01 03:00:35]

Nit: Maybe most of this function should be a helper so this could just be:

{
  // On-focus inputs display no suggestions, so we need not enforce
  // inlineability and default match constraints.  (BTW, does doing so actually
  // break stuff?  If so say what.  Maybe the thing we have to avoid is the
  // PersistTopSuggestions() calls?  Maybe those also make the name I choose
  // below incorrect?)
  if (!input_.from_omnibox_focus())
    EnforceConstraints();

  UpdateDone();
}

[Mark P, 2017/03/01 20:02:32]

Refactored to make this cleaner.

+    return;
+  }
+
   PersistTopSuggestions(&default_results_);
   PersistTopSuggestions(&keyword_results_);
   ConvertResultsToAutocompleteMatches();
@@ -738,6 +760,9 @@ bool SearchProvider::IsQueryPotentionallyPrivate() const {
   // If the input type might be a URL, we take extra care so that private data
   // isn't sent to the server.

[Peter Kasting, 2017/03/01 03:00:35]

Nit: While here: This comment seems like it belongs as part of a declaration
comment or something, not here.

And the comment below starts with "Next", which looks like there used to be code
here but now there isn't?  Would be nice to fix this stuff up.

[Mark P, 2017/03/01 20:02:33]

Done.  (Moved a slightly revised version of the comment to the header.)
Removed "Next".

 
+  if (input_.text().empty())
+    return false;
+
   // Next we check the scheme.  If this is UNKNOWN/URL with a scheme that isn't
   // http/https/ftp, we shouldn't send it.  Sending things like file: and data:
   // is both a waste of time and a disclosure of potentially private, local