[omnibox] [bookmark-paths] Match and display paths in bookmark provider

Currently, bookmark suggestions are limited to title & URL matches. E.g
the input 'planets jupiter' won't suggest a bookmark titled 'jupiter' in
a directory 'planets'.

This is the 3rd of 3 CLs implementing bookmark-paths which will allow
inputs to match bookmark paths (without contributing to the bookmark
suggestion's score).

This CL updates the bookmark provider to:
1) Ask the underlying bookmark model for path matching when querying for
bookmarks.
2) Include the path in the suggestion contents or description depending
on feature params.

Bug: 1129524
Change-Id: I0b0d5f57a03e1e7a7a09ca5ab6f7b60ad161ece5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2441130
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824946}

components/omnibox/browser/bookmark_provider.cc

@@ -8,6 +8,7 @@
 #include <functional>
 #include <vector>
 
+#include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
@@ -19,6 +20,7 @@
 #include "components/omnibox/browser/autocomplete_result.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/titled_url_match_utils.h"
+#include "components/omnibox/common/omnibox_features.h"
 #include "components/prefs/pref_service.h"
 #include "components/query_parser/snippet.h"
 #include "components/search_engines/omnibox_focus_type.h"
@@ -81,7 +83,8 @@ void BookmarkProvider::DoAutocomplete(const AutocompleteInput& input) {
       input.text(), kMaxBookmarkMatches,
       OmniboxFieldTrial::IsShortBookmarkSuggestionsEnabled()
           ? query_parser::MatchingAlgorithm::ALWAYS_PREFIX_SEARCH
-          : query_parser::MatchingAlgorithm::DEFAULT);
+          : query_parser::MatchingAlgorithm::DEFAULT,
+      base::FeatureList::IsEnabled(omnibox::kBookmarkPaths));
   if (matches.empty())
     return;  // There were no matches.
   const base::string16 fixed_up_input(FixupUserInput(input).second);

components/omnibox/browser/titled_url_match_utils.cc

@@ -4,18 +4,38 @@
 
 #include "components/omnibox/browser/titled_url_match_utils.h"
 
+#include <numeric>
 #include <vector>
 
+#include "base/metrics/field_trial_params.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/bookmarks/browser/titled_url_node.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_match_classification.h"
 #include "components/omnibox/browser/history_provider.h"
-#include "components/omnibox/browser/in_memory_url_index_types.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/url_prefix.h"
+#include "components/omnibox/common/omnibox_features.h"
 #include "components/url_formatter/url_formatter.h"
 
+namespace {
+
+// Concatenates |ancestors| in reverse order and using '/' as the delimiter.
+base::string16 ConcatAncestorsTitles(
+    std::vector<base::StringPiece16> ancestors) {
+  return ancestors.empty()
+             ? base::string16()
+             : std::accumulate(std::next(ancestors.rbegin()), ancestors.rend(),
+                               base::string16(*ancestors.rbegin()),
+                               [](base::string16& a, base::StringPiece16& b) {
+                                 return a + base::UTF8ToUTF16("/") +
+                                        base::string16(b);
+                               });
+}
+
+}  // namespace
+
 namespace bookmarks {
 
 AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
@@ -26,15 +46,20 @@ AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
     const AutocompleteSchemeClassifier& scheme_classifier,
     const AutocompleteInput& input,
     const base::string16& fixed_up_input_text) {
+  const base::string16 title = titled_url_match.node->GetTitledUrlNodeTitle();
   const GURL& url = titled_url_match.node->GetTitledUrlNodeUrl();
+  const base::string16 path = ConcatAncestorsTitles(
+      titled_url_match.node->GetTitledUrlNodeAncestorTitles());
 
   // The AutocompleteMatch we construct is non-deletable because the only way to
   // support this would be to delete the underlying object that created the
   // titled_url_match. E.g., for the bookmark provider this would mean deleting
   // the underlying bookmark, which is unlikely to be what the user intends.
   AutocompleteMatch match(provider, relevance, false, type);
-  const base::string16& url_utf16 = base::UTF8ToUTF16(url.spec());
   match.destination_url = url;
+  match.RecordAdditionalInfo("Title", title);
+  match.RecordAdditionalInfo("URL", url.spec());
+  match.RecordAdditionalInfo("Path", path);
 
   bool match_in_scheme = false;
   bool match_in_subdomain = false;
@@ -43,9 +68,24 @@ AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
                                         &match_in_scheme, &match_in_subdomain);
   auto format_types = AutocompleteMatch::GetFormatTypes(
       input.parts().scheme.len > 0 || match_in_scheme, match_in_subdomain);
