Ctrl-based actions should take precedence over ctrl-enter

Ctrl-based actions should take precedence over ctrl-enter

When the user presses ctrl, prefer (keeping) inline autocomplete
matches to (new, surprising) exact matches that include the
desired TLD.

BUG=52566
TEST=none

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

[Ilya Sherman, 2010-08-18 09:49:48]

The interaction between ctrl-enter and ctrl-based text actions is kinda hairy --
hopefully this approach is along the right lines.  At least, I haven't yet come
up with crazy edge cases where this would completely break...

Also, there will probably need to be some OS-specific follow-ups to this bug. 
See for example crbug.com/52565

[Peter Kasting, 2010-08-18 20:04:36]

I think there's a simpler conceptual solution here.

Ctrl-enter is supposed to act on the visible text (which includes the inline
autocompletion).  So we should make AutocompleteEditModel::GetDesiredTLD() only
return a non-empty string when, given display_text = user_text_ +
inline_autocomplete_text_, the following both hold:

* display_text == DisplayTextFromUserText(display_text) [or
DisplayTextFromUserText() could be refactored to split the conditional inside it
out, and we could call that directly]

* URLFixerUpper::FixupURL(display_text, std::wstring()) !=
URLFixerUpper::FixupURL(display_text, "com")

I think this should handle all problematic cases.

http://codereview.chromium.org/3152029/diff/2001/3001
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/3152029/diff/2001/3001#newcode911
chrome/browser/autocomplete/autocomplete.cc:911: if (updated_matches || done_) {
Nit: {} can be removed

[Ilya Sherman, 2010-08-20 09:53:45]

On 2010/08/18 20:04:36, Peter Kasting wrote:
> I think there's a simpler conceptual solution here.
> 
> Ctrl-enter is supposed to act on the visible text (which includes the inline
> autocompletion).  So we should make AutocompleteEditModel::GetDesiredTLD()
only
> return a non-empty string when, given display_text = user_text_ +
> inline_autocomplete_text_, the following both hold:
> 
> * display_text == DisplayTextFromUserText(display_text) [or
> DisplayTextFromUserText() could be refactored to split the conditional inside
it
> out, and we could call that directly]
> 
> * URLFixerUpper::FixupURL(display_text, std::wstring()) !=
> URLFixerUpper::FixupURL(display_text, "com")
> 
> I think this should handle all problematic cases.

This doesn't quite work:  If the user has typed "foo" and currently has an
inline autocomplete suggestion of "foobar"; but has also visited "www.foo.com",
then the revised GetDesiredTLD() logic will still return "com" as the desired
TLD.  Thus, pressing ctrl will again make the autocomplete suggestion disappear.
 This case admittedly seems fantastically unlikely, but we should probably get
it right anyway.

[Ilya Sherman, 2010-08-20 10:01:58]

On 2010/08/20 09:53:45, Ilya Sherman wrote:
> On 2010/08/18 20:04:36, Peter Kasting wrote:
> > I think there's a simpler conceptual solution here.
> > 
> > Ctrl-enter is supposed to act on the visible text (which includes the inline
> > autocompletion).  So we should make AutocompleteEditModel::GetDesiredTLD()
> only
> > return a non-empty string when, given display_text = user_text_ +
> > inline_autocomplete_text_, the following both hold:
> > 
> > * display_text == DisplayTextFromUserText(display_text) [or
> > DisplayTextFromUserText() could be refactored to split the conditional
inside
> it
> > out, and we could call that directly]
> > 
> > * URLFixerUpper::FixupURL(display_text, std::wstring()) !=
> > URLFixerUpper::FixupURL(display_text, "com")
> > 
> > I think this should handle all problematic cases.
> 
> This doesn't quite work:  If the user has typed "foo" and currently has an
> inline autocomplete suggestion of "foobar"; but has also visited
"www.foo.com",
> then the revised GetDesiredTLD() logic will still return "com" as the desired
> TLD.  Thus, pressing ctrl will again make the autocomplete suggestion
disappear.
>  This case admittedly seems fantastically unlikely, but we should probably get
> it right anyway.

I meant to also say: It seems like if ctrl-enter should act on the entire
visible text, then we should modify AutocompleteEditModel::StartAutocomplete()
so that the first argument to popup_->StartAutocomplete includes the entire
visible text in the case that GetDesiredTLD() returns "com" rather than the
empty string.

This exact change doesn't work though -- it affects more than just ctrl-enter
behavior; and even for ctrl-enter, it makes it seem like we're in "tricky corner
case #1" when we aren't (that is, if the inline autocompletion is an intranet
address).

