Add a Physical Web omnibox autocomplete provider

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
new file mode 100644
index 0000000000000000000000000000000000000000..7195779ac77a7c0a7e84d28f87ef9db3ba52c6ba
--- /dev/null
+++ b/components/omnibox/browser/physical_web_provider.cc
@@ -0,0 +1,142 @@
+// 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 "url/gurl.h"
+
+namespace {
+
+// The maximum number of match results to provide, including the overflow item.
+static const int kPhysicalWebMaxMatches = 1;

[Mark P, 2016/08/08 20:41:37]

side comment: later you might want to put this under experimental control

[Mark P, 2016/08/08 20:41:38]

If this is set to 1, does that mean that if there's a physical web device
nearby, no URL is every displayed, only the overflow item?

[mattreynolds, 2016/08/09 21:42:02]

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.

Re: side comment, we do plan to create an experiment for this, but currently we
only have permission to add one item.

+
+// Relevance score of a Physical Web URL autocomplete match.

[Mark P, 2016/08/08 20:41:37]

Please comment, justifying why this is intentionally higher than the clipboard
suggestions.
Or maybe it lower, justifying why it's lower. :-)

[mattreynolds, 2016/08/09 21:42:03]

The current value was chosen arbitrarily, I think we should lower it below the
clipboard URL relevance.

ClipboardURLProvider says: "The relevance is 800 to beat ZeroSuggest results."

ZeroSuggestProvider sets its default relevance as 100 but the actual relevance
scores come from the server.  Do you know what their range is?

+static const int kPhysicalWebUrlRelevance = 1000;
+
+// Relevance score of the overflow item.

[Mark P, 2016/08/08 20:41:38]

Please explain what an "overflow" item is or point to an explanation.

[mattreynolds, 2016/08/09 21:42:02]

I added some explanation to the comment above kPhysicalWebMaxMatches

+static const int kOverflowItemRelevance = kPhysicalWebUrlRelevance - 1;
+
+// The path to the Physical Web WebUI page.
+const char kChromeUIPhysicalWebURL[] = "chrome://physical-web";
+
+}
+
+//static

[Mark P, 2016/08/08 20:41:37]

nit: space between "//" and "static"

[mattreynolds, 2016/08/09 21:42:03]

Done.

+PhysicalWebProvider* PhysicalWebProvider::Create(
+    AutocompleteProviderClient* client) {
+  return new PhysicalWebProvider(client);
+}
+
+void PhysicalWebProvider::Start(const AutocompleteInput& input,
+                                bool minimal_changes) {
+  Stop(false, false);
+
+  done_ = false;
+  matches_.clear();
+
+  PhysicalWebDataSource* data_source = client_->GetPhysicalWebDataSource();
+  std::unique_ptr<base::ListValue> metadata_list;
+  if (data_source) {
+    metadata_list = data_source->GetMetadata();
+  } else {
+    metadata_list = base::MakeUnique<base::ListValue>();
+  }

[Mark P, 2016/08/08 20:41:37]

Why don't you simply return early if you have no data?

[mattreynolds, 2016/08/09 21:42:03]

Done.

+
+  ConstructMatches(metadata_list.get());
+  AppendOverflowItemIfNecessary(metadata_list.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) {
+  int metadata_count = metadata_list->GetSize();

[Mark P, 2016/08/08 20:41:37]

general comment: most of your ints throughout should probably be size_ts.

[Mark P, 2016/08/08 20:41:37]

nit: const

[mattreynolds, 2016/08/09 21:42:03]

Done x2

+
+  // Leave room for an overflow item.
+  int max_rows = (metadata_count <= kPhysicalWebMaxMatches)
+      ? metadata_count : kPhysicalWebMaxMatches - 1;

[Mark P, 2016/08/08 20:41:37]

nit: operators go at end of lines, not beginning of following lines

[mattreynolds, 2016/08/09 21:42:03]

Done.

+
+  for (int i = 0; i < metadata_count && (int)matches_.size() < max_rows; ++i) {

[Mark P, 2016/08/08 20:41:37]

Don't all these Value getters have return values (e.g., success)?

[mattreynolds, 2016/08/09 21:42:03]

Added validation.  I think in general it's fine to not validate here since the
data comes from an internal data source that performs its own validation, but it
couldn't hurt either.

+    base::DictionaryValue* metadata_item = NULL;
+    metadata_list->GetDictionary(i, &metadata_item);
+
+    std::string url_string;
+    std::string title_string;
+    metadata_item->GetString("resolvedUrl", &url_string);
+    metadata_item->GetString("title", &title_string);
+
+    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 always omit protocols and never appear bold

[Mark P, 2016/08/08 20:41:37]

I am skeptical of this omitting protocols.  This philosophy differs from the
default omnibox where we always display secure protocols explicitly.  I don't
see why this situation is different.

[mattreynolds, 2016/08/09 21:42:03]

Changed to kFormatUrlOmitHttp.

+    match.contents = url_formatter::FormatUrl(url,
+        url_formatter::kFormatUrlOmitAll, net::UnescapeRule::SPACES, nullptr,
+        nullptr, nullptr);
+    match.fill_into_edit +=
+        AutocompleteInput::FormattedStringWithEquivalentMeaning(
+            url, match.contents, client_->GetSchemeClassifier());
+
+    AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,
+        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);
+  }
+}
+
+void PhysicalWebProvider::AppendOverflowItemIfNecessary(
+    base::ListValue* metadata_list) {
+  int previous_match_count = matches_.size();

[Mark P, 2016/08/08 20:41:37]

nit: const

[mattreynolds, 2016/08/09 21:42:03]

Done.

+  int additional_items = metadata_list->GetSize() - previous_match_count;
+  if (additional_items > 0) {
+    GURL url(kChromeUIPhysicalWebURL);

[Mark P, 2016/08/08 20:41:37]

only used in one place; no need to declare a variable for it.

[mattreynolds, 2016/08/09 21:42:02]

Done.

+    std::ostringstream contents_buf;
+    contents_buf << additional_items
+        << ((previous_match_count > 0) ? " more" : "")
+        << " web page" << ((additional_items == 1) ? "" : "s")
+        << " found";
+
+    AutocompleteMatch match(this, kOverflowItemRelevance, false,
+        AutocompleteMatchType::PHYSICAL_WEB);
+    match.destination_url = url;
+
+    match.description = base::UTF8ToUTF16(contents_buf.str().c_str());
+    AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,
+        match.description.length(), ACMatchClassification::NONE,
+        &match.description_class);
+
+    match.allowed_to_be_default_match = matches_.empty();

[Mark P, 2016/08/08 20:41:37]

I think the overflow item should never be allowed to be the default, and it
should even have a chance to be if the earlier code works right.

[mattreynolds, 2016/08/09 21:42:03]

In the case where we only have one "slot" to fill (kPhysicalWebMaxMatches == 1),
the overflow item will often be the only match item from this provider.  I think
it's appropriate for it to be the default match in that case.

[Mark P, 2016/08/10 23:33:19]

Acknowledged.

+
+    matches_.push_back(match);
+  }
+}