-
-  match.contents = url_formatter::FormatUrl(
+  const base::string16 formatted_url = url_formatter::FormatUrl(
       url, format_types, net::UnescapeRule::SPACES, nullptr, nullptr, nullptr);
+
+  if (base::GetFieldTrialParamByFeatureAsBool(
+          omnibox::kBookmarkPaths,
+          OmniboxFieldTrial::kBookmarkPathsUiReplaceUrl, false)) {
+    match.contents = path;
+  } else if (base::GetFieldTrialParamByFeatureAsBool(
+                 omnibox::kBookmarkPaths,
+                 OmniboxFieldTrial::kBookmarkPathsUiDynamicReplaceUrl, false)) {
+    match.contents = !titled_url_match.has_ancestor_match &&
+                             !titled_url_match.url_match_positions.empty()
+                         ? formatted_url
+                         : path;
+  } else {
+    match.contents = formatted_url;
+  }
+
   // Bookmark classification diverges from relevance scoring. Specifically,
   // 1) All occurrences of the input contribute to relevance; e.g. for the input
   // 'pre', the bookmark 'pre prefix' will be scored higher than 'pre suffix'.
@@ -63,7 +103,18 @@ AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
       ACMatchClassification::MATCH | ACMatchClassification::URL,
       ACMatchClassification::URL);
 
-  match.description = titled_url_match.node->GetTitledUrlNodeTitle();
+  if (base::GetFieldTrialParamByFeatureAsBool(
+          omnibox::kBookmarkPaths,
+          OmniboxFieldTrial::kBookmarkPathsUiReplaceTitle, false)) {
+    match.description = path + base::UTF8ToUTF16("/") + title;
+  } else if (base::GetFieldTrialParamByFeatureAsBool(
+                 omnibox::kBookmarkPaths,
+                 OmniboxFieldTrial::kBookmarkPathsUiAppendAfterTitle, false)) {
+    match.description = title + base::UTF8ToUTF16(" : ") + path;
+  } else {
+    match.description = title;
+  }
+
   base::TrimWhitespace(match.description, base::TRIM_LEADING,
                        &match.description);
   auto description_terms = FindTermMatches(input.text(), match.description);
@@ -74,7 +125,7 @@ AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
   // The inline_autocomplete_offset should be adjusted based on the formatting
   // applied to |fill_into_edit|.
   size_t inline_autocomplete_offset = URLPrefix::GetInlineAutocompleteOffset(
-      input.text(), fixed_up_input_text, false, url_utf16);
+      input.text(), fixed_up_input_text, false, base::UTF8ToUTF16(url.spec()));
   auto fill_into_edit_format_types = url_formatter::kFormatUrlOmitDefaults;
   if (match_in_scheme)
     fill_into_edit_format_types &= ~url_formatter::kFormatUrlOmitHTTP;

components/omnibox/browser/titled_url_match_utils_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "components/bookmarks/browser/titled_url_match.h"
 #include "components/bookmarks/browser/titled_url_node.h"
 #include "components/omnibox/browser/autocomplete_input.h"
@@ -16,6 +17,7 @@
 #include "components/omnibox/browser/autocomplete_provider.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/test_scheme_classifier.h"
+#include "components/omnibox/common/omnibox_features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/metrics_proto/omnibox_event.pb.h"
 #include "url/gurl.h"