Maybe there's something similar to these ideas that would work here, but I
haven't come up with anything yet that would cover all the cases.

[Peter Kasting, 2010-08-20 19:06:26]

On 2010/08/20 09:53:45, Ilya Sherman wrote:
> If the user has typed "foo" and currently has an
> inline autocomplete suggestion of "foobar"; but has also visited
"www.foo.com",
> then the revised GetDesiredTLD() logic will still return "com" as the desired
> TLD.  Thus, pressing ctrl will again make the autocomplete suggestion
disappear.

Not true, because the inline autocompletion won't be "foobar", it will be
"foobar/", which can't have a .com suffix added :)

This is admittedly a bit fragile... but it's better than any other solution I
can think of.

[Ilya Sherman, 2010-08-20 20:33:14]

On 2010/08/20 19:06:26, Peter Kasting wrote:
> On 2010/08/20 09:53:45, Ilya Sherman wrote:
> > If the user has typed "foo" and currently has an
> > inline autocomplete suggestion of "foobar"; but has also visited
> "www.foo.com",
> > then the revised GetDesiredTLD() logic will still return "com" as the
desired
> > TLD.  Thus, pressing ctrl will again make the autocomplete suggestion
> disappear.
> 
> Not true, because the inline autocompletion won't be "foobar", it will be
> "foobar/", which can't have a .com suffix added :)
> 
> This is admittedly a bit fragile... but it's better than any other solution I
> can think of.

Well, turns out URLFixerUpper::FixupURL() tries to be smart and insert the TLD
before any path; so it does think that "foobar/" can have the ".com" suffix
added.

This does give me an idea though: can we just check

   !inline_autocomplete_text_.empty()

instead of 

  URLFixerUpper::FixupURL(display_text, std::wstring()) !=
  URLFixerUpper::FixupURL(display_text, "com")

?  Are there any cases where we are providing an inline autocompletion for which
it makes sense to press ctrl-enter?

[Peter Kasting, 2010-08-20 20:38:32]

On 2010/08/20 20:33:14, Ilya Sherman wrote:
> ?  Are there any cases where we are providing an inline autocompletion for
which
> it makes sense to press ctrl-enter?

Whether or not there are, if we allow any, then for those cases ctrl-a will be
screwed up.  Therefore, I say "no".

[Peter Kasting, 2010-08-24 23:59:04]

http://codereview.chromium.org/3152029/diff/16002/19003
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/3152029/diff/16002/19003#newcode174
chrome/browser/autocomplete/autocomplete_edit.cc:174: // autocomplete suggestion
of "foo.net".  She now presses ctrl-a (e.g. to
Nit: "He" is the correct neuter pronoun, if you must use one.

http://codereview.chromium.org/3152029/diff/16002/19003#newcode723
chrome/browser/autocomplete/autocomplete_edit.cc:723: return ((keyword_ui_state_
!= NO_KEYWORD) &&
Nit: Put as many conditions on one line as possible.

http://codereview.chromium.org/3152029/diff/16002/19003#newcode730
chrome/browser/autocomplete/autocomplete_edit.cc:730: return
(!KeywordIsSelected()) ?
Nit: Remove parens, remove ! and reverse subsequent values (2 places)

[Ilya Sherman, 2010-08-25 00:31:14]

cleaned up revision uploaded

[Peter Kasting, 2010-08-25 01:35:07]

LGTM

http://codereview.chromium.org/3152029/diff/21001/19009
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/3152029/diff/21001/19009#newcode187
chrome/browser/autocomplete/autocomplete_edit.cc:187: !KeywordIsSelected())?
Nit: Put as many conditions on one line as possible.

http://codereview.chromium.org/3152029/diff/21001/19009#newcode735
chrome/browser/autocomplete/autocomplete_edit.cc:735: return KeywordIsSelected()
?
Nit: This will all fit on one line.

[mrossetti, 2010-08-25 16:16:48]

LGTM