Skip to content

Commit

Permalink
[omnibox] [rich-autocompletion] Fix shortcut provider fill_into_edit
Browse files Browse the repository at this point in the history
When rich autocompleting titles, |fill_into_edit| &
|fill_into_edit_additional_text| are swapped (i.e. the former contains
the title, and the latter contains the URL). This is consistent with how
the suggestion should be displayed. But the shortcut DB must persist the
original (i.e. URL) |fill_into_edit|; otherwise the shortcut provider
may autocomplete the title even when title autocompletion is
inappropriate (e.g. because the input is too short). Therefore,
|swapped_fill_into_edit| is set to true when rich autocompleting titles.

Bug: 1062446
Change-Id: I925752e60d2529d0fd8f9fac7f51aea169bc678d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2399567
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805176}
  • Loading branch information
manukh authored and Commit Bot committed Sep 9, 2020
1 parent 6596244 commit c534089
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 4 deletions.
4 changes: 4 additions & 0 deletions components/omnibox/browser/autocomplete_match.cc
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ AutocompleteMatch::AutocompleteMatch(const AutocompleteMatch& match)
deletable(match.deletable),
fill_into_edit(match.fill_into_edit),
fill_into_edit_additional_text(match.fill_into_edit_additional_text),
swapped_fill_into_edit(match.swapped_fill_into_edit),
inline_autocompletion(match.inline_autocompletion),
prefix_autocompletion(match.prefix_autocompletion),
allowed_to_be_default_match(match.allowed_to_be_default_match),
Expand Down Expand Up @@ -202,6 +203,7 @@ AutocompleteMatch& AutocompleteMatch::operator=(
deletable = match.deletable;
fill_into_edit = match.fill_into_edit;
fill_into_edit_additional_text = match.fill_into_edit_additional_text;
swapped_fill_into_edit = match.swapped_fill_into_edit;
inline_autocompletion = match.inline_autocompletion;
prefix_autocompletion = match.prefix_autocompletion;
allowed_to_be_default_match = match.allowed_to_be_default_match;
Expand Down Expand Up @@ -1227,6 +1229,7 @@ bool AutocompleteMatch::TryRichAutocompletion(
base::CompareCase::SENSITIVE)) {
fill_into_edit = secondary_text;
fill_into_edit_additional_text = primary_text;
swapped_fill_into_edit = true;
inline_autocompletion = secondary_text.substr(input_text_lower.length());
allowed_to_be_default_match = true;
RecordAdditionalInfo("autocompletion", "secondary & prefix");
Expand Down Expand Up @@ -1266,6 +1269,7 @@ bool AutocompleteMatch::TryRichAutocompletion(
if (can_autocomplete_titles && secondary_find_index != base::string16::npos) {
fill_into_edit = secondary_text;
fill_into_edit_additional_text = primary_text;
swapped_fill_into_edit = true;
inline_autocompletion =
secondary_text.substr(secondary_find_index + input_text_lower.length());
prefix_autocompletion = secondary_text.substr(0, secondary_find_index);
Expand Down
12 changes: 9 additions & 3 deletions components/omnibox/browser/autocomplete_match.h
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,6 @@ struct AutocompleteMatch {
// returned by various providers. This is used to rank matches among all
// responding providers, so different providers must be carefully tuned to
// supply matches with appropriate relevance.
//
// TODO(pkasting): http://b/1111299 This should be calculated algorithmically,
// rather than being a fairly fixed value defined by the table above.
int relevance = 0;

// How many times this result was typed in / selected from the omnibox.
Expand All @@ -482,6 +479,15 @@ struct AutocompleteMatch {
// |fill_into_edit|. Always empty if kRichAutocompletionShowTitlesParam is
// disabled.
base::string16 fill_into_edit_additional_text;
// When rich autocompleting titles, |fill_into_edit| &
// |fill_into_edit_additional_text| are swapped (i.e. the former contains the
// title, and the latter contains the URL). This is consistent with how the
// suggestion should be displayed. But the shortcut DB must persist the
// original (i.e. URL) |fill_into_edit|; otherwise the shortcut provider may
// autocomplete the title even when title autocompletion is inappropriate
// (e.g. because the input is too short). Therefore, |swapped_fill_into_edit|
// is set to true when rich autocompleting titles.
bool swapped_fill_into_edit = false;

// The inline autocompletion to display after the user's input in the
// omnibox, if this match becomes the default match. It may be empty.
Expand Down
6 changes: 5 additions & 1 deletion components/omnibox/browser/shortcuts_backend.cc
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,12 @@ ShortcutsDatabase::Shortcut::MatchCore ShortcutsBackend::MatchToMatchCore(
? normalized_match->description_class
: normalized_match->description_class_for_shortcuts;

auto fill_into_edit = normalized_match->swapped_fill_into_edit
? normalized_match->fill_into_edit_additional_text
: normalized_match->fill_into_edit;

return ShortcutsDatabase::Shortcut::MatchCore(
normalized_match->fill_into_edit, normalized_match->destination_url,
fill_into_edit, normalized_match->destination_url,
static_cast<int>(normalized_match->document_type),
normalized_match->contents,
StripMatchMarkers(normalized_match->contents_class), description,
Expand Down

0 comments on commit c534089

Please sign in to comment.