Ensure nearby URL count metric is properly initialized

components/omnibox/browser/physical_web_provider.cc

Index: components/omnibox/browser/physical_web_provider.cc
diff --git a/components/omnibox/browser/physical_web_provider.cc b/components/omnibox/browser/physical_web_provider.cc
index 15ba262ab3d8c5e8acbdcc15a9a85de3dc69ab47..92e164bfed00db2575fc84eb87ddf024a7979ec5 100644
--- a/components/omnibox/browser/physical_web_provider.cc
+++ b/components/omnibox/browser/physical_web_provider.cc
@@ -56,14 +56,18 @@ void PhysicalWebProvider::Start(const AutocompleteInput& input,
   done_ = false;
   matches_.clear();
 
+  const bool input_from_focus = input.from_omnibox_focus();

[Mark P, 2017/02/23 00:38:59]

optional nit: I don't think this alias buys you much.

[mattreynolds, 2017/02/23 02:03:06]

Removed

+
   had_physical_web_suggestions_ = false;
-  if (input.from_omnibox_focus())
-    had_physical_web_suggestions_at_focus_or_later_ = false;
+  if (input_from_focus) {

[Mark P, 2017/02/23 00:38:59]

nit: no need for { } here

[mattreynolds, 2017/02/23 02:03:06]

Removed

+    BeginOmniboxSession();
+  }
 
   // Don't provide suggestions in incognito mode.

[Mark P, 2017/02/23 00:38:59]

nit: combine 66-72 with 74-81, as the result is the same.

[mattreynolds, 2017/02/23 02:03:06]

Done.

   if (client_->IsOffTheRecord()) {
     done_ = true;
     nearby_url_count_ = 0;
+    nearby_url_count_at_focus_ = 0;

[Mark P, 2017/02/23 00:38:59]

Should this
had_physical_web_suggestions_at_focus_or_later_ = false
be in this block too?

[mattreynolds, 2017/02/23 02:03:06]

Yep, added

     return;
   }
 
@@ -72,14 +76,20 @@ void PhysicalWebProvider::Start(const AutocompleteInput& input,
   if (!data_source) {
     done_ = true;
     nearby_url_count_ = 0;
+    nearby_url_count_at_focus_ = 0;
     return;
   }
 
-  const bool input_from_focus = input.from_omnibox_focus();
+  auto metadata_list = data_source->GetMetadataList();
+  nearby_url_count_ = metadata_list->size();
+  if (input_from_focus) {

[Mark P, 2017/02/23 00:38:59]

nit: {} unnecessary

[mattreynolds, 2017/02/23 02:03:06]

Removed

+    nearby_url_count_at_focus_ = nearby_url_count_;
+  }
+
   const bool empty_input_from_user = !input_from_focus && input.text().empty();
 
   if (input_from_focus || empty_input_from_user) {
-    ConstructZeroSuggestMatches(data_source->GetMetadataList());
+    ConstructZeroSuggestMatches(std::move(metadata_list));
 
     if (!matches_.empty()) {
       had_physical_web_suggestions_ = true;
@@ -105,7 +115,7 @@ void PhysicalWebProvider::Start(const AutocompleteInput& input,
           client_, input, input.current_url(), history_url_provider_, -1));
     }
   } else {
-    ConstructQuerySuggestMatches(data_source->GetMetadataList(), input);
+    ConstructQuerySuggestMatches(std::move(metadata_list), input);
 
     if (!matches_.empty()) {
       had_physical_web_suggestions_ = true;
@@ -148,8 +158,30 @@ void PhysicalWebProvider::AddProviderInfo(ProvidersInfo* provider_info) const {
 
   // When the user accepts an autocomplete suggestion, record the number of
   // nearby Physical Web URLs at the time the provider last constructed matches.
-  UMA_HISTOGRAM_EXACT_LINEAR("Omnibox.SuggestionUsed.NearbyURLCount",
-                             nearby_url_count_, 50);
+  UMA_HISTOGRAM_EXACT_LINEAR(
+      "Omnibox.SuggestionUsed.NearbyURLCount.AtMatchCreation",
+      nearby_url_count_, 50);
+
+  // On Android, it is somehow possible to accept an omnibox suggestion without
+  // first focusing the omnibox. In this situation, the nearby URL count at
+  // focus will be invalid because the omnibox was never focused. In an earlier

[Mark P, 2017/02/23 00:38:59]

omit the sentence "In an earlier version" and revise the following sentence
(perhaps "Now we guard" -> "We guard").  No one reading the code needs to know
the history.

[mattreynolds, 2017/02/23 02:03:07]

Done.

+  // version, this caused invalid data to be recorded by the histogram. Now we
+  // guard against recording the invalid data and instead record that we hit
+  // this corner case.
+  // TODO(crbug.com/691059): Remove once the focus-less suggestion mystery is
+  // solved.
+  bool suggestion_used_without_focus =

[Mark P, 2017/02/23 00:38:59]

nit: const

[mattreynolds, 2017/02/23 02:03:06]

Done.

+      (nearby_url_count_at_focus_ == string::npos);
+  UMA_HISTOGRAM_BOOLEAN(
+      "Omnibox.PhysicalWebProvider.SuggestionUsedWithoutOmniboxFocus",
+      suggestion_used_without_focus);
+
+  if (!suggestion_used_without_focus) {
+    // When the user accepts an autocomplete suggestion, record the number of
+    // nearby Physical Web URLs at the time the omnibox input was last focused.
+    UMA_HISTOGRAM_EXACT_LINEAR("Omnibox.SuggestionUsed.NearbyURLCount.AtFocus",
+                               nearby_url_count_at_focus_, 50);
+  }
 }
 
 PhysicalWebProvider::PhysicalWebProvider(
@@ -158,6 +190,8 @@ PhysicalWebProvider::PhysicalWebProvider(
     : AutocompleteProvider(AutocompleteProvider::TYPE_PHYSICAL_WEB),
       client_(client),
       history_url_provider_(history_url_provider),
+      nearby_url_count_(string::npos),
+      nearby_url_count_at_focus_(string::npos),
       zero_suggest_enabled_(
           OmniboxFieldTrial::InPhysicalWebZeroSuggestFieldTrial()),
       after_typing_enabled_(
@@ -165,17 +199,26 @@ PhysicalWebProvider::PhysicalWebProvider(
       zero_suggest_base_relevance_(
           OmniboxFieldTrial::GetPhysicalWebZeroSuggestBaseRelevance()),
       after_typing_base_relevance_(
-          OmniboxFieldTrial::GetPhysicalWebAfterTypingBaseRelevance()) {}
+          OmniboxFieldTrial::GetPhysicalWebAfterTypingBaseRelevance()),
+      had_physical_web_suggestions_(false),
+      had_physical_web_suggestions_at_focus_or_later_(false) {}
 
 PhysicalWebProvider::~PhysicalWebProvider() {
 }
 
+void PhysicalWebProvider::BeginOmniboxSession() {
+  // Some metrics are tracked per-session and must be reset each time a new
+  // session begins. Per-session metrics are recorded in AddProviderInfo.

[Mark P, 2017/02/23 00:38:59]

Add something like
// Both these variables are set properly later within the Start() flow.

[mattreynolds, 2017/02/23 02:03:06]

Done.

+  nearby_url_count_at_focus_ = string::npos;
+  had_physical_web_suggestions_at_focus_or_later_ = false;
+}
+
 void PhysicalWebProvider::ConstructZeroSuggestMatches(
     std::unique_ptr<physical_web::MetadataList> metadata_list) {
-  nearby_url_count_ = metadata_list->size();
+  size_t nearby_url_count = metadata_list->size();
   size_t used_slots = 0;
 
-  for (size_t i = 0; i < nearby_url_count_; ++i) {
+  for (size_t i = 0; i < nearby_url_count; ++i) {
     const auto& metadata_item = (*metadata_list)[i];
     std::string url_string = metadata_item.resolved_url.spec();
     std::string title_string = metadata_item.title;
@@ -189,7 +232,7 @@ void PhysicalWebProvider::ConstructZeroSuggestMatches(
     // Append an overflow item if creating a match for each metadata item would
     // exceed the match limit.
     const size_t remaining_slots = kPhysicalWebMaxMatches - used_slots;
-    const size_t remaining_metadata = nearby_url_count_ - i;
+    const size_t remaining_metadata = nearby_url_count - i;
     if ((remaining_slots == 1) && (remaining_metadata > remaining_slots)) {
       AppendOverflowItem(remaining_metadata, relevance, title);
       break;