Updated and refactored URLIndexPrivateData::HistoryItemsForTerms to handle searching the history fo…

Updated and refactored URLIndexPrivateData::HistoryItemsForTerms to handle searching the history for both the original search string as well as the string with a break inserted.

The goal is to support mid-word autocomplete in the Omnibox.  Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only.  We want to also search for "fungoodtimes".

BUG=591979
TEST=0. Clear browser history.
1. Visit the following link: https://twitter.com/fungoodtimes
2. Open a new browser tab.
3.Type into the Omnibox "funtimes".  Note the lack of the suggestion for the above URL.
4. Insert the cursor between the "n" and "t" in "funtime" and type "good".
5. The above URL should show in the autocomplete list.

Committed: https://crrev.com/eccdb2b8454318889abf958d86fbf668e2908cc6
Cr-Commit-Position: refs/heads/master@{#420721}

[Lavar Askew, 2016-09-23 02:22:02]

open.hyperion@gmail.com changed reviewers:
+ mpearson@chromium.org

[Lavar Askew, 2016-09-23 02:22:02]

Hi Mark,

The proper change set has been uploaded.

Thanks,

Lavar

[Mark P, 2016-09-23 04:04:39]

looks good to me

Thanks for polishing this.

I'll go ahead and click the submit button for you.

--mark

[Mark P, 2016-09-23 04:04:45]

lgtm

[Mark P, 2016-09-23 04:04:50]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2016-09-23 04:05:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 04:18:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 04:18:09]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mark P, 2016-09-23 17:24:13]

On 2016/09/23 04:18:09, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

FYI, this looks like a legitimate failure.  One test in components_unittests is
failing on every platform:

InMemoryURLIndexTest.TypedCharacterCaching: 
../../components/omnibox/browser/in_memory_url_index_unittest.cc:809: Failure
Value of: cache.size()
  Actual: 0
Expected: 1U
Which is: 1

Looking at the test,
https://cs.chromium.org/chromium/src/components/omnibox/browser/in_memory_url...
it's clear we're failing because the cache is cleared at a time when it
shouldn't be.

Looking more at the code, it looks like we messed up original logic for
if (was_trimmed) {
  search_term_cache_.clear();
}
and are clearing the cache more aggressively than before.

We replaced this test with
if (pre_filter_item_count_ > post_filter_item_count_)

One of those variables must be wrong.

Aha!

We only increment post_filter_item_count_ inside a
if (pre_filter_item_count_ > kItemsToScoreLimit) { }
block.

This means post_filter_item_count_ is only incremented if we hit the items to
score limit.  Otherwise it always stays 0.  Oops.  If we didn't hit that limit,
post_filter_item_count_ should be incremented by same amount as
pre_filter_item_count_ because we're not actually filtering anything.

(Oddly, it looks like the old code was wrong in this case--not setting
post_filter_item_count_ correctly--but that didn't matter because we didn't use
post_filter_item_count_ in the was_trimmed test (or pretty much anywhere else
for that matter).)

--mark

P.S.  It's always nice when a unit test actually catches a real bug! :-)


Can you revise the code and upload a new patchset please?

[Lavar Askew, 2016-09-23 17:52:20]

Done.

[Mark P, 2016-09-23 18:02:13]

https://codereview.chromium.org/2364553004/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_index_private_data.cc (right):

https://codereview.chromium.org/2364553004/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:228:
history_id_set.clear();
Should we be clearing post_filter_item_count_ here?
After all, if we're on the second pass through the list of search strings, the
history_id_set will include both the first pass and second pass items that we
then filter.  Incrementing post_filter_item_count_ to the size of the final
filtered set would end up double-counting the first pass's items.

[Lavar Askew, 2016-09-23 19:07:48]

https://codereview.chromium.org/2364553004/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_index_private_data.cc (right):

https://codereview.chromium.org/2364553004/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:228:
history_id_set.clear();
On 2016/09/23 18:02:13, Mark P wrote:
> Should we be clearing post_filter_item_count_ here?
> After all, if we're on the second pass through the list of search strings, the
> history_id_set will include both the first pass and second pass items that we
> then filter.  Incrementing post_filter_item_count_ to the size of the final
> filtered set would end up double-counting the first pass's items.

I disagree because history_id_set is reinitialized each time through the for
loop.

[Mark P, 2016-09-23 19:32:14]

Okay, I'll try to hit the CQ (commit queue) button for you. :-)

--mark

https://codereview.chromium.org/2364553004/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_index_private_data.cc (right):

https://codereview.chromium.org/2364553004/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:228:
history_id_set.clear();
On 2016/09/23 19:07:48, Lavar Askew wrote:
> On 2016/09/23 18:02:13, Mark P wrote:
> > Should we be clearing post_filter_item_count_ here?
> > After all, if we're on the second pass through the list of search strings,
the
> > history_id_set will include both the first pass and second pass items that
we
> > then filter.  Incrementing post_filter_item_count_ to the size of the final
> > filtered set would end up double-counting the first pass's items.
> 
> I disagree because history_id_set is reinitialized each time through the for
> loop.

Ah, good point.

[Mark P, 2016-09-23 19:32:17]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2016-09-23 19:32:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2364553004/#ps20001
(title: "Fixed bug to properly increment post_filter_item_count_.")

[commit-bot: I haz the power, 2016-09-23 19:33:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 20:41:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-23 20:45:25]

Description was changed from

==========
Updated and refactored URLIndexPrivateData::HistoryItemsForTerms to handle
searching the history for both the original search string as well as the string
with a break inserted.

The goal is to support mid-word autocomplete in the Omnibox.  Currently, if the
user types "funtimes", inserts the cursor between the "n" and the "t" and begins
typing the word "good" the Ominbox will search for URL results that match
"fungood times" only.  We want to also search for "fungoodtimes".

BUG=591979
TEST=0. Clear browser history.
1. Visit the following link: https://twitter.com/fungoodtimes
2. Open a new browser tab.
3.Type into the Omnibox "funtimes".  Note the lack of the suggestion for the
above URL.
4. Insert the cursor between the "n" and "t" in "funtime" and type "good".
5. The above URL should show in the autocomplete list.
==========

to

==========
Updated and refactored URLIndexPrivateData::HistoryItemsForTerms to handle
searching the history for both the original search string as well as the string
with a break inserted.

The goal is to support mid-word autocomplete in the Omnibox.  Currently, if the
user types "funtimes", inserts the cursor between the "n" and the "t" and begins
typing the word "good" the Ominbox will search for URL results that match
"fungood times" only.  We want to also search for "fungoodtimes".

BUG=591979
TEST=0. Clear browser history.
1. Visit the following link: https://twitter.com/fungoodtimes
2. Open a new browser tab.
3.Type into the Omnibox "funtimes".  Note the lack of the suggestion for the
above URL.
4. Insert the cursor between the "n" and "t" in "funtime" and type "good".
5. The above URL should show in the autocomplete list.

Committed: https://crrev.com/eccdb2b8454318889abf958d86fbf668e2908cc6
Cr-Commit-Position: refs/heads/master@{#420721}
==========

[commit-bot: I haz the power, 2016-09-23 20:45:26]

Patchset 2 (id:??) landed as
https://crrev.com/eccdb2b8454318889abf958d86fbf668e2908cc6
Cr-Commit-Position: refs/heads/master@{#420721}