Fix DCHECK with Trailing Slash.

Fix DCHECK with Trailing Slash.

A DCHECK was being hit when a full URL with a scheme was entered and the user typed a slash. The slash was being removed by FormatURLWithOffsets but the inline_autocomplete_offset was not being adjusted to account for the removed slash.

BUG=112226
TEST=Ran unit tests.
TBR=pkasting@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120080

[mrossetti, 2012-02-01 03:43:50]

This will need to be merged.

[Peter Kasting, 2012-02-02 06:35:14]

https://chromiumcodereview.appspot.com/9307027/diff/1/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://chromiumcodereview.appspot.com/9307027/diff/1/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider.cc:146: // by the
FormatURLWithOffsets call above.
We should not be stripping slashes that the user typed.  If that's what's
happening, this isn't the right fix.

How does the HUP handle this scenario?

[mrossetti, 2012-02-02 20:03:36]

On 2012/02/02 06:35:14, Peter Kasting wrote:
>
https://chromiumcodereview.appspot.com/9307027/diff/1/chrome/browser/autocomp...
> File chrome/browser/autocomplete/history_quick_provider.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9307027/diff/1/chrome/browser/autocomp...
> chrome/browser/autocomplete/history_quick_provider.cc:146: // by the
> FormatURLWithOffsets call above.
> We should not be stripping slashes that the user typed.  If that's what's
> happening, this isn't the right fix.
> 
> How does the HUP handle this scenario?

With my debug harness in place on both the HQP and the HUP, the results of
FormatURL is identical in both. That is, the trailing slash is removed for
fill_into_edit.

Here are debug output from each during a live session:

User types: 'http://www.teamliquid.net/'
  Candidate destination URL: 'http://www.teamliquid.net/'
    HUP fill_into_edit: 'http://www.teamliquid.net'
    HUP inline_autocomplete_offset: 25
    HQP fill_into_edit: 'http://www.teamliquid.net'
    HQP inline_autocomplete_offset: 25
  Candidate destination URL: 'http://www.teamliquid.net/tlpd'
    HUP fill_into_edit: 'http://www.teamliquid.net/tlpd'
    HUP inline_autocomplete_offset: 26
    HQP fill_into_edit: 'http://www.teamliquid.net/tlpd'
    HQP inline_autocomplete_offset: 26

User types: 'tea'
  Candidate destination URL: 'http://www.teamliquid.net/'
    HUP fill_into_edit: 'http://www.teamliquid.net'
    HUP inline_autocomplete_offset: 7
    HQP fill_into_edit: 'http://www.teamliquid.net'
    HQP inline_autocomplete_offset: 7
  Candidate destination URL: 'http://www.teamliquid.net/tlpd'
    HUP fill_into_edit: 'http://www.teamliquid.net/tlpd'
    HUP inline_autocomplete_offset: 7
    HQP fill_into_edit: 'http://www.teamliquid.net/tlpd'
    HQP inline_autocomplete_offset: 7

The HUP handles an exact match using a different flow of control than all other
HUP suggestions: via HistoryURLProvider::SuggestExactInput. SuggestExactInput
does not set inline_autocomplete_offset, leaving it npos, which, according to
the header comment for inline_autocomplete_offset, indicates the match "should
not be autocompleted." There is also a comment in SuggestExactInput:

    // NOTE: Don't set match.input_location (to allow inline autocompletion)
    // here, it's surprising and annoying.

The comment doesn't make sense because there is no such field in
AutocompleteMatch. (There is such a field in HistoryMatch but this function does
not reference a HistoryMatch.) It's possible the comment should have said
'inline_autocomplete_offset' instead of 'match.input_location' but even so there
is nothing to explain what "surprising and annoying" means.

Clearly, there is something up-stream that knows what to do with this top-level,
autocompletable suggestion. I thought URL_WHAT_YOU_TYPED might be a trigger but
there seems to be nothing in the code that actually uses URL_WHAT_YOU_TYPED!

Then I tracked down is_history_what_you_typed_match. (Interesting that we're
inserting provider-specific knowledge into AutocompleteMatch.) I followed
is_history_what_you_typed_match around the dark, dusty corridors and thought I'd
discovered how an npos'ed suggestion was converted into an autocompleteable one
in FixupExactSuggestion. I could not, however, find anywhere that
inline_autocomplete_offset was being changed to indicate the suggestion could be
inlined.

