[omnibox][rich-autocompletion][bookmark-paths] Show URL for rich ac bks.

**tldr**

When bookmarks are rich autocompleted with a path-matched shortcut text,
the omnibox inlines `tit[le] - folder/title`, with no indication of the
URL.

https://screenshot.googleplex.com/5Fp9eneifmYy7Pv


**Features**

With the launch of bookmark paths, bookmarks may show their bookmark
path instead of their URL in the dropdown. This is fine, since selecting
the suggestion will show the `fill_into_edit` (formatted URL) in the
omnibox.

With the launch of rich AC, non-URL inputs may inline matches. Inlined
matches show their `inline_autocompletion` and `additional_text` in the
omnibox, instead of their `fill_into_edit`. `inline_autocompletion` can
be the title, URL, or shortcut text, whichever the user was typing.
`additional_text` will equal `contents` when the user was typing title,
and empty otherwise. This too is fine, as `contents` is always the URL
for bookmarks.

But when both features are enabled jointly, then bookmarks can be
inlined with `contents` set to their path instead of their URL.


**Current behavior**

Say we suggest a bookmark shortcut named 'title', in folder 'folder, and
with the url 'url.com'.

When selected from the dropdown:
1) For the shortcut text 'url', the omnibox displays 'url.com'.
2) For the shortcut text 'url title', the omnibox displays 'url.com'.
3) For the shortcut text 'title', the omnibox displays 'url.com'.

When rich autocompleted and inlined:
4) For the shortcut text 'url', the omnibox displays 'url.com'.
5) For the shortcut text 'url title', the omnibox displays 'url title -
   url.com'.
6) For the shortcut text 'title', the omnibox displays 'title -
   folder/title'.


**The change**

Case 6 is problematic, as the user has no reminder where they're going
to navigate to when they select the suggestion. Case 6 didn't occur
until the launch of both features, hence it wasn't an issue until now.

This CL fixes case 6 to display 'title - url.com' instead.

For cases 5 & 6, this CL also changes *dropdown text* to display
'title - url.com' instead of 'title - folder/title'. Doing so addresses
the risk of  shortcut bookmarks showing stale paths after the bookmark
is moved. Though if the bookmark is also provided from the bookmark
provider, it  will be preferred, and the dropdown text will continue
showing 'title - folder/title'.


**Impelemntation details**

We previously added the `description_for_shortcuts` field to allow the
doc provider to show a different description when shortcuted than its
original description to avoid a stale description.

Since this situation is similar, i.e. we want to show a different
contents* (as opposed to description) when shortcuted than its original
contents, we reuse the same field. So for bookmarks, `description` will
be the title, `contents` will be the URL or path, while
`description_for_shortcut` will be the URL.

This is a bit messy, since `description_for_shortcuts` now actually
represents the contents for bookmarks (i.e. when
`swap_contents_and_description` is true). But since we hope to
eventually remove `swap_contents_and_description` field, the interim
messiness seems better than adding yet another field
`contents_for_shortcuts`.

This bug doesn't apply to non-shortcut bookmarks, as those won't rich
autocomlete and therefore always inline their URLs.

This fix won't fix existing shortcuts, as they were persisted with their
original contents rather than the `description_for_shortcuts`.

Bug: 1347986, 1129524
Change-Id: I285fb31b714848c75c89d7ea6c824a4b78018bf4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4159819
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098408}

components/omnibox/browser/autocomplete_match.cc

