Show PhysicalWebProvider suggestions with omnibox input

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 fc65e2e4464f0b0669f89351ffc27dfa25b90593..354b4af98d34c9928909fd19eae7310518f9c0ba 100644
--- a/components/omnibox/browser/physical_web_provider.cc
+++ b/components/omnibox/browser/physical_web_provider.cc
@@ -2,14 +2,21 @@
 // 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"
 #include "base/values.h"
+#include "components/bookmarks/browser/titled_url_index.h"
+#include "components/bookmarks/browser/titled_url_node_sorter.h"
 #include "components/omnibox/browser/autocomplete_provider_client.h"
 #include "components/omnibox/browser/autocomplete_provider_listener.h"
+#include "components/omnibox/browser/autocomplete_provider_utils.h"
 #include "components/omnibox/browser/history_url_provider.h"
-#include "components/omnibox/browser/physical_web_provider.h"
+#include "components/omnibox/browser/physical_web_node.h"
+#include "components/omnibox/browser/url_prefix.h"
 #include "components/omnibox/browser/verbatim_match.h"
 #include "components/physical_web/data_source/physical_web_data_source.h"
 #include "components/url_formatter/url_formatter.h"
@@ -31,6 +38,9 @@ static const int kPhysicalWebUrlBaseRelevance = 700;
 // Physical Web match item (non-overflow item) we allow the omnibox display to
 // truncate the title instead.
 static const size_t kMaxTitleLengthInOverflow = 15;
+
+// The maximum number of Physical Web URLs to retrieve from the index.
+static const size_t kPhysicalWebIndexMaxMatches = 50;
 }

[Mark P, 2016/12/21 18:57:50]

style nit (I think): blank line before closing namespace brace

[mattreynolds, 2016/12/21 22:25:58]

Done.

 
 // static
@@ -52,12 +62,6 @@ void PhysicalWebProvider::Start(const AutocompleteInput& input,
   done_ = false;
   matches_.clear();
 
-  // Stop providing suggestions when the user enters text into the omnibox.
-  if (!input.from_omnibox_focus()) {
-    done_ = true;
-    return;
-  }
-
   // Don't provide suggestions in incognito mode.
   if (client_->IsOffTheRecord()) {
     done_ = true;
@@ -73,16 +77,21 @@ void PhysicalWebProvider::Start(const AutocompleteInput& input,
     return;
   }
 
-  ConstructMatches(data_source->GetMetadata().get());
-
-  // 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));
+  if (input.from_omnibox_focus()) {
+    ConstructZeroSuggestMatches(data_source->GetMetadata());
+
+    // 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->GetMetadata(), input);
   }
 
   done_ = true;
@@ -116,7 +125,8 @@ PhysicalWebProvider::PhysicalWebProvider(
 PhysicalWebProvider::~PhysicalWebProvider() {
 }
 
-void PhysicalWebProvider::ConstructMatches(base::ListValue* metadata_list) {
+void PhysicalWebProvider::ConstructZeroSuggestMatches(
+    std::unique_ptr<base::ListValue> metadata_list) {
   nearby_url_count_ = metadata_list->GetSize();
   size_t used_slots = 0;
 
@@ -178,6 +188,40 @@ void PhysicalWebProvider::ConstructMatches(base::ListValue* metadata_list) {
       "Omnibox.PhysicalWebProviderMatches", matches_.size(), kMaxMatches);
 }
 
+void PhysicalWebProvider::ConstructQuerySuggestMatches(
+    std::unique_ptr<base::ListValue> metadata_list,
+    const AutocompleteInput& input) {
+  bookmarks::TitledUrlIndex index(NULL);

[Mark P, 2016/12/21 18:57:50]

Please either add a comment (in this changelist or another) to the constructor
of TitledUrlIndex that NULL is valid.  And generally explain what the passed
parameter means.

If you don't want to do that there (I might not understand why), then do it
here.

Also, nit: NULL -> nullptr

[mattreynolds, 2016/12/21 22:25:58]

I'll add the comment here and comment the TitledUrlIndex constructor in a
separate CL.

[Mark P, 2016/12/21 23:21:40]

Acknowledged.

+  std::vector<std::unique_ptr<PhysicalWebNode> > nodes;
+  size_t metadata_count = metadata_list->GetSize();

[Mark P, 2016/12/21 18:57:50]

nit: const

[mattreynolds, 2016/12/21 22:25:58]

Done.

+  for (size_t i = 0; i < metadata_count; ++i) {
+    base::DictionaryValue* metadata_item = NULL;
+    if (metadata_list->GetDictionary(i, &metadata_item)) {
+      nodes.push_back(base::MakeUnique<PhysicalWebNode>(*metadata_item));

[Mark P, 2016/12/21 18:57:50]

This seems roundabout to me.  Why not call index.Add() directly?  Why do you
need the nodes vector?

[mattreynolds, 2016/12/21 22:25:58]

TitledUrlIndex doesn't take ownership of the PhysicalWebNodes we're creating. By
storing them in a vector of unique_ptrs we can ensure they are properly cleaned
up once the vector falls out of scope.

[Mark P, 2016/12/21 23:21:40]

Acknowledged.

+      index.Add(nodes.back().get());
+    }
+  }
+
+  std::vector<bookmarks::TitledUrlMatch> titled_url_matches;
+  index.GetResultsMatching(input.text(), kPhysicalWebIndexMaxMatches,
+                           query_parser::MatchingAlgorithm::DEFAULT,
+                           &titled_url_matches);
+
+  size_t used_slots = 0;
+  const base::string16 fixed_up_input(FixupUserInput(input).second);
+  for (auto titled_url_match : titled_url_matches) {
+    const int relevance = kPhysicalWebUrlBaseRelevance - used_slots;
+    matches_.push_back(
+        TitledUrlMatchToAutocompleteMatch(
+            this, client_->GetSchemeClassifier(), input, fixed_up_input,
+            titled_url_match, AutocompleteMatchType::PHYSICAL_WEB, relevance));
+    ++used_slots;
+    if (matches_.size() >= kPhysicalWebMaxMatches) {
+      break;
+    }
+  }
+}
+
 void PhysicalWebProvider::AppendOverflowItem(int additional_url_count,
                                              int relevance,
                                              const base::string16& title) {