But then I discovered EnsureMatchPresent and made the vital connection. This
function creates a new HistoryMatch with an input_location taken from the
HistoryMatches for the URL! This new match, if it is to be promoted, at the head
of the list and eventually becomes an inline-able AutocompleteMatch once again.
From an old, dying AutocompleteMatch springs a brand new, inline-able
AutocompleteMatch! Mystery solved!

Bottom line: The HQP uses a much different approach for identifying inline-able
suggestions.

[Peter Kasting, 2012-02-03 21:53:24]

On 2012/02/02 20:03:36, mrossetti wrote:
> The HUP handles an exact match using a different flow of control than all
other
> HUP suggestions: via HistoryURLProvider::SuggestExactInput. SuggestExactInput
> does not set inline_autocomplete_offset, leaving it npos, which, according to
> the header comment for inline_autocomplete_offset, indicates the match "should
> not be autocompleted." There is also a comment in SuggestExactInput:
> 
>     // NOTE: Don't set match.input_location (to allow inline autocompletion)
>     // here, it's surprising and annoying.
> 
> The comment doesn't make sense because there is no such field in
> AutocompleteMatch. (There is such a field in HistoryMatch but this function
does
> not reference a HistoryMatch.) It's possible the comment should have said
> 'inline_autocomplete_offset' instead of 'match.input_location' but even so
there
> is nothing to explain what "surprising and annoying" means.

Digging into the pre-public-launch history of this file, "input_location" was
the old name for "inline_autocomplete_offset".

The "surprising and annoying" bit dates from very early days.  It was originally
referring to the possibility of autocompleting ".com" when the user pressed the
<ctrl> key and the best result became the "ctrl-enter" result of "user typing +
www. and .com".  I think in a subsequent version it expanded to include not
autocompleting a trailing '/' after a completely-typed hostname, which felt
weird in actual use, like you had finished typing but Chrome needed to let you
know your input wasn't good enough or something.

Since then we implemented trailing slash stripping, which does away with the
autocompleted '/' in most cases, but it seems like that would still be an issue
for intranet hostnames.

> Then I tracked down is_history_what_you_typed_match.

I forgot about this bit.  There are important things this does, such as affect
the scoring for pasted URLs.  You should make sure that the HQP sets this for
any matches that are equivalent to "what the user typed" directly, if you create
such.

> But then I discovered EnsureMatchPresent and made the vital connection. This
> function creates a new HistoryMatch with an input_location taken from the
> HistoryMatches for the URL!

I thought FixupExactSuggestion() explicitly passed npos for the
|input_location|?

Furthermore I'm wondering how HistoryMatchToACMatch() manages to handle
|input_location| being npos correctly, when it looks like it just blindly adds
it to the input text length without testing...

[mrossetti, 2012-02-07 02:27:58]

On 2012/02/03 21:53:24, Peter Kasting wrote:
> Since then we implemented trailing slash stripping, which does away with the
> autocompleted '/' in most cases, but it seems like that would still be an
issue
> for intranet hostnames.

I'll check on that.

> > Then I tracked down is_history_what_you_typed_match.
> 
> I forgot about this bit.  There are important things this does, such as affect
> the scoring for pasted URLs.  You should make sure that the HQP sets this for
> any matches that are equivalent to "what the user typed" directly, if you
create
> such.

I will do so, but I'm starting to think that the scoring algorithms ought to be
abstracted out of the HUP and made common to any provider that wants to properly
score. Those would be the HUP and the HQP I suppose. That looks like a several
week project. Do you have an opinion?

> > But then I discovered EnsureMatchPresent and made the vital connection. This
> > function creates a new HistoryMatch with an input_location taken from the
> > HistoryMatches for the URL!
> 
> I thought FixupExactSuggestion() explicitly passed npos for the
> |input_location|?

Yes, your are right!

> Furthermore I'm wondering how HistoryMatchToACMatch() manages to handle
> |input_location| being npos correctly, when it looks like it just blindly adds
> it to the input text length without testing...

It does seem to do so. When I get some time in the next couple of weeks I'll
work through this a bit more thoroughly and try to figure things out.

In the meantime, at a minimum, the change from this CL needs to be merged into
the M18 branch. Do I have your approval to do so?

[Peter Kasting, 2012-02-07 02:30:42]

On 2012/02/07 02:27:58, mrossetti wrote:
> In the meantime, at a minimum, the change from this CL needs to be merged into
> the M18 branch. Do I have your approval to do so?

I guess.  I'm uneasy about all this stuff but it's not the highest-priority bug
on the stack to dig into further.