Skip to content

Commit

Permalink
Omnibox: Fix Inline Autocompletion of Navsuggest in Forced Query Mode
Browse files Browse the repository at this point in the history
This moves the block of code that adds the "?" from one place to another.
Its original location was wrong because it incremented
|inline_autocomplete_offset| before passing it to FormatUrl().
This could change how FormatUrl() would correct inline_autocomplete_offset
when its value is near the end of the formatted string.
This is wrong.

In both locations, in effect we are adding the "?" prefix after FormatUrl()
runs.  This means we should correct the inline_autocomplete_offset value
to compensate for the extra character "?" only after FormatUrl() runs.

This fixes the bug.

I also added a test that would've caught this issue.

BUG=313451

Review URL: https://codereview.chromium.org/62153002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@233544 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
mpearson@chromium.org committed Nov 7, 2013
1 parent f1b6487 commit 1411903
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 8 deletions.
15 changes: 7 additions & 8 deletions chrome/browser/autocomplete/search_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1777,14 +1777,6 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
bool trim_http = !AutocompleteInput::HasHTTPScheme(input) &&
(!prefix || (match_start != 0));

// Preserve the forced query '?' prefix in |match.fill_into_edit|.
// Otherwise, user edits to a suggestion would show non-Search results.
if (input_.type() == AutocompleteInput::FORCED_QUERY) {
match.fill_into_edit = ASCIIToUTF16("?");
if (inline_autocomplete_offset != string16::npos)
++inline_autocomplete_offset;
}

const std::string languages(
profile_->GetPrefs()->GetString(prefs::kAcceptLanguages));
const net::FormatUrlTypes format_types =
Expand All @@ -1794,6 +1786,13 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
net::FormatUrl(navigation.url(), languages, format_types,
net::UnescapeRule::SPACES, NULL, NULL,
&inline_autocomplete_offset));
// Preserve the forced query '?' prefix in |match.fill_into_edit|.
// Otherwise, user edits to a suggestion would show non-Search results.
if (input_.type() == AutocompleteInput::FORCED_QUERY) {
match.fill_into_edit.insert(0, ASCIIToUTF16("?"));
if (inline_autocomplete_offset != string16::npos)
++inline_autocomplete_offset;
}
if (!input_.prevent_inline_autocomplete() &&
(inline_autocomplete_offset != string16::npos)) {
DCHECK(inline_autocomplete_offset <= match.fill_into_edit.length());
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/autocomplete/search_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2362,6 +2362,8 @@ TEST_F(SearchProviderTest, NavigationInline) {
"?www.abc.com", "c.com", true },
{ "?ab", "http://www.abc.com",
"?www.abc.com", "c.com", true },
{ "?abc.com", "http://www.abc.com",
"?www.abc.com", "", true },
};

for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
Expand Down

0 comments on commit 1411903

Please sign in to comment.