Omnibox - Warm Up PSuggest on Focus

components/omnibox/browser/search_provider.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 "components/omnibox/browser/search_provider.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <cmath>
 #include <utility>
 
 #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"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/rand_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/trace_event.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
@@ skipping 190 matching lines @@
   TRACE_EVENT0("omnibox", "SearchProvider::Start");
   // Do our best to load the model as early as possible.  This will reduce
   // odds of having the model not ready when really needed (a non-empty input).
   TemplateURLService* model = client()->GetTemplateURLService();
   DCHECK(model);
   model->Load();
 
   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;
   }
 
   keyword_input_ = input;
   const TemplateURL* keyword_provider =
       KeywordProvider::GetSubstitutingTemplateURLForInput(model,
                                                           &keyword_input_);
   if (keyword_provider == NULL)
     keyword_input_.Clear();
@@ skipping 22 matching lines @@
       keyword_provider->keyword() : base::string16());
   if (!minimal_changes ||
       !providers_.equal(default_provider_keyword, keyword_provider_keyword)) {
     // Cancel any in-flight suggest requests.
     if (!done_)
       Stop(false, false);
   }
 
   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) {
       AutocompleteMatch match;
       match.provider = this;
       match.contents.assign(l10n_util::GetStringUTF16(IDS_EMPTY_KEYWORD_VALUE));
       match.contents_class.push_back(
           ACMatchClassification(0, ACMatchClassification::NONE));
       match.keyword = providers_.default_provider();
       match.allowed_to_be_default_match = true;
       matches_.push_back(match);
     }
     Stop(true, false);
     return;
   }
 
   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();
+  // 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();
+    // Raw results are not needed any more.
+    raw_default_history_results_.clear();
+    raw_keyword_history_results_.clear();
+  }
 
   StartOrStopSuggestQuery(minimal_changes);
   UpdateMatches();
 }
 
 void SearchProvider::Stop(bool clear_cached_results,
                           bool due_to_user_inactivity) {
   StopSuggest();
   done_ = true;
 
@@ skipping 73 matching lines @@
 
   // Ensure the request succeeded and that the provider used is still available.
   // A verbatim match cannot be generated without this provider, causing errors.
   const bool request_succeeded =
       source->GetStatus().is_success() && (source->GetResponseCode() == 200) &&
       GetTemplateURL(is_keyword);
 
   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)));
     if (data) {
       SearchSuggestionParser::Results* results =
           is_keyword ? &keyword_results_ : &default_results_;
       results_updated = ParseSuggestResults(*data, -1, is_keyword, results);
       if (results_updated)
         SortResults(is_keyword, results);
     }
@@ skipping 80 matching lines @@
       UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Success.GoogleResponseTime",
                           elapsed_time);
     } else {
       UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Failure.GoogleResponseTime",
                           elapsed_time);
     }
   }
 }
 
 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();
 
   // Check constraints that may be violated by suggested relevances.
   if (!matches_.empty() &&
       (default_results_.HasServerProvidedScores() ||
        keyword_results_.HasServerProvidedScores())) {
     // These blocks attempt to repair undesirable behavior by suggested
     // relevances with minimal impact, preserving other suggested relevances.
@@ skipping 221 matching lines @@
   const TemplateURL* default_url = providers_.GetDefaultProviderURL();
   const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
   return !client()->IsOffTheRecord() && client()->SearchSuggestEnabled() &&
          ((default_url && !default_url->suggestions_url().empty() &&
            !*query_is_private) ||
           (keyword_url && !keyword_url->suggestions_url().empty()));
 }
 
 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
   // data.  Other "schemes" may actually be usernames, and we don't want to send
   // passwords.  If the scheme is OK, we still need to check other cases below.
   // If this is QUERY, then the presence of these schemes means the user
   // explicitly typed one, and thus this is probably a URL that's being entered
   // and happens to currently be invalid -- in which case we again want to run
   // our checks below.  Other QUERY cases are less likely to be URLs and thus we
   // assume we're OK.
@@ skipping 739 matching lines @@
   for (MatchMap::const_iterator i(map.begin()); i != map.end(); ++i)
     matches.push_back(i->second);
   std::sort(matches.begin(), matches.end(), &AutocompleteMatch::MoreRelevant);
 
   // If there is a top scoring entry, find the corresponding answer.
   if (!matches.empty())
     return answers_cache_.GetTopAnswerEntry(matches[0].contents);
 
   return AnswersQueryData();
 }