Allow space to accept keyword even when inline autocomplete is available.

Allow space to accept keyword even when inline autocomplete is available.

This CL assumes keyword query is always synchronous.

BUG=70527
TEST=See bug report.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72457

[James Su, 2011-01-24 20:31:22]

One point: this CL only accepts keyword when the selection is inline
autocomplete. It doesn't work for selection explicitly made by the user. I think
it's desired behavior.

On 2011/01/24 05:16:29, James Su wrote:

[Peter Kasting, 2011-01-24 22:12:24]

On 2011/01/24 20:31:22, James Su wrote:
> One point: this CL only accepts keyword when the selection is inline
> autocomplete. It doesn't work for selection explicitly made by the user. I
think
> it's desired behavior.

We should treat user selections and inline autocompletion the same here.  (We
try to treat them the same in general, because they're visually
indistinguishable.)  Since we want to let this case work for inline
autocompletion, let's make it work for normal selections too; I can't think of a
real downside of that.

[James Su, 2011-01-24 22:53:50]

On 2011/01/24 22:12:24, Peter Kasting wrote:
> On 2011/01/24 20:31:22, James Su wrote:
> > One point: this CL only accepts keyword when the selection is inline
> > autocomplete. It doesn't work for selection explicitly made by the user. I
> think
> > it's desired behavior.
> 
> We should treat user selections and inline autocompletion the same here.  (We
> try to treat them the same in general, because they're visually
> indistinguishable.)  Since we want to let this case work for inline
> autocompletion, let's make it work for normal selections too; I can't think of
a
> real downside of that.

If the user changes the selection manually, then he/she may just want to edit a
search term. Accepting keyword in such case may surprise the user.
And it's actually not trivial to handle this case, because the |user_text_| will
be changed to included the selected part, then it's not that easy to check if
the selected part was replaced by a space or the user just deleted it, or
something else.

[Peter Kasting, 2011-01-24 23:05:08]

On 2011/01/24 22:53:50, James Su wrote:
> If the user changes the selection manually, then he/she may just want to edit
a
> search term. Accepting keyword in such case may surprise the user.

I thought through the use cases here and I can't come up with anything that's
going to be bad.  The only case we'd fire keyword mode on is if the user is
trying to type "<keyword> <more text here>", and in that case, our original
behavior would have switched into keyword mode too (just one character later
than now) so we're not losing anything.

To be sure, I went and talked to sky about this, and he agreed that in this case
we should treat user selection and inline autocompletion the same.

> And it's actually not trivial to handle this case, because the |user_text_|
will
> be changed to included the selected part, then it's not that easy to check if
> the selected part was replaced by a space or the user just deleted it, or
> something else.

I know.  The code that currently sets |old_user_text| will need to get the
unselected portion of |user_text_|.  I think just doing that should be enough to
make this work, but I could be wrong.

[James Su, 2011-01-24 23:25:08]

On 2011/01/24 23:05:08, Peter Kasting wrote:
> On 2011/01/24 22:53:50, James Su wrote:
> > If the user changes the selection manually, then he/she may just want to
edit
> a
> > search term. Accepting keyword in such case may surprise the user.
> 
> I thought through the use cases here and I can't come up with anything that's
> going to be bad.  The only case we'd fire keyword mode on is if the user is
> trying to type "<keyword> <more text here>", and in that case, our original
> behavior would have switched into keyword mode too (just one character later
> than now) so we're not losing anything.
> 
> To be sure, I went and talked to sky about this, and he agreed that in this
case
> we should treat user selection and inline autocompletion the same.
Supporting it is probably no harm. I just think the chance that a user wants to
accept a keyword by this way would be very rare.

> 
> > And it's actually not trivial to handle this case, because the |user_text_|
> will
> > be changed to included the selected part, then it's not that easy to check
if
> > the selected part was replaced by a space or the user just deleted it, or
> > something else.
> 
> I know.  The code that currently sets |old_user_text| will need to get the
> unselected portion of |user_text_|.  I think just doing that should be enough
to
> make this work, but I could be wrong.

After re-checking the code, I think we can handle this case without sending more
information to the model. I'll update this CL to support this case asap.

[James Su, 2011-01-24 23:46:07]

One quick question: shall we accept keyword "foo" in such case:
1. Paste "foo bar" (there is a space in between)
2. Select "bar"
3. Press space

On 2011/01/24 23:25:08, James Su wrote:
> On 2011/01/24 23:05:08, Peter Kasting wrote:
> > On 2011/01/24 22:53:50, James Su wrote:
> > > If the user changes the selection manually, then he/she may just want to
> edit
> > a
> > > search term. Accepting keyword in such case may surprise the user.
> > 
> > I thought through the use cases here and I can't come up with anything
that's
> > going to be bad.  The only case we'd fire keyword mode on is if the user is
> > trying to type "<keyword> <more text here>", and in that case, our original
> > behavior would have switched into keyword mode too (just one character later
> > than now) so we're not losing anything.
> > 
> > To be sure, I went and talked to sky about this, and he agreed that in this
> case
> > we should treat user selection and inline autocompletion the same.
> Supporting it is probably no harm. I just think the chance that a user wants
to
> accept a keyword by this way would be very rare.
> 
> > 
> > > And it's actually not trivial to handle this case, because the
|user_text_|
> > will
> > > be changed to included the selected part, then it's not that easy to check
> if
> > > the selected part was replaced by a space or the user just deleted it, or
> > > something else.
> > 
> > I know.  The code that currently sets |old_user_text| will need to get the
> > unselected portion of |user_text_|.  I think just doing that should be
enough
> to
> > make this work, but I could be wrong.
> 
> After re-checking the code, I think we can handle this case without sending
more
> information to the model. I'll update this CL to support this case asap.