@@ -37,8 +39,10 @@ class FakeAutocompleteProvider : public AutocompleteProvider {
 
 class MockTitledUrlNode : public bookmarks::TitledUrlNode {
  public:
-  MockTitledUrlNode(const base::string16& title, const GURL& url)
-      : title_(title), url_(url) {}
+  MockTitledUrlNode(const base::string16& title,
+                    const GURL& url,
+                    std::vector<base::string16> ancestors = {})
+      : title_(title), url_(url), ancestors_(ancestors) {}
 
   // TitledUrlNode
   const base::string16& GetTitledUrlNodeTitle() const override {
@@ -47,18 +51,23 @@ class MockTitledUrlNode : public bookmarks::TitledUrlNode {
   const GURL& GetTitledUrlNodeUrl() const override { return url_; }
   std::vector<base::StringPiece16> GetTitledUrlNodeAncestorTitles()
       const override {
-    return {};
+    std::vector<base::StringPiece16> ancestors;
+    std::transform(
+        ancestors_.begin(), ancestors_.end(), std::back_inserter(ancestors),
+        [](auto& ancestor) { return base::StringPiece16(ancestor); });
+    return ancestors;
   }
 
  private:
   base::string16 title_;
   GURL url_;
+  std::vector<base::string16> ancestors_;
 };
 
 std::string ACMatchClassificationsAsString(
-    const ACMatchClassifications& clasifications) {
+    const ACMatchClassifications& classifications) {
   std::string position_string("{");
-  for (auto classification : clasifications) {
+  for (auto classification : classifications) {
     position_string +=
         "{offset " + base::NumberToString(classification.offset) + ", style " +
         base::NumberToString(classification.style) + "}, ";
@@ -295,3 +304,104 @@ TEST(TitledUrlMatchUtilsTest, EmptyInlineAutocompletion) {
   EXPECT_FALSE(autocomplete_match.allowed_to_be_default_match);
   EXPECT_TRUE(autocomplete_match.inline_autocompletion.empty());
 }
+
+TEST(TitledUrlMatchUtilsTest, PathsInContentsAndDescription) {
+  scoped_refptr<FakeAutocompleteProvider> provider =
+      new FakeAutocompleteProvider(AutocompleteProvider::Type::TYPE_BOOKMARK);
+  TestSchemeClassifier classifier;
+  std::vector<base::string16> ancestors = {base::UTF8ToUTF16("parent"),
+                                           base::UTF8ToUTF16("grandparent")};
+
+  // Verifies contents and description of the AutocompleteMatch returned from
+  // |bookmarks::TitledUrlMatchToAutocompleteMatch()|.
+  auto test = [&](std::string title, std::string url, bool has_url_match,
+                  bool has_ancestor_match, std::string expected_contents,
+                  std::string expected_description) {
+    SCOPED_TRACE("title [" + title + "], url [" + url + "], has_url_match [" +
+                 std::string(has_url_match ? "true" : "false") +
+                 "], has_ancestor_match [" +
+                 std::string(has_ancestor_match ? "true" : "false") + "].");
+    MockTitledUrlNode node(base::UTF8ToUTF16(title), GURL(url), ancestors);
+    bookmarks::TitledUrlMatch titled_url_match;
+    titled_url_match.node = &node;
+    if (has_url_match)
+      titled_url_match.url_match_positions.push_back(
+          {8, 8});  // 8 in order to be after 'https://'
+    titled_url_match.has_ancestor_match = has_ancestor_match;
+    AutocompleteInput input(base::string16(), metrics::OmniboxEventProto::NTP,
+                            classifier);
+    AutocompleteMatch autocomplete_match = TitledUrlMatchToAutocompleteMatch(
+        titled_url_match, AutocompleteMatchType::BOOKMARK_TITLE, 1,
+        provider.get(), classifier, input, base::string16());
+    EXPECT_EQ(base::UTF16ToUTF8(autocomplete_match.contents),
+              expected_contents);
+    EXPECT_EQ(base::UTF16ToUTF8(autocomplete_match.description),
+              expected_description);
+  };
+
+  // Invokes |test()| with the 4 combinations of |has_url_match| true|false x
+  // |has_ancestor_match| true|false.
+  auto test_with_and_without_url_and_ancestor_matches =
+      [&](std::string title, std::string url, std::string expected_contents,
+          std::string expected_description) {
+        for (bool has_url_match : {false, true}) {
+          for (bool has_ancestor_match : {false, true}) {
+            test(title, url, has_url_match, has_ancestor_match,
+                 expected_contents, expected_description);
+          }
+        }
+      };
+
+  {
+    SCOPED_TRACE("Feature disabled");
+    test_with_and_without_url_and_ancestor_matches("title", "https://url.com",
+                                                   "url.com", "title");
+  }
+  {
+    SCOPED_TRACE("Feature enabled");
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeature(omnibox::kBookmarkPaths);
+    test_with_and_without_url_and_ancestor_matches("title", "https://url.com",
+                                                   "url.com", "title");
+  }
+  {
+    SCOPED_TRACE("Feature enabled, kBookmarkPathsUiReplaceTitle");
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeatureWithParameters(
+        omnibox::kBookmarkPaths,
+        {{OmniboxFieldTrial::kBookmarkPathsUiReplaceTitle, "true"}});
+    test_with_and_without_url_and_ancestor_matches(
+        "title", "https://url.com", "url.com", "grandparent/parent/title");
+  }
+  {
+    SCOPED_TRACE("Feature enabled, kBookmarkPathsUiReplaceUrl");
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeatureWithParameters(
+        omnibox::kBookmarkPaths,
+        {{OmniboxFieldTrial::kBookmarkPathsUiReplaceUrl, "true"}});
+    test_with_and_without_url_and_ancestor_matches(
+        "title", "https://url.com", "grandparent/parent", "title");
+  }
+  {
+    SCOPED_TRACE("Feature enabled, kBookmarkPathsUiAppendAfterTitle");
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeatureWithParameters(
+        omnibox::kBookmarkPaths,
+        {{OmniboxFieldTrial::kBookmarkPathsUiAppendAfterTitle, "true"}});
+    test_with_and_without_url_and_ancestor_matches(
+        "title", "https://url.com", "url.com", "title : grandparent/parent");
+  }
+  {
+    SCOPED_TRACE("Feature enabled, kBookmarkPathsUiDynamicReplaceUrl");
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeatureWithParameters(
+        omnibox::kBookmarkPaths,
+        {{OmniboxFieldTrial::kBookmarkPathsUiDynamicReplaceUrl, "true"}});
+    test("title", "https://url.com", false, false, "grandparent/parent",
+         "title");
+    test("title", "https://url.com", true, false, "url.com", "title");
+    test("title", "https://url.com", false, true, "grandparent/parent",
+         "title");
+    test("title", "https://url.com", true, true, "grandparent/parent", "title");
+  }
+}