Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(120)

Issue 62153002: Omnibox: Fix Inline Autocompletion of Navsuggest in Forced Query Mode (Closed)

Created:
7 years, 1 month ago by Mark P
Modified:
7 years, 1 month ago
Reviewers:
Peter Kasting
CC:
chromium-reviews, James Su
Visibility:
Public.

Description

Omnibox: Fix Inline Autocompletion of Navsuggest in Forced Query Mode 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 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233544

Patch Set 1 #

Patch Set 2 : reupload #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -8 lines) Patch
M chrome/browser/autocomplete/search_provider.cc View 2 chunks +7 lines, -8 lines 0 comments Download
M chrome/browser/autocomplete/search_provider_unittest.cc View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Mark P
Simple. I'm not sure my code review explanation is clear, but regardless you should be ...
7 years, 1 month ago (2013-11-06 15:52:24 UTC) #1
Peter Kasting
LGTM
7 years, 1 month ago (2013-11-06 19:55:42 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/62153002/20001
7 years, 1 month ago (2013-11-06 23:33:46 UTC) #3
commit-bot: I haz the power
7 years, 1 month ago (2013-11-07 08:14:28 UTC) #4
Message was sent while issue was closed.
Change committed as 233544

Powered by Google App Engine
This is Rietveld 408576698