[Peter Kasting, 2011-01-24 23:47:52]

On 2011/01/24 23:46:07, James Su wrote:
> One quick question: shall we accept keyword "foo" in such case:
> 1. Paste "foo bar" (there is a space in between)
> 2. Select "bar"
> 3. Press space

I wouldn't, for now at least.

[James Su, 2011-01-25 01:04:36]

CL has been updated to cover the selection case.

[Peter Kasting, 2011-01-25 02:12:21]

LGTM

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:632: const bool
had_inline_autocomplete = !inline_autocomplete_text_.empty();
Nit: You can roll this back into the statement below now.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:665: // Try to accept the
current keyword if the user only typed a space at the
Nit: This comment might now be more accurate as:

  // Change to keyword mode if the user has typed a keyword name and is now
  // pressing space after the name.  Accepting the keyword will update our
  // state, so in that case there's no need to also return true here.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:668: if (text_differs &&
allow_keyword_ui_change && !just_deleted_text &&
Nit: Even more condensed:

  return !text_differs || !allow_keyword_ui_change || just_deleted_text ||
      !MaybeAcceptKeywordBySpace(old_user_text, user_text_);

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:791: !old_user_text.empty() &&
new_user_text.length() >= 2 &&
Nit: "!old_user_text.empty()" is unnecessary given the length check you do
below, which will reject all cases where this is empty.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:795:
IsSpaceCharForAcceptingKeyword(new_user_text[new_user_text.length() - 1]) &&
Nit: 80 columns.

Also, I think it'll be a littler clearer if we move this check and the next one
-- the ones that have to do with what characters occur at the end of
|new_user_text| -- to just after the "length >= 2" line (before the
|old_user_text|-related checks).

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:745: //
Keyword could be accepted by pressing space with a selected range at the
Nit: could -> can

[James Su, 2011-01-25 02:52:48]

Committed.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:632: const bool
had_inline_autocomplete = !inline_autocomplete_text_.empty();
On 2011/01/25 02:12:22, Peter Kasting wrote:
> Nit: You can roll this back into the statement below now.

Done.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:665: // Try to accept the
current keyword if the user only typed a space at the
On 2011/01/25 02:12:22, Peter Kasting wrote:
> Nit: This comment might now be more accurate as:
> 
>   // Change to keyword mode if the user has typed a keyword name and is now
>   // pressing space after the name.  Accepting the keyword will update our
>   // state, so in that case there's no need to also return true here.

Done.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:668: if (text_differs &&
allow_keyword_ui_change && !just_deleted_text &&
On 2011/01/25 02:12:22, Peter Kasting wrote:
> Nit: Even more condensed:
> 
>   return !text_differs || !allow_keyword_ui_change || just_deleted_text ||
>       !MaybeAcceptKeywordBySpace(old_user_text, user_text_);
I'd prefer to keep the old one, which is more readable to me.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:791: !old_user_text.empty() &&
new_user_text.length() >= 2 &&
On 2011/01/25 02:12:22, Peter Kasting wrote:
> Nit: "!old_user_text.empty()" is unnecessary given the length check you do
> below, which will reject all cases where this is empty.

Done.

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:795:
IsSpaceCharForAcceptingKeyword(new_user_text[new_user_text.length() - 1]) &&
On 2011/01/25 02:12:22, Peter Kasting wrote:
> Nit: 80 columns.
> 
> Also, I think it'll be a littler clearer if we move this check and the next
one
> -- the ones that have to do with what characters occur at the end of
> |new_user_text| -- to just after the "length >= 2" line (before the
> |old_user_text|-related checks).

Done.

[Peter Kasting, 2011-01-25 04:25:48]

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6281011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:668: if (text_differs &&
allow_keyword_ui_change && !just_deleted_text &&
On 2011/01/25 02:52:48, James Su wrote:
> On 2011/01/25 02:12:22, Peter Kasting wrote:
> > Nit: Even more condensed:
> > 
> >   return !text_differs || !allow_keyword_ui_change || just_deleted_text ||
> >       !MaybeAcceptKeywordBySpace(old_user_text, user_text_);
> I'd prefer to keep the old one, which is more readable to me.

Then how about this:

  return !(text_differs && allow_keyword_ui_change && !just_deleted_text &&
      MaybeAcceptKeywordBySpace(old_user_text, user_text_));

I really don't like "if (x) return true; return false;"-type blocks.