@@ -1255,6 +1255,12 @@ size_t AutocompleteMatch::EstimateMemoryUsage() const {
 
 void AutocompleteMatch::UpgradeMatchWithPropertiesFrom(
     AutocompleteMatch& duplicate_match) {
+  // TODO(manukh): There's some duplicate logic between `BetterDuplicate()` and
+  //   `UpgradeMatchWithPropertiesFrom()`. This is unavoidable due to having to
+  //   taking different fields from different duplicates, rather having 1 match
+  //   that's absolutely overrides all other matches. Perhaps we can avoid this
+  //   if we join the 2 functions.
+
   // For Entity Matches, absorb the duplicate match's |allowed_to_be_default|
   // and |inline_autocompletion| properties.
   if (type == AutocompleteMatchType::SEARCH_SUGGEST_ENTITY &&
@@ -1290,6 +1296,22 @@ void AutocompleteMatch::UpgradeMatchWithPropertiesFrom(
     actions = std::move(duplicate_match.actions);
   }
 
+  // Prefer fresh suggestion text over potentially stale shortcut text for
+  // bookmark paths and document metadata. Don't edit the omnibox text (i.e.
+  // `fill_into_edit`, `inline_autocompletion`, and `additional_text`) as the
+  // duplicate may not be `allowed_to_be_default_match`.
+  if (GetDeduplicationProviderPreferenceScore(
+          duplicate_match.provider->type()) >
+      GetDeduplicationProviderPreferenceScore(provider->type())) {
+    contents = duplicate_match.contents;
+    contents_class = duplicate_match.contents_class;
+    description = duplicate_match.description;
+    description_class = duplicate_match.description_class;
+    description_for_shortcuts = duplicate_match.description_for_shortcuts;
+    description_class_for_shortcuts =
+        duplicate_match.description_class_for_shortcuts;
+  }
+
   // Copy `rich_autocompletion_triggered` for counterfactual logging.
   if (rich_autocompletion_triggered == RichAutocompletionType::kNone) {
     rich_autocompletion_triggered =

components/omnibox/browser/autocomplete_match.h

@@ -687,10 +687,15 @@ struct AutocompleteMatch {
   // Additional helper text for each entry, such as a title or description.
   std::u16string description;
   ACMatchClassifications description_class;
-  // In the case of the document provider, the description includes a last
-  // updated date that may become stale. To avoid showing stale descriptions,
-  // when |description_for_shortcut| is not empty, it will be stored instead of
-  // |description| in the shortcuts provider.
+  // In the case of the document provider, `description` includes a last
+  // updated date that may become stale. Likewise for the bookmark provider,
+  // `contents` may be the path which may become stale when the bookmark is
+  // moved. To avoid showing stale text, when `description_for_shortcut`
+  // is not empty, it will be stored instead of `description` (or `contents` if
+  // `swap_contents_and_description` is true) in the shortcuts provider.
+  // TODO(manukh) This is a temporary misnomer (since it can represent both
+  //   `description` and `contents`) until `swap_contents_and_description` is
+  //   removed.
   std::u16string description_for_shortcuts;
   ACMatchClassifications description_class_for_shortcuts;
 

components/omnibox/browser/autocomplete_match_unittest.cc

@@ -443,28 +443,52 @@ TEST(AutocompleteMatchTest, DedupeDriveURLs) {
     CheckDuplicateCase(caseI);
 }
 
-TEST(AutocompleteMatchTest, UpgradeMatchPropertiesWhileMergingDuplicates) {
-  AutocompleteMatch search_history_match(nullptr, 500, true,
+TEST(AutocompleteMatchTest, UpgradeMatchWithPropertiesFrom) {
+  scoped_refptr<FakeAutocompleteProvider> bookmark_provider =
+      new FakeAutocompleteProvider(AutocompleteProvider::Type::TYPE_BOOKMARK);
+  scoped_refptr<FakeAutocompleteProvider> history_provider =
+      new FakeAutocompleteProvider(
+          AutocompleteProvider::Type::TYPE_HISTORY_QUICK);
+  scoped_refptr<FakeAutocompleteProvider> search_provider =
+      new FakeAutocompleteProvider(AutocompleteProvider::Type::TYPE_SEARCH);
+
+  AutocompleteMatch search_history_match(search_provider.get(), 500, true,
                                          AutocompleteMatchType::SEARCH_HISTORY);
 
   // Entity match should get the increased score, but not change types.
-  AutocompleteMatch entity_match(nullptr, 400, false,
+  AutocompleteMatch entity_match(search_provider.get(), 400, false,
                                  AutocompleteMatchType::SEARCH_SUGGEST_ENTITY);
   entity_match.UpgradeMatchWithPropertiesFrom(search_history_match);
-  EXPECT_EQ(500, entity_match.relevance);
-  EXPECT_EQ(AutocompleteMatchType::SEARCH_SUGGEST_ENTITY, entity_match.type);
+  EXPECT_EQ(entity_match.relevance, 500);
+  EXPECT_EQ(entity_match.type, AutocompleteMatchType::SEARCH_SUGGEST_ENTITY);
 
   // Suggest and search-what-typed matches should get the search history type.
-  AutocompleteMatch suggest_match(nullptr, 400, true,
+  AutocompleteMatch suggest_match(search_provider.get(), 400, true,
                                   AutocompleteMatchType::SEARCH_SUGGEST);
   AutocompleteMatch search_what_you_typed(
-      nullptr, 400, true, AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED);
+      search_provider.get(), 400, true,
+      AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED);
   suggest_match.UpgradeMatchWithPropertiesFrom(search_history_match);
   search_what_you_typed.UpgradeMatchWithPropertiesFrom(search_history_match);
-  EXPECT_EQ(500, suggest_match.relevance);
-  EXPECT_EQ(500, search_what_you_typed.relevance);
-  EXPECT_EQ(AutocompleteMatchType::SEARCH_HISTORY, suggest_match.type);
-  EXPECT_EQ(AutocompleteMatchType::SEARCH_HISTORY, search_what_you_typed.type);
+  EXPECT_EQ(suggest_match.relevance, 500);
+  EXPECT_EQ(search_what_you_typed.relevance, 500);
+  EXPECT_EQ(suggest_match.type, AutocompleteMatchType::SEARCH_HISTORY);
+  EXPECT_EQ(search_what_you_typed.type, AutocompleteMatchType::SEARCH_HISTORY);
+
+  // Some providers should bestow their suggestion texts even if not the primary
+  // duplicate.
+  AutocompleteMatch history_match(history_provider.get(), 800, true,
+                                  AutocompleteMatchType::HISTORY_TITLE);
+  AutocompleteMatch bookmark_match(bookmark_provider.get(), 400, true,
+                                   AutocompleteMatchType::BOOKMARK_TITLE);
+  history_match.contents = u"overwrite";
+  history_match.inline_autocompletion = u"preserve";
+  bookmark_match.contents = u"propagate";
+  bookmark_match.inline_autocompletion = u"discard";
+  history_match.UpgradeMatchWithPropertiesFrom(bookmark_match);
+  EXPECT_EQ(history_match.type, AutocompleteMatchType::HISTORY_TITLE);
+  EXPECT_EQ(history_match.contents, u"propagate");
+  EXPECT_EQ(history_match.inline_autocompletion, u"preserve");
 }
 
 TEST(AutocompleteMatchTest, MergeScoringSignals) {

components/omnibox/browser/shortcuts_backend.cc

@@ -66,23 +66,6 @@ AutocompleteMatch::Type GetTypeForShortcut(AutocompleteMatch::Type type) {
   }
 }
 
-// Get either `description_for_shortcuts` if non-empty or fallback to
-// `description`.
-const std::u16string& GetDescription(const AutocompleteMatch& match) {
-  return match.description_for_shortcuts.empty()
-             ? match.description
-             : match.description_for_shortcuts;
-}
-
-// Get either `description_class_for_shortcuts` if non-empty or fallback to
-// `description_class`.
-const ACMatchClassifications& GetDescriptionClass(
-    const AutocompleteMatch& match) {
-  return match.description_class_for_shortcuts.empty()
-             ? match.description_class
-             : match.description_class_for_shortcuts;
-}
-
 // Expand the last word in `text` to a full word in `match_text`. E.g., if
 // `text` is 'Cha Aznav' and the `match_text` is 'Charles Aznavour', will return
 // 'Cha Aznavour'.
@@ -182,6 +165,56 @@ std::u16string ExpandToFullWord(const std::u16string& text,
 
 // ShortcutsBackend -----------------------------------------------------------
 
+// static
+const std::u16string& ShortcutsBackend::GetDescription(
+    const AutocompleteMatch& match) {
+  return match.swap_contents_and_description ||
+                 match.description_for_shortcuts.empty()
+             ? match.description
+             : match.description_for_shortcuts;
+}
+
+// static
+const std::u16string& ShortcutsBackend::GetSwappedDescription(
+    const AutocompleteMatch& match) {
+  return match.swap_contents_and_description ? GetContents(match)
+                                             : GetDescription(match);
+}
+
+// static
+const ACMatchClassifications& ShortcutsBackend::GetDescriptionClass(
+    const AutocompleteMatch& match) {
+  return match.swap_contents_and_description ||
+                 match.description_class_for_shortcuts.empty()
+             ? match.description_class
+             : match.description_class_for_shortcuts;
+}
+
+// static
+const std::u16string& ShortcutsBackend::GetContents(
+    const AutocompleteMatch& match) {
+  return !match.swap_contents_and_description ||
+                 match.description_for_shortcuts.empty()
+             ? match.contents
+             : match.description_for_shortcuts;
+}
+
+// static
+const std::u16string& ShortcutsBackend::GetSwappedContents(
+    const AutocompleteMatch& match) {
+  return match.swap_contents_and_description ? match.description
+                                             : match.contents;
+}
+
+// static
+const ACMatchClassifications& ShortcutsBackend::GetContentsClass(
+    const AutocompleteMatch& match) {
+  return !match.swap_contents_and_description ||
+                 match.description_class_for_shortcuts.empty()
+             ? match.contents_class
+             : match.description_class_for_shortcuts;
+}
+
 ShortcutsBackend::ShortcutsBackend(
     TemplateURLService* template_url_service,
     std::unique_ptr<SearchTermsData> search_terms_data,
@@ -305,13 +338,10 @@ void ShortcutsBackend::AddOrUpdateShortcut(const std::u16string& text,
   // is usually also recognizable and helpful when there are whitespace or other
   // discrepancies between the title and host (e.g. 'Stack Overflow' and
   // 'stackoverflow.com').
-  const auto& match_text = match.swap_contents_and_description
-                               ? GetDescription(match)
-                               : match.contents;
   const auto expanded_text =
       OmniboxFieldTrial::IsShortcutExpandingEnabled()
           ? ExpandToFullWord(
-                text, match_text + u" " +
+                text, GetSwappedContents(match) + u" " +
                           base::UTF8ToUTF16(match.destination_url.host()))
           : text;
   AddShortcut(ShortcutsDatabase::Shortcut(
@@ -346,8 +376,8 @@ ShortcutsDatabase::Shortcut::MatchCore ShortcutsBackend::MatchToMatchCore(
 
   return ShortcutsDatabase::Shortcut::MatchCore(
       normalized_match->fill_into_edit, normalized_match->destination_url,
-      normalized_match->document_type, normalized_match->contents,
-      StripMatchMarkers(normalized_match->contents_class),
+      normalized_match->document_type, GetContents(*normalized_match),
+      StripMatchMarkers(GetContentsClass(*normalized_match)),
       GetDescription(*normalized_match),
       StripMatchMarkers(GetDescriptionClass(*normalized_match)),
       normalized_match->transition, match_type, normalized_match->keyword);

components/omnibox/browser/shortcuts_backend.h

@@ -47,6 +47,23 @@ class ShortcutsBackend : public RefcountedKeyedService,
   typedef std::multimap<std::u16string, const ShortcutsDatabase::Shortcut>
       ShortcutMap;
 
+  // Get either `contents`, `description`, or `description_class_for_shortcuts`
+  // (or their classifications) depending on the method called,
+  // `swap_contents_and_description`, and whether
+  // `description_class_for_shortcuts` is empty.
+  // TODO(manukh): Simplify these once `swap_contents_and_description` is
+  //   removed.
+  static const std::u16string& GetDescription(const AutocompleteMatch& match);
+  static const std::u16string& GetSwappedDescription(
+      const AutocompleteMatch& match);
+  static const ACMatchClassifications& GetDescriptionClass(
+      const AutocompleteMatch& match);
+  static const std::u16string& GetContents(const AutocompleteMatch& match);
+  static const std::u16string& GetSwappedContents(
+      const AutocompleteMatch& match);
+  static const ACMatchClassifications& GetContentsClass(
+      const AutocompleteMatch& match);
+
   // For unit testing, set |suppress_db| to true to prevent creation
   // of the database, in which case all operations are performed in memory only.
   ShortcutsBackend(TemplateURLService* template_url_service,

components/omnibox/browser/shortcuts_provider.cc

@@ -508,17 +508,18 @@ AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
 #else
     } else {
 #endif
-      // Try rich autocompletion first. For document suggestions,
-      // `match.contents` is the title, while `description` is something like
-      // 'Google Docs' and shouldn't be autocompleted. For all other nav
-      // suggestions, `contents` is the URL and `description` is the title.
+      // Try rich autocompletion first. For document suggestions, hide the
+      // URL from `additional_text` and don't try to inline the metadata (e.g.
+      // 'Google Docs' or '1/1/2023').
       bool autocompleted =
           match.type == AutocompleteMatch::Type::DOCUMENT_SUGGESTION
-              ? match.TryRichAutocompletion(u"", match.contents, input,
-                                            shortcut.text)
-              : match.TryRichAutocompletion(match.contents, match.description,
-                                            input, shortcut.text);
-
+              ? match.TryRichAutocompletion(
+                    u"", ShortcutsBackend::GetSwappedContents(match), input,
+                    shortcut.text)
+              : match.TryRichAutocompletion(
+                    ShortcutsBackend::GetSwappedContents(match),
+                    ShortcutsBackend::GetSwappedDescription(match), input,
+                    shortcut.text);
       if (!autocompleted) {
         const size_t inline_autocomplete_offset =
             URLPrefix::GetInlineAutocompleteOffset(

components/omnibox/browser/titled_url_match_utils.cc

@@ -85,11 +85,14 @@ AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
   // Otherwise, display the path, even if the input matches both or neither.
   // Except if kBookmarkPaths is disabled, in which case, always display the
   // URL.
-  match.contents = !base::FeatureList::IsEnabled(omnibox::kBookmarkPaths) ||
-                           (!titled_url_match.has_ancestor_match &&
-                            !titled_url_match.url_match_positions.empty())
-                       ? formatted_url
-                       : path;
+  bool show_path = base::FeatureList::IsEnabled(omnibox::kBookmarkPaths) &&
+                   (titled_url_match.has_ancestor_match ||
+                    titled_url_match.url_match_positions.empty());
+  match.contents = show_path ? path : formatted_url;
+  // The path can become stale (when the bookmark is moved). So persist the URL
+  // instead when creating shortcuts.
+  if (show_path)
+    match.description_for_shortcuts = formatted_url;
 
   // Bookmark classification diverges from relevance scoring. Specifically,
   // 1) All occurrences of the input contribute to relevance; e.g. for the input
@@ -108,6 +111,14 @@ AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
       ACMatchClassification::MATCH | ACMatchClassification::URL,
       ACMatchClassification::URL);
 
+  if (show_path) {
+    auto terms = FindTermMatches(input.text(), match.description_for_shortcuts);
+    match.description_class_for_shortcuts = ClassifyTermMatches(
+        terms, match.description_for_shortcuts.length(),
+        ACMatchClassification::MATCH | ACMatchClassification::URL,
+        ACMatchClassification::URL);
+  }
+
   match.description = title;
 
   base::TrimWhitespace(match.description, base::TRIM_LEADING,