Ensure nearby URL count metric is properly initialized

components/omnibox/browser/physical_web_provider.cc

 // Copyright (c) 2016 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/physical_web_provider.h"
 
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 40 matching lines @@
                                 bool minimal_changes) {
   DCHECK(kPhysicalWebMaxMatches < kMaxMatches);
 
   Stop(false, false);
 
   done_ = false;
   matches_.clear();
 
   had_physical_web_suggestions_ = false;
   if (input.from_omnibox_focus())
-    had_physical_web_suggestions_at_focus_or_later_ = false;
+    BeginOmniboxSession();
 
-  // Don't provide suggestions in incognito mode.
-  if (client_->IsOffTheRecord()) {
+  physical_web::PhysicalWebDataSource* data_source =
+      client_->GetPhysicalWebDataSource();
+
+  // Don't provide results in incognito mode or if the data source could not be
+  // created.
+  if (client_->IsOffTheRecord() || !data_source) {
     done_ = true;
     nearby_url_count_ = 0;
+    nearby_url_count_at_focus_ = 0;
+    had_physical_web_suggestions_at_focus_or_later_ = false;
     return;
   }
 
-  physical_web::PhysicalWebDataSource* data_source =
-      client_->GetPhysicalWebDataSource();
-  if (!data_source) {
-    done_ = true;
-    nearby_url_count_ = 0;
-    return;
-  }
+  auto metadata_list = data_source->GetMetadataList();
+  nearby_url_count_ = metadata_list->size();
+  if (input.from_omnibox_focus())
+    nearby_url_count_at_focus_ = nearby_url_count_;
 
-  const bool input_from_focus = input.from_omnibox_focus();
-  const bool empty_input_from_user = !input_from_focus && input.text().empty();
+  const bool empty_input_from_user =
+      !input.from_omnibox_focus() && input.text().empty();
 
-  if (input_from_focus || empty_input_from_user) {
-    ConstructZeroSuggestMatches(data_source->GetMetadataList());
+  if (input.from_omnibox_focus() || empty_input_from_user) {
+    ConstructZeroSuggestMatches(std::move(metadata_list));
 
     if (!matches_.empty()) {
       had_physical_web_suggestions_ = true;
       had_physical_web_suggestions_at_focus_or_later_ = true;
     }
 
     // Don't show zero-suggest suggestions unless the PhysicalWebZeroSuggest
     // omnibox experiment parameter is enabled. If the omnibox input is empty
     // because the user cleared it, also require that PhysicalWebAfterTyping is
     // enabled.
     if (!zero_suggest_enabled_ ||
         (empty_input_from_user && !after_typing_enabled_)) {
       matches_.clear();
     }
 
     // In zero-suggest, Physical Web matches should never be default. If the
     // omnibox input is non-empty and we have at least one Physical Web match,
     // add the current URL as the default so that hitting enter after focusing
     // the omnibox causes the current page to reload. If the input field is
     // empty, no default match is required.
     if (!matches_.empty() && !input.text().empty()) {
       matches_.push_back(VerbatimMatchForURL(
           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;
       had_physical_web_suggestions_at_focus_or_later_ = true;
     }
 
     // Don't show Physical Web suggestions after the user starts typing unless
     // the PhysicalWebAfterTyping omnibox experiment parameter is enabled.
     if (!after_typing_enabled_) {
       matches_.clear();
@@ skipping 22 matching lines @@
     if (had_physical_web_suggestions_)
       new_entry.mutable_field_trial_triggered()->Add(field_trial_hashes[i]);
     if (had_physical_web_suggestions_at_focus_or_later_) {
       new_entry.mutable_field_trial_triggered_in_session()->Add(
           field_trial_hashes[i]);
     }
   }
 
   // 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. 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.
+  const bool suggestion_used_without_focus =
+      (nearby_url_count_at_focus_ == std::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(
     AutocompleteProviderClient* client,
     HistoryURLProvider* history_url_provider)
     : AutocompleteProvider(AutocompleteProvider::TYPE_PHYSICAL_WEB),
       client_(client),
       history_url_provider_(history_url_provider),
+      nearby_url_count_(std::string::npos),
+      nearby_url_count_at_focus_(std::string::npos),
       zero_suggest_enabled_(
           OmniboxFieldTrial::InPhysicalWebZeroSuggestFieldTrial()),
       after_typing_enabled_(
           OmniboxFieldTrial::InPhysicalWebAfterTypingFieldTrial()),
       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.
+  // The values set here are intended to be overwritten in Start() when the user
+  // focuses the omnibox.

[Mark P, 2017/02/23 05:09:49]

Now that I read this code closer, I don't think this function
BeginOmniboxSession is necessary.  I think you can drop it entirely.

Start() looks like
  if (focus)
    set these variables  (via BeginOmniboxSession)
  if (special case) {
    set these variables
    return
  }
  if (focus)
    set these variables
  } else {
    ..
  }


There is no code path on which these variables don't get overridden later within
the same function call, implying that setting them here is not needed.

[mattreynolds, 2017/02/23 19:42:14]

Makes sense, removed.

+  nearby_url_count_at_focus_ = std::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;
     base::string16 title =
         AutocompleteMatch::SanitizeString(base::UTF8ToUTF16(title_string));
 
     // Add match items with decreasing relevance to preserve the ordering in
     // the metadata list.
     int relevance = zero_suggest_base_relevance_ - used_slots;
 
     // 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;
     }
 
     GURL url(url_string);
 
     AutocompleteMatch match(this, relevance, false,
         AutocompleteMatchType::PHYSICAL_WEB);
     match.destination_url = url;
@@ skipping 80 matching lines @@
       AutocompleteInput::FormattedStringWithEquivalentMeaning(
           url, match.contents, client_->GetSchemeClassifier());
 
   match.description =
       l10n_util::GetStringUTF16(IDS_PHYSICAL_WEB_OVERFLOW_DESCRIPTION);
   match.description_class.push_back(
       ACMatchClassification(0, ACMatchClassification::NONE));
 
   matches_.push_back(match);
 }