Omnibox: Trim Leading Spaces After Initial Forced-Query '?' (issue 11896016)

4 views
Skip to first unread message

mpea...@chromium.org

unread,
Jan 21, 2013, 10:37:45 AM1/21/13
to pkas...@chromium.org, chromium...@chromium.org, su...@chromium.org
Reviewers: Peter Kasting,

Description:
Omnibox: Trim Leading Spaces After Initial Forced-Query '?'

BUG=170727


Please review this at https://codereview.chromium.org/11896016/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/browser/autocomplete/autocomplete_input.cc


Index: chrome/browser/autocomplete/autocomplete_input.cc
diff --git a/chrome/browser/autocomplete/autocomplete_input.cc
b/chrome/browser/autocomplete/autocomplete_input.cc
index
ae0d4bc0e45c045df4850bbae095835f55960990..136219df32a19a8578d078aeb0a0f6afb954a5e6
100644
--- a/chrome/browser/autocomplete/autocomplete_input.cc
+++ b/chrome/browser/autocomplete/autocomplete_input.cc
@@ -72,6 +72,16 @@ AutocompleteInput::AutocompleteInput(const string16&
text,

size_t chars_removed = RemoveForcedQueryStringIfNecessary(type_, &text_);
AdjustCursorPositionIfNecessary(chars_removed, &cursor_position_);
+ if (chars_removed) {
+ // Remove spaces between opening questions mark and first actual
character.
+ string16 trimmed_text;
+ if ((TrimWhitespace(text_, TRIM_LEADING, &trimmed_text) &
TRIM_LEADING) !=
+ 0) {
+ AdjustCursorPositionIfNecessary(text_.length() -
trimmed_text.length(),
+ &cursor_position_);
+ text_ = trimmed_text;
+ }
+ }
}

AutocompleteInput::~AutocompleteInput() {


pkas...@chromium.org

unread,
Jan 21, 2013, 1:53:15 PM1/21/13
to mpea...@chromium.org, chromium...@chromium.org, su...@chromium.org
Make sure the following still works correctly:

* Search for "foo bar"
* Double-check that "foo b" will now inline-autocomplete
* Type "? foo b"
* Ensure the autocompletion is "ar", not "r" or something


https://codereview.chromium.org/11896016/diff/1/chrome/browser/autocomplete/autocomplete_input.cc
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11896016/diff/1/chrome/browser/autocomplete/autocomplete_input.cc#newcode76
chrome/browser/autocomplete/autocomplete_input.cc:76: // Remove spaces
between opening questions mark and first actual character.
Nit: questions -> question

https://codereview.chromium.org/11896016/

pkas...@chromium.org

unread,
Jan 21, 2013, 1:53:21 PM1/21/13
to mpea...@chromium.org, chromium...@chromium.org, su...@chromium.org
LGTM

https://codereview.chromium.org/11896016/

pkas...@chromium.org

unread,
Jan 21, 2013, 1:53:40 PM1/21/13
to mpea...@chromium.org, chromium...@chromium.org, su...@chromium.org
Sigh, three messages... forgot to ask if this could get a test too

https://codereview.chromium.org/11896016/

mpea...@chromium.org

unread,
Jan 21, 2013, 3:50:54 PM1/21/13
to pkas...@chromium.org, chromium...@chromium.org, su...@chromium.org

Added unit tests.

Also did the manual test you requested. Works fine.

Submitting.

--mark



https://codereview.chromium.org/11896016/diff/1/chrome/browser/autocomplete/autocomplete_input.cc
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11896016/diff/1/chrome/browser/autocomplete/autocomplete_input.cc#newcode76
chrome/browser/autocomplete/autocomplete_input.cc:76: // Remove spaces
between opening questions mark and first actual character.
On 2013/01/21 18:53:15, Peter Kasting wrote:
> Nit: questions -> question

Done.

https://codereview.chromium.org/11896016/

commi...@chromium.org

unread,
Jan 21, 2013, 3:51:58 PM1/21/13
to mpea...@chromium.org, pkas...@chromium.org, chromium...@chromium.org, su...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpea...@chromium.org/11896016/5001


https://chromiumcodereview.appspot.com/11896016/

commi...@chromium.org

unread,
Jan 21, 2013, 6:23:28 PM1/21/13
to mpea...@chromium.org, pkas...@chromium.org, chromium...@chromium.org, su...@chromium.org
Change committed as 177957

https://chromiumcodereview.appspot.com/11896016/
Reply all
Reply to author
Forward
0 new messages