Add a Physical Web omnibox autocomplete provider

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 "base/memory/ptr_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "components/omnibox/browser/autocomplete_provider_client.h"
+#include "components/omnibox/browser/autocomplete_provider_listener.h"
+#include "components/omnibox/browser/physical_web_provider.h"
+#include "components/physical_web/data_source/physical_web_data_source.h"
+#include "components/url_formatter/url_formatter.h"
+#include "grit/components_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "url/gurl.h"
+
+namespace {
+
+// The maximum number of match results to provide. If the number of nearby URLs
+// exceeds this limit, an overflow item is created. Tapping the overflow item
+// navigates to a page with the full list of nearby URLs. The overflow item is
+// counted as a match result for the purposes of the match limit.
+//
+// ex: With kPhysicalWebMaxMatches == 1, there should be at most one suggestion
+// created by this provider. If there is a single nearby URL, then the
+// suggestion will be for that URL. If there are multiple nearby URLs, the
+// suggestion will be the overflow item which navigates to the WebUI when
+// tapped.
+static const size_t kPhysicalWebMaxMatches = 1;
+
+// Relevance score of a Physical Web URL autocomplete match. This score is
+// intended to be between ClipboardURLProvider and ZeroSuggestProvider.
+static const int kPhysicalWebUrlRelevance = 700;
+
+// Relevance score of the overflow item.
+static const int kOverflowItemRelevance = kPhysicalWebUrlRelevance - 1;
+
+}
+
+// static
+PhysicalWebProvider* PhysicalWebProvider::Create(
+    AutocompleteProviderClient* client) {
+  return new PhysicalWebProvider(client);
+}
+
+void PhysicalWebProvider::Start(const AutocompleteInput& input,
+                                bool minimal_changes) {
+  DCHECK(kPhysicalWebMaxMatches < kMaxMatches);
+
+  Stop(false, false);
+
+  done_ = false;
+  matches_.clear();
+
+  PhysicalWebDataSource* data_source = client_->GetPhysicalWebDataSource();
+  if (!data_source) {
+    done_ = true;
+    return;
+  }
+
+  ConstructMatches(data_source->GetMetadata().get());
+
+  done_ = true;
+}
+
+void PhysicalWebProvider::Stop(bool clear_cached_results,
+                               bool due_to_user_inactivity) {
+  done_ = true;
+}
+
+PhysicalWebProvider::PhysicalWebProvider(AutocompleteProviderClient* client)
+    : AutocompleteProvider(AutocompleteProvider::TYPE_PHYSICAL_WEB),
+      client_(client) {
+}
+
+PhysicalWebProvider::~PhysicalWebProvider() {
+}
+
+void PhysicalWebProvider::ConstructMatches(base::ListValue* metadata_list) {
+  const size_t metadata_count = metadata_list->GetSize();
+
+  // Leave room for an overflow item.
+  const bool needs_overflow = metadata_count > kPhysicalWebMaxMatches;

[Mark P, 2016/08/11 23:16:03]

This needs overflow is no longer correct, as you may not add something if
there's a malformed entry in metadata.  Just do a direct size comparison at the
time you need.  This also means max_rows could be calculated incorrectly.

[mattreynolds, 2016/08/16 20:35:25]

Fixed so that we only count slots as used once we've successfully created an
AutocompleteMatch and added it to matches_. There's a small chance we'll create
the overflow item unnecessarily. (eg: suppose we have 1 slot and 2 metadata
items, the second item could be invalid since we never validate it)

[Mark P, 2016/08/16 22:49:08]

Acknowledged.  This seems like small enough of a deal that it isn't worth
fixing.

+  const size_t max_rows = needs_overflow ? kPhysicalWebMaxMatches - 1 :
+      metadata_count;
+
+  for (size_t i = 0; i < metadata_count && matches_.size() < max_rows; ++i) {

[Mark P, 2016/08/11 23:16:03]

nit: parens around the comparison operators
(but this comparison may be revised given the above comment)

[mattreynolds, 2016/08/16 20:35:25]

Acknowledged.

+    base::DictionaryValue* metadata_item = NULL;
+    if (!metadata_list->GetDictionary(i, &metadata_item)) {
+      continue;
+    }
+
+    std::string url_string;
+    std::string title_string;
+    if (!metadata_item->GetString("resolvedUrl", &url_string) ||
+        !metadata_item->GetString("title", &title_string)) {
+      continue;
+    }
+
+    GURL url(url_string);
+    base::string16 title = base::UTF8ToUTF16(title_string);
+
+    AutocompleteMatch match(this, kPhysicalWebUrlRelevance, false,
+        AutocompleteMatchType::PHYSICAL_WEB);
+    match.destination_url = url;
+
+    // Physical Web results should omit http:// (but not https://) and never
+    // appear bold.
+    match.contents = url_formatter::FormatUrl(url,
+        url_formatter::kFormatUrlOmitHTTP, net::UnescapeRule::SPACES,
+        nullptr, nullptr, nullptr);
+    match.fill_into_edit +=

[Mark P, 2016/08/11 23:16:03]

Why +=?

[mattreynolds, 2016/08/16 20:35:25]

Not sure, I borrowed this code from ZeroSuggestProvider::NavigationToMatch. It
doesn't appear to have any purpose there either, so I switched to just =

+        AutocompleteInput::FormattedStringWithEquivalentMeaning(
+            url, match.contents, client_->GetSchemeClassifier());
+
+    AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,

[Mark P, 2016/08/11 23:16:03]

This more naturally immediately follows the initialization of match.contents.

[Mark P, 2016/08/11 23:16:03]

I think the first three parameters should actually be
0, match.contents.length(), match.contents.length()
not what you have.

What do you think? (and where did you get this from, if it came from somewhere
else?)

ditto analogous comment for the two other calls below

[mattreynolds, 2016/08/16 20:35:25]

This was also copied from ZeroSuggestProvider::NavigationToMatch. We're trying
to mark the entire string as "no match". ClassifyLocationInString has an
early-exit case with match_location=base::string16::npos that will avoid
classifying any portion of the string with ACMatchClassification::MATCH.
However, if we pass match_location=0 then it will create a classification with
the MATCH bit set.

[Mark P, 2016/08/16 22:49:08]

Well, that's a poorly commented behavior of ClassifyLocationInString(), but so
be it.  Leaving it as-is is fine, though if you want to do the more direct
thing:
match.contents_class.push_back(ACMatchClassification(0,
ACMatchClassification::URL));
rather than the current long-winded call, that would be good too.

+        match.contents.length(), ACMatchClassification::URL,
+        &match.contents_class);
+
+    match.description = AutocompleteMatch::SanitizeString(title);
+    AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,
+        match.description.length(), ACMatchClassification::NONE,
+        &match.description_class);
+
+    match.allowed_to_be_default_match = matches_.empty();
+
+    matches_.push_back(match);

[Mark P, 2016/08/11 23:16:03]

I notice you give all results the same relevance score.  This means that can
change their order in the final sort phase.  Does the physical web data come in
a particular order (e.g., quality of signal)?  If so, you may want to use that
order and give all the results decreasing relevance scores.

Obviously, the overflow item should have a relevance score just below your
lowest-scoring item.

[mattreynolds, 2016/08/16 20:35:25]

Yes, we would like to preserve the order from the data source. I've adjusted the
relevance scores so they decrease from the base value.

+  }
+
+  if (needs_overflow) {
+    AppendOverflowItem(metadata_count - matches_.size());
+  }
+}
+
+void PhysicalWebProvider::AppendOverflowItem(int additional_url_count) {
+  AutocompleteMatch match(this, kOverflowItemRelevance, false,
+      AutocompleteMatchType::PHYSICAL_WEB);
+  match.destination_url = GURL("chrome://physical-web");
+  match.description = l10n_util::GetPluralStringFUTF16(
+      IDS_PHYSICAL_WEB_OVERFLOW, additional_url_count);
+  AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,
+      match.description.length(), ACMatchClassification::NONE,
+      &match.description_class);
+
+  match.allowed_to_be_default_match = matches_.empty();
+
+  matches_.push_back(match);
+}