Omnibox - Zero Suggest - Log When/Whether Contextual Search is Possible

components/omnibox/browser/zero_suggest_provider.cc

Index: components/omnibox/browser/zero_suggest_provider.cc
diff --git a/components/omnibox/browser/zero_suggest_provider.cc b/components/omnibox/browser/zero_suggest_provider.cc
index 1a5d022917791cebdb92f6881df8209ef52197c7..eb6e0c42d4d0d8eececcb9037ab8c41bc97078cc 100644
--- a/components/omnibox/browser/zero_suggest_provider.cc
+++ b/components/omnibox/browser/zero_suggest_provider.cc
@@ -19,6 +19,7 @@
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/history/core/browser/top_sites.h"
+#include "components/metrics/proto/omnibox_event.pb.h"
 #include "components/metrics/proto/omnibox_input_type.pb.h"
 #include "components/omnibox/browser/autocomplete_classifier.h"
 #include "components/omnibox/browser/autocomplete_input.h"
@@ -43,19 +44,28 @@
 
 namespace {
 
+// Represents if, on focus, ZeroSuggestProvider is allowed to display

[Peter Kasting, 2017/03/02 21:31:34]

Nit: "Represents whether ZeroSuggestProvider is allowed to display contextual
suggestions on focus, and if not, why not."

[Mark P, 2017/03/02 23:17:00]

Done.

+// contextual suggestions and, if not, why not.
+// These values are written to logs.  New enum values can be added, but existing
+// enums must never be renumbered or deleted and reused.
+enum ZeroSuggestEligibleOnFocus {

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Maybe you should use an enum class, and then remove ZERO_SUGGEST_ from the
values below.

I might also call this ZeroSuggestEligibility (is "On focus" necessary in the
name to distinguish from another kind of zerosuggest eligibility?).

[Mark P, 2017/03/02 23:17:00]

Sure.  Done.
I thought so originally because I had the zero suggest on profile open
histogram.  Then I realized that was also a zero suggest on focus, just recorded
on profile open.  Took your shorter name.  Zero suggest only does this involving
focus.

+  ZERO_SUGGEST_ELIGIBLE = 0,
+  ZERO_SUGGEST_GENERALLY_ELIGIBLE_BUT_NOT_THIS_URL = 1,
+  ZERO_SUGGEST_GENERALLY_INELIGIBLE = 2,

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Maybe _URL_INELIGIBLE versus _GLOBALLY_INELIGIBLE?  I'm not a fan of
"generally" because it's vague (are there exceptions to the general rule?), and
_BUT_NOT_THIS_URL is so verbose.

[Mark P, 2017/03/02 23:17:00]

Revised the middle enum name and added a comment.
Yes, there are exceptions here.  For example, if the user has an incognito
window open, they're generally ineligible as long as it's open, but they become
eligible again when it's closed.  This is why I intentionally am using a term
like that as opposed to plain old ineligible.

Left the last enum name.

+  ZERO_SUGGEST_ELIGIBLE_MAX_VALUE
+};
+
 // TODO(hfung): The histogram code was copied and modified from
 // search_provider.cc.  Refactor and consolidate the code.
 // We keep track in a histogram how many suggest requests we send, how
 // many suggest requests we invalidate (e.g., due to a user typing
 // another character), and how many replies we receive.
-// *** ADD NEW ENUMS AFTER ALL PREVIOUSLY DEFINED ONES! ***
-//     (excluding the end-of-list enum value)
-// We do not want values of existing enums to change or else it screws
-// up the statistics.
+// These values are written to logs.  New enum values can be added, but existing
+// enums must never be renumbered or deleted and reused.
 enum ZeroSuggestRequestsHistogramValue {
   ZERO_SUGGEST_REQUEST_SENT = 1,
-  ZERO_SUGGEST_REQUEST_INVALIDATED,
-  ZERO_SUGGEST_REPLY_RECEIVED,
+  ZERO_SUGGEST_REQUEST_INVALIDATED = 2,
+  ZERO_SUGGEST_REPLY_RECEIVED = 3,
   ZERO_SUGGEST_MAX_REQUEST_HISTOGRAM_VALUE
 };
 
@@ -68,6 +78,9 @@ void LogOmniboxZeroSuggestRequest(
 // Relevance value to use if it was not set explicitly by the server.
 const int kDefaultZeroSuggestRelevance = 100;
 
+// Used for testing whether zero suggest is ever available.
+std::string kArbitraryInsecureUrlString = "http://www.google.com/";

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Would an intranet hostname like "test/" work?  It would be nice to use a
URL that's obviously not something we rely on having real meaning or being
accessible.  And bonus points for saving a couple bytes in the binary by being
shorter :)

[Mark P, 2017/03/02 23:17:00]

No.  I can easily imagine having a test (not sure if we do) that prevents
sending intranet URLs to a suggest server.
I'm open to another full domain name URL.  I think Google is appropriate at this
point, as the only zero suggest provider suggest server is Google (so it's even
more likely that a URL-level restriction test will say, yes, it's okay to send a
Google URL to a Google suggest server).

+
 }  // namespace
 
 // static
