Omnibox: SearchProvider: Fix Navsuggestions in Prevent-Inlining Mode

Omnibox: SearchProvider: Fix Navsuggestions in Prevent-Inlining Mode

Precisely, navsuggestions that look like a user's input (trailing slash or not, omitted scheme/droppable prefix)--are effectively a what-you-typed suggestion--sometimes were previously erroneously marked as "not allowed to be the default match" when the user is backspacing (i.e., prevent_inline_autocompletion mode).

This causes some wonkiness, especially now that reorder mode is default.  See listed bug.

This fixes the trouble and added/corrects tests.

BUG=344585
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252486

[Mark P, 2014-02-19 00:26:19]

Peter,

Can you review this ASAP?  I think I want to request it gets merged into Beta.

thanks,
mark

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

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1514: // Actual input to use when
matching, discarding trailing slash if any.
This helps handle cases like input doodle.com/ with a navsuggestion formatted
URL of doodle.com

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1549: if
(inline_autocomplete_offset != base::string16::npos) {
The restructuring in this block allows handling simple cases such as input
doodle.com with navsuggestion formatted_url doodle.com (identical).  (This was
broken before.)  The new structure also plays nicely with "/"-handling code
above, so fixes those cases too.

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider_unittest.cc (left):

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3035: { "abc.com/",     
       "http://www.abc.com",
This seems okay to inline; removing this test case here.  Added a similar test
case below that checks that it can inline.

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider_unittest.cc:3131: { "ftp://www.ab", 
  "ftp://www.abc.com/path/file.htm?q=x#foo",
I didn't actually change this block, only the spacing.

[Peter Kasting, 2014-02-19 20:39:31]

This feels really fragile.

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

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1514: // Actual input to use when
matching, discarding trailing slash if any.
On 2014/02/19 00:26:19, Mark P wrote:
> This helps handle cases like input doodle.com/ with a navsuggestion formatted
> URL of http://doodle.com

What about e.g. matches where the "www." is only present on one side, or matches
with escaping or punycode/Unicode differences?

I'm worried that if we want to check for "equivalent URLs", we need something
smarter than this.

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1561:
match.inline_autocompletion.empty());
Does this mean that an input of "doodle.com" won't allow a navsuggestion of
"doodle.com/" to be default, because we'd have a slash as the inline
autocompletion?

[Mark P, 2014-02-19 23:23:01]

I need to think more about your comments and the alternates I raised.  In the
meantime, I'll send out my comments in hopes they (and the alternates and nuance
they add) will spur additional comments by you to help me clarify my thinking.

--mark

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

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1514: // Actual input to use when
matching, discarding trailing slash if any.
On 2014/02/19 20:39:32, Peter Kasting wrote:
> On 2014/02/19 00:26:19, Mark P wrote:
> > This helps handle cases like input doodle.com/ with a navsuggestion
formatted
> > URL of http://doodle.com
> 
> What about e.g. matches where the "www." is only present on one side, or
matches
> with escaping or punycode/Unicode differences?

The code (below) uses BestUrlPrefix, exactly like all the other providers, to
decide what is inlineable.  This means for instance if the user types doodle.com
and the navsuggestion URL is http://www.doodle.com, that will be identified as
inlineable.  Likewise, if the user types www.doodle.com, it will also be.  But
if the user types www.doodle.com and the navsuggestion URL is merely
http://doodle.com/, it cannot be identified as inlineable.  All this is like
HistoryURL (and other) providers.

> I'm worried that if we want to check for "equivalent URLs", we need something
> smarter than this.

I think we do this equivalent URLs check right enough (see above).  The messed
up time is fixed by this '/' handling is to better deal with (unnecessary)
trailing slashes.

I find it odd that we need this code.  HistoryURL provider turns out not to need
it because it returns results based on matching against the URL (not the URL
with the trimmed slash).  HistoryQuick provider turns out not to need it because
it does substring matching against the URL (not the URL with the trimmed slash).

This makes me think of an alternate implementation.  For the purpose of
calculating allowed_to_be_default_match, rather than trimming the optional
trailing slash on the input and matching it against formatted_url (which lacks
the trailing slash and is used to calculate match_conents), leave the input
as-is and match it against a (lightly-formatted) URL, one that keeps the
trailing slash.  Would you prefer this approach?

Note that this approach might be slightly more complex because we'd still want
to compute inline_autocomplete_offset and inline_autocompletion using
formatted_url, just not allowed_to_be_default_match.  (In other words, we'd do
some of the code below twice, once with each version of the URL.)  Obviously I
can add helper functions add needed.

https://codereview.chromium.org/171753002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:1561:
match.inline_autocompletion.empty());
On 2014/02/19 20:39:32, Peter Kasting wrote:
> Does this mean that an input of "doodle.com" won't allow a navsuggestion of
> "doodle.com/" to be default, because we'd have a slash as the inline
> autocompletion?

There's some subtlety here.
Technically, you're right: in prevent_inline_autocomplete mode, then doodle.com
will not allow a navsuggestion of doodle.com/ to be marked as inlineable.  But
note that because right now we're using formatted_url (which is always computed
in a way to drop the trailing slash on hostnames) in order to compute
formatted_url and inline_autocompletion, then this will never happen.

There might be some concern about an input doodle.com/abc and the navsuggestion
doodle.com/abc/ not being marked as inlineable.  That might be another issue
worth talking about (outside the scope of this which clearly needs to be fixed
ASAP).


Also note, I just realized HistoryQuick provider is already bad on this front:
doodle.com against the URL suggestion doodle.com/ will not be marked as
inlineable in prevent_inline_autocomplete mode.  (HQP uses the URL with the
slash for this decision.)  This sounds like another issue.  I'll file a bug.

[Peter Kasting, 2014-02-20 01:11:55]

On 2014/02/19 23:23:01, Mark P wrote:
> This makes me think of an alternate implementation.  For the purpose of
> calculating allowed_to_be_default_match, rather than trimming the optional
> trailing slash on the input and matching it against formatted_url (which lacks
> the trailing slash and is used to calculate match_conents), leave the input
> as-is and match it against a (lightly-formatted) URL, one that keeps the
> trailing slash.  Would you prefer this approach?

Yeah, this sort of idea seems probably better.  Again, I'd worry about anything
that could change about the URL during formatting, whether that be escaping,
Unicode conversion, etc.  (I'd also worry about those things if they COULDN'T
change during formatting, to ensure that we could still mark-as-default any
navigation that was "equivalent to" the user's input.  Maybe we should try and
fixup both input _and_ navigation and then see if they turned out the same?)

[Mark P, 2014-02-20 20:30:46]

I like this new approach a lot more.  I think you will too.

--mark

[Peter Kasting, 2014-02-20 22:55:31]

LGTM

https://codereview.chromium.org/171753002/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/171753002/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/base_search_provider.cc:212: size_t match_start =
(prefix == NULL) ? formatted_url_.find(input_text)
Nit: While here: Wrap after '?' and not before ':'

[Mark P, 2014-02-20 23:20:39]

https://codereview.chromium.org/171753002/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/171753002/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/base_search_provider.cc:212: size_t match_start =
(prefix == NULL) ? formatted_url_.find(input_text)
On 2014/02/20 22:55:32, Peter Kasting wrote:
> Nit: While here: Wrap after '?' and not before ':'

Done.

[Mark P, 2014-02-21 01:49:01]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/171753002/240001

[commit-bot: I haz the power, 2014-02-21 03:48:08]

Change committed as 252486