Fix keyword recognition.

Fix keyword recognition.

The InMemoryURLIndex was not properly recognizing that a search string with a trailing space was not inline-able. The test for determingin inline-ability has been made more robust.

BUG=110428
TEST=Type 'http' and then <space>. If this doesn't offer a [Search Google] then things are working properly. Also enhanced the InMemoryURLIndexTest.Retrieval unit test.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119338

[mrossetti, 2012-01-26 00:47:39]

Peter, I'm hoping you can recommend several additional manual tests to prove
that this is a righteous fix. Please advise.

[Peter Kasting, 2012-01-26 01:59:22]

I'm confused.  I've tried stepping through this on my own profile (where the
problem doesn't happen) and it seems like a prerequisite of
MaybeAcceptKeywordBySpace() behavior badly is for the UpdatePopup() call in
OnAfterPossibleChange() to result in a top result that has a keyword hint.  But
if the user has typed e.g. "ama ", I don't understand why the top result would
have a keyword hint, because the top result there should be the Search What You
Typed match.

Are you saying that the HQP is returning "amazon.com" as the top result for "ama
" in addition to returning it for "ama"?  That seems bad if so.

(BTW, this code is confusing as all hell.  You should get suzhe to look as well
since he wrote it.  Every time the two of us have tried to make changes here
I've been endlessly confused.)

[mrossetti, 2012-01-26 16:19:23]

On 2012/01/26 01:59:22, Peter Kasting wrote:
> Are you saying that the HQP is returning "amazon.com" as the top result for
"ama
> " in addition to returning it for "ama"?  That seems bad if so.

Peter, you are brilliant! That's almost _exactly_ what's going on but close
enough that it points to the solution. The HQP breaks down search strings on
whitespace. "ama " is an edge case where when broken up whitespace into separate
terms we get just a single term and the meaning of the trailing space is lost. I
know the proper fix now—in this edge case there really are two terms, one
happens to be empty, but it should eliminate the proffered result from
inline-able consideration.

[Peter Kasting, 2012-01-26 18:56:33]

I don't know your code well enough to tell you otherwise, but the "two terms"
thing sounds a bit weird -- it seems like the simpler check for inline
autocompletion would be, once we have the ranked results, to do a direct string
compare of the original, un-tokenized user input against the beginning of the
best result (accounting for prefixes like "http://www." and such, of course). 
If it's a pure prefix, we can inline autocomplete, otherwise no.

In any case, while debugging this I found some other issues, and I'm not
convinced the patch above is completely wrong, just maybe not the right fix for
this particular bug.

[mrossetti, 2012-01-26 20:40:08]

Yes, I agree that the HQP inline test was missing a check that would catch
this case (and for this specific bug I've got a fix and am updating the
unit tests). I also agree that at some point (when we figure out how to do
multi-term inline formatting properly the autocomplete edit controller's
approach will have to change (but I'm backing out that change for now).

On Thu, Jan 26, 2012 at 11:56 AM, <pkasting@chromium.org> wrote:

> I don't know your code well enough to tell you otherwise, but the "two
> terms"
> thing sounds a bit weird -- it seems like the simpler check for inline
> autocompletion would be, once we have the ranked results, to do a direct
> string
> compare of the original, un-tokenized user input against the beginning of
> the
> best result (accounting for prefixes like "http://www." and such, of
> course).
> If it's a pure prefix, we can inline autocomplete, otherwise no.
>
> In any case, while debugging this I found some other issues, and I'm not
> convinced the patch above is completely wrong, just maybe not the right
> fix for
> this particular bug.
>
>
https://chromiumcodereview.**appspot.com/9289034/<https://chromiumcodereview....
>

[mrossetti, 2012-01-26 21:30:28]

Please take a look at this chance. It's more appropriate.

[mrossetti, 2012-01-26 21:31:04]

On 2012/01/26 21:30:28, mrossetti wrote:
> Please take a look at this chance. It's more appropriate.

s/chance/change/ dangit

[Peter Kasting, 2012-01-26 22:19:25]

https://chromiumcodereview.appspot.com/9289034/diff/2004/chrome/browser/histo...
File chrome/browser/history/url_index_private_data.cc (right):

https://chromiumcodereview.appspot.com/9289034/diff/2004/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:534: //  2) there is one match
within the URL, AND
Shouldn't we allow multiple URL matches for our one term?  I mean, if I type
"foo", it should be able to match "foofoofoo.com".  Or will that not be rejected
by this?

https://chromiumcodereview.appspot.com/9289034/diff/2004/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:535: //  2) it starts at the
beginning of the candidate URL, OR
Nit: To make clear how tightly the ANDs and ORd on these lines bind, sub-indent
some of these items as "2a" and "2b" or whatever

[mrossetti, 2012-01-26 22:58:55]

Searching for 'foo' will now find, inline, and highlight properly in
'foofoofoo.com'.

https://chromiumcodereview.appspot.com/9289034/diff/2004/chrome/browser/histo...
File chrome/browser/history/url_index_private_data.cc (right):

https://chromiumcodereview.appspot.com/9289034/diff/2004/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:534: //  2) there is one match
within the URL, AND
On 2012/01/26 22:19:25, Peter Kasting wrote:
> Shouldn't we allow multiple URL matches for our one term?  I mean, if I type
> "foo", it should be able to match "foofoofoo.com".  Or will that not be
rejected
> by this?

Done.

https://chromiumcodereview.appspot.com/9289034/diff/2004/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:535: //  2) it starts at the
beginning of the candidate URL, OR
On 2012/01/26 22:19:25, Peter Kasting wrote:
> Nit: To make clear how tightly the ANDs and ORd on these lines bind,
sub-indent
> some of these items as "2a" and "2b" or whatever

Done.

[Peter Kasting, 2012-01-26 23:04:30]

LGTM

https://chromiumcodereview.appspot.com/9289034/diff/6003/chrome/browser/histo...
File chrome/browser/history/url_index_private_data.cc (right):

https://chromiumcodereview.appspot.com/9289034/diff/6003/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:538: //  4) AND the search
string does not end in whitespace (making it look to
Nit: 4 -> 3

https://chromiumcodereview.appspot.com/9289034/diff/6003/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:541: match.can_inline =
match.url_matches.size() > 0 &&
Nit: !empty()

[mrossetti, 2012-01-26 23:09:36]

Thanks for the review. Will commit once bots are happy.

https://chromiumcodereview.appspot.com/9289034/diff/6003/chrome/browser/histo...
File chrome/browser/history/url_index_private_data.cc (right):

https://chromiumcodereview.appspot.com/9289034/diff/6003/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:538: //  4) AND the search
string does not end in whitespace (making it look to
On 2012/01/26 23:04:30, Peter Kasting wrote:
> Nit: 4 -> 3

Done.

https://chromiumcodereview.appspot.com/9289034/diff/6003/chrome/browser/histo...
chrome/browser/history/url_index_private_data.cc:541: match.can_inline =
match.url_matches.size() > 0 &&
On 2012/01/26 23:04:30, Peter Kasting wrote:
> Nit: !empty()

Done.