@@ -101,32 +114,36 @@ void ZeroSuggestProvider::Start(const AutocompleteInput& input,
   current_query_ = input.current_url().spec();
   current_page_classification_ = input.current_page_classification();
   current_url_match_ = MatchForCurrentURL();
-  TemplateURLService* template_url_service = client()->GetTemplateURLService();
-
-  const TemplateURL* default_provider =
-      template_url_service->GetDefaultSearchProvider();
-  if (default_provider == NULL)
-    return;
 
-  base::string16 prefix;
-  TemplateURLRef::SearchTermsArgs search_term_args(prefix);
-  std::string url_string;
-  if (OmniboxFieldTrial::InZeroSuggestRedirectToChromeFieldTrial()) {
-    url_string = OmniboxFieldTrial::ZeroSuggestRedirectToChromeServerAddress();
-  } else {
-    url_string = default_provider->suggestions_url_ref().ReplaceSearchTerms(
-        search_term_args, template_url_service->search_terms_data());
-  }
+  std::string url_string(GetContextualSuggestionsUrl());

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Prefer = to () for simple inits like this; see
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab...

[Mark P, 2017/03/02 23:17:00]

Done.

   GURL suggest_url(url_string);
-
   if (!suggest_url.is_valid())
     return;
 
   // No need to send the current page URL in personalized suggest or
   // most visited field trials.
-  if (CanSendURL(input.current_url(), suggest_url, default_provider,
+  const TemplateURLService* template_url_service =
+      client()->GetTemplateURLService();

[Peter Kasting, 2017/03/02 21:31:34]

I don't have specific recommendations here, but I notice this code and
GetContextualSuggestionsUrl() both have to fetch the TemplateURLService/default
search provider like this, and both this code and the constructor get the
contextual suggestions URL and call CanSendURL().

Think about whether there's any way to refactor into more helpers to avoid code
duplication or something.  There may not be.

[Mark P, 2017/03/02 23:17:00]

There isn't, because most of the parameters between the two CanSendURL calls
differ, partially intentionally (different page classification for example in
the constructor call), partially as a side effect of logging in the constructor
(cannot call overloaded functions).

I was a little bothered by the extra calls to GetTemplateURLService() and
GetDefaultSearchProvider() in the current flow in Start().  (They get called
both here and in GetContextualSuggestionsUrl().)  Yet, I tried to pass in those
variables to GetContextualSuggestionsUrl() just so we don't have to make those
calls again.  It looks ugly.

And I doubt it's any more efficient; these calls are fast, and the compiler
might be smart enough to optimize them down to one.

+  const TemplateURL* default_provider =
+      template_url_service->GetDefaultSearchProvider();
+  const bool can_send_current_url =
+      CanSendURL(input.current_url(), suggest_url, default_provider,
                  current_page_classification_,
-                 template_url_service->search_terms_data(), client()) &&
+                 template_url_service->search_terms_data(), client());
+  GURL arbitrary_insecure_url(kArbitraryInsecureUrlString);
+  const bool can_send_ordinary_url =
+      CanSendURL(arbitrary_insecure_url, suggest_url, default_provider,
+                 current_page_classification_,
+                 template_url_service->search_terms_data(), client());
+  const ZeroSuggestEligibleOnFocus eligibility =
+      can_send_current_url
+          ? ZERO_SUGGEST_ELIGIBLE
+          : (can_send_ordinary_url
+                 ? ZERO_SUGGEST_GENERALLY_ELIGIBLE_BUT_NOT_THIS_URL
+                 : ZERO_SUGGEST_GENERALLY_INELIGIBLE);

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Not a fan of nesting ?:.  How about something like this to avoid it:

  ZeroSuggestEligibleOnFocus eligibility = ZERO_SUGGEST_ELIGIBLE;
  if (!can_send_current_url) {
    eligibility = CanSendURL(arbitrary_insecure_url, ...)
                      ? ZERO_SUGGEST_GENERALLY_ELIGIBLE_BUT_NOT_THIS_URL
                      : ZERO_SUGGEST_GENERALLY_INELIGIBLE;
  }

This also avoids the second CanSendURL() call unless it's actually needed.

[Mark P, 2017/03/02 23:17:00]

Good idea.  Done.

+  UMA_HISTOGRAM_ENUMERATION("Omnibox.ZeroSuggest.Eligible.OnFocus", eligibility,
+                            ZERO_SUGGEST_ELIGIBLE_MAX_VALUE);
+  if (can_send_current_url &&
       !OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial() &&
       !OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial()) {
     // Update suggest_url to include the current_page_url.
@@ -136,6 +153,8 @@ void ZeroSuggestProvider::Start(const AutocompleteInput& input,
           OmniboxFieldTrial::ZeroSuggestRedirectToChromeAdditionalFields();
       suggest_url = GURL(url_string);
     } else {
+      base::string16 prefix;

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Can just pass string16() in the call below, no need for this temp

[Mark P, 2017/03/02 23:17:00]

I tried getting rid of it when I wrote the code, then looked at the code and
thought, oh, why am I passing an empty string into SearchTermsArgs's
constructor.  Having this extra variable makes it clear.

I thought there was a tip-of-the-week episode about this recently, but I can't
find it.

+      TemplateURLRef::SearchTermsArgs search_term_args(prefix);
       search_term_args.current_page_url = current_query_;
       suggest_url =
           GURL(default_provider->suggestions_url_ref().ReplaceSearchTerms(
@@ -209,6 +228,23 @@ ZeroSuggestProvider::ZeroSuggestProvider(
       results_from_cache_(false),
       waiting_for_most_visited_urls_request_(false),
       weak_ptr_factory_(this) {
+  // Record whether contextual zero suggest is possible for this user / profile.
+  // To check whether this is allowed, use an arbitrary insecure (http) URL

[Peter Kasting, 2017/03/02 21:31:34]

Nit: I'd move these last 3/4ths of the comment to just above the actual
histogram call below, since they're specific explanations of technical details
in that call, rather than a general description of what the block is doing like
the first line is.

[Mark P, 2017/03/02 23:17:00]

Good diagnosis of the situation.  Done.

+  // as the URL we'd want suggestions for.  The value of OTHER as the current
+  // page classification is to correspond with that URL.
+  const TemplateURLService* template_url_service =
+      client->GetTemplateURLService();
+  // Template URL service can be null in tests.
+  if (template_url_service != nullptr) {

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Optional, but I find it slightly more common to just do "if (x)" or "if
(!x)" than "if (x != nullptr)" or "if (x == nullptr)".

[Mark P, 2017/03/02 23:17:00]

Left as is.  (That may be true in general in the codebase, but the code in these
search provider files tends to explicitly check for nulls.)

+    GURL suggest_url(GetContextualSuggestionsUrl());
+    UMA_HISTOGRAM_BOOLEAN(
+        "Omnibox.ZeroSuggest.Eligible.OnProfileOpen",
+        suggest_url.is_valid() &&
+            CanSendURL(GURL(kArbitraryInsecureUrlString), suggest_url,
+                       template_url_service->GetDefaultSearchProvider(),
+                       metrics::OmniboxEventProto::OTHER,
+                       template_url_service->search_terms_data(), client));
+  }
 }
 
 ZeroSuggestProvider::~ZeroSuggestProvider() {
@@ -479,6 +515,25 @@ bool ZeroSuggestProvider::ShouldShowNonContextualZeroSuggest(
   return true;
 }
 
+std::string ZeroSuggestProvider::GetContextualSuggestionsUrl() const {
+  // Without a default search provider, refuse to do anything (even if the user
+  // in the redirect-to-chrome field trial).

[Peter Kasting, 2017/03/02 21:31:34]

Nit: in -> is in

[Mark P, 2017/03/02 23:17:00]

Done.

+  const TemplateURLService* template_url_service =
+      client()->GetTemplateURLService();
+  const TemplateURL* default_provider =
+      template_url_service->GetDefaultSearchProvider();
+  if (default_provider == nullptr)
+    return "";

[Peter Kasting, 2017/03/02 21:31:34]

Nit: "" -> std::string()

[Mark P, 2017/03/02 23:17:00]

Done.

+
+  if (OmniboxFieldTrial::InZeroSuggestRedirectToChromeFieldTrial()) {

[Peter Kasting, 2017/03/02 21:31:34]

Nit: No {}

[Mark P, 2017/03/02 23:17:00]

Done.

+    return OmniboxFieldTrial::ZeroSuggestRedirectToChromeServerAddress();
+  }
+  base::string16 prefix;

[Peter Kasting, 2017/03/02 21:31:34]

Nit: Can just pass string16() in the call below, no need for this temp

[Mark P, 2017/03/02 23:17:00]

See earlier comment.

+  TemplateURLRef::SearchTermsArgs search_term_args(prefix);
+  return default_provider->suggestions_url_ref().ReplaceSearchTerms(
+      search_term_args, template_url_service->search_terms_data());
+}
+
 void ZeroSuggestProvider::MaybeUseCachedSuggestions() {
   if (!OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial())
     return;