Omnibox SearchProvider Experiment Client Implementation.

Omnibox SearchProvider Experiment Client Implementation.
See http://goto/omnibox-v20-experiment and http://b/6426178

*Accept suggestion and verbatim relevance scores extensions.
*Keep inline autocompletable matches between URL requests.
-Only accept suggested relevances if Instant is disabled.
-Reject scores that make the top match non-inlinable.
-Make BestURLPrefix perform case-insensitive comparison.

TODO: Limit # of suggestions parsed or clamp their scores?

BUG=125871
TEST=SearchProviderTest.SuggestRelevanceExperiment & manual.

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

[Mark P, 2012-05-02 20:03:09]

I'm not that familiar with the search_provider in particular, but everything
that you'd done looks reasonable / looks like you're on the right track.

Minor comment: I find it a little confusing that
CalculateRelevanceForWhatYouTyped() checked the has_verbatim_relevance and uses
it if appropriate whereas CalculateRelevanceForNavigation() and for suggest()
both expect the caller to have looked at the appropriate has_relevance flag
beforehand and only call it if the flag is not true.  This seems inconsistent.

--mark

[msw, 2012-05-03 23:46:23]

This is still a work in progress, but early feedback is appreciated.

[Peter Kasting, 2012-05-04 00:11:33]

Hasty feedback... hope it makes any sense

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete.cc (left):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete.cc:1018: bool notify_default_match =
is_synchronous_pass;
Why wouldn't we want to always notify on the synchronous pass?

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete.cc:722: // TODO(msw): Somehow need to
stop inlining stale data "a[mazon]" -> "ad".
We should never be reaching here with stale data.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:904: // TODO(msw): Run this in
all cases, or fixup default_match_changed?
Shouldn't need to touch this, I wouldn't think.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:389: if
(i->inline_autocomplete_offset != string16::npos) {
I wouldn't try to adjust the offsets directly, I'd just reconstruct the
matches... see below

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:680: // TODO(msw): Merge matches
from_previous = true?
So remember that if you merge previous matches, you need to re-construct them at
least insofar as the inline autocomplete offset goes -- this is maybe why you
have stale autocompletion issues today.  If you're currently storing the full
old AutocompleteMatches, I'd just store the Suggestions (or whatever the data
structures are called) and reconstruct the matches here

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:690: // TODO(msw): Additional
logic to only +1 if we have a what-you-typed?
Yeah

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:834: //           
default_provider_suggest_text_???
It's not at all clear to me how instant is supposed to interact with these
experiments.  You should probably get the suggest team to clarify, maybe for now
they'll just say to turn the experiment off for instant users?

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:836: // TODO(msw): Do not demote
on just_deleted_text_/prevent_inline_autocomplete?
I think your code does this currently and is correct to do so

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/sea...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1095: // Testing
the suggest experiment provider...
Make sure you don't check this in :)

[msw, 2012-05-04 09:43:40]

Thanks for preliminary feedback! It's in better shape, but still a WIP; PTAL.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete.cc (left):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete.cc:1018: bool notify_default_match =
is_synchronous_pass;
On 2012/05/04 00:11:33, Peter Kasting wrote:
> Why wouldn't we want to always notify on the synchronous pass?

Reverted.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete.cc:722: // TODO(msw): Somehow need to
stop inlining stale data "a[mazon]" -> "ad".
On 2012/05/04 00:11:33, Peter Kasting wrote:
> We should never be reaching here with stale data.

It's no longer 'stale'; but see CopyOldMatches for |!done_| synchronous passes.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:904: // TODO(msw): Run this in
all cases, or fixup default_match_changed?
On 2012/05/04 00:11:33, Peter Kasting wrote:
> Shouldn't need to touch this, I wouldn't think.

Reverted.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:389: if
(i->inline_autocomplete_offset != string16::npos) {
On 2012/05/04 00:11:33, Peter Kasting wrote:
> I wouldn't try to adjust the offsets directly, I'd just reconstruct the
> matches... see below

Done; but I still may need to update the results synchronously...

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:680: // TODO(msw): Merge matches
from_previous = true?
On 2012/05/04 00:11:33, Peter Kasting wrote:
> So remember that if you merge previous matches, you need to re-construct them
at
> least insofar as the inline autocomplete offset goes -- this is maybe why you
> have stale autocompletion issues today.  If you're currently storing the full
> old AutocompleteMatches, I'd just store the Suggestions (or whatever the data
> structures are called) and reconstruct the matches here

Done.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:690: // TODO(msw): Additional
logic to only +1 if we have a what-you-typed?
On 2012/05/04 00:11:33, Peter Kasting wrote:
> Yeah

Done.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:834: //           
default_provider_suggest_text_???
On 2012/05/04 00:11:33, Peter Kasting wrote:
> It's not at all clear to me how instant is supposed to interact with these
> experiments.  You should probably get the suggest team to clarify, maybe for
now
> they'll just say to turn the experiment off for instant users?

I have a TODO on the CR & in the CL, will e-mail/ask tomorrow.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:836: // TODO(msw): Do not demote
on just_deleted_text_/prevent_inline_autocomplete?
On 2012/05/04 00:11:33, Peter Kasting wrote:
> I think your code does this currently and is correct to do so

Cool, thanks for the confirmation.

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/12001/chrome/browser/sea...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1095: // Testing
the suggest experiment provider...
On 2012/05/04 00:11:33, Peter Kasting wrote:
> Make sure you don't check this in :)

Done.

[Mark P, 2012-05-04 20:31:37]

I did a quick drive-by looking for odd things.  Only found one.

https://chromiumcodereview.appspot.com/10274023/diff/16002/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/16002/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:645: const size_t
max_total_matches = kMaxMatches + (add_verbatim_result ? 0 : 1);
Are the 1s and 0s backward here?

[msw, 2012-05-04 22:13:39]

Still a bunch to do :(

https://chromiumcodereview.appspot.com/10274023/diff/16002/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/16002/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:645: const size_t
max_total_matches = kMaxMatches + (add_verbatim_result ? 0 : 1);
On 2012/05/04 20:31:38, Mark P wrote:
> Are the 1s and 0s backward here?

Done.

[msw, 2012-06-04 23:25:24]

Hey Peter, can you please take a look ASAP? Thanks!
Mark, I'd appreciate another set of eyes if you have time.

[Peter Kasting, 2012-06-05 00:47:50]

LGTM.  Nice test suite!  Meant I didn't have to worry about checking every fine
detail of the logic :)

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete.h (right):

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete.h:177: // used in place of client-side
calculated values. See http://crbug.com/125871
Nit: I'd just omit the bug reference

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:552: i =
(i->suggestion().find(input) != 0) ? list->erase(i) : (i + 1);
Nit: Seems clearer:

  i = StartsWith(i->suggestion(), input, false) ? (i + 1) : list->erase(i);

Note that passing "false" here does case-insensitive comparison... I think
that's probably what you want (though you should check with the suggest guys).

BestURLPrefix() should also do case-insensitive comparisons, I think, which
should address the similar problem below.

You may also want to remove the "!" in RemoveStaleNavigationResults() and
reverse the conditional so the orders in the two functions still match.

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:694: return true;
Nit: Did you want to preserve the more fine-grained tracking you previously had?
 It seems like it'd work OK as long as we also added in a "results_updated |=
(!suggest_results.empty() || !navigation_results.empty());" before clearing
those lists.

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.h:362: bool has_verbatim_relevance_;
Nit: You could eliminate this and substitute with a "verbatim_relevance_ > 0"
conditional everywhere you check it, I think, which seems just as understandable
and slightly simpler.

[msw, 2012-06-05 01:49:40]

Thanks; I'll land with green try jobs.

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete.h (right):

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete.h:177: // used in place of client-side
calculated values. See http://crbug.com/125871
On 2012/06/05 00:47:51, Peter Kasting wrote:
> Nit: I'd just omit the bug reference

Done.

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:552: i =
(i->suggestion().find(input) != 0) ? list->erase(i) : (i + 1);
On 2012/06/05 00:47:51, Peter Kasting wrote:
> Nit: Seems clearer:
> 
>   i = StartsWith(i->suggestion(), input, false) ? (i + 1) : list->erase(i);
> 
> Note that passing "false" here does case-insensitive comparison... I think
> that's probably what you want (though you should check with the suggest guys).
> 
> BestURLPrefix() should also do case-insensitive comparisons, I think, which
> should address the similar problem below.
> 
> You may also want to remove the "!" in RemoveStaleNavigationResults() and
> reverse the conditional so the orders in the two functions still match.

Done.

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:694: return true;
On 2012/06/05 00:47:51, Peter Kasting wrote:
> Nit: Did you want to preserve the more fine-grained tracking you previously
had?
>  It seems like it'd work OK as long as we also added in a "results_updated |=
> (!suggest_results.empty() || !navigation_results.empty());" before clearing
> those lists.

I'll leave this as-is. Getting the exact same results would also trigger a
results_updated 'false positive'. The only downside here seems to be a (minor?)
perf penalty. I think code simplicity is more important for now.

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10274023/diff/39001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.h:362: bool has_verbatim_relevance_;
On 2012/06/05 00:47:51, Peter Kasting wrote:
> Nit: You could eliminate this and substitute with a "verbatim_relevance_ > 0"
> conditional everywhere you check it, I think, which seems just as
understandable
> and slightly simpler.

Done.

[Peter Kasting, 2012-06-05 02:28:21]

On 2012/06/05 01:49:40, msw wrote:
> > BestURLPrefix() should also do case-insensitive comparisons, I think, which
> > should address the similar problem below.
> 
> Done.

Sorry, I was unclear -- I meant by this "BestURLPrefix() should be changed to do
case-insensitive comparisons".  It doesn't do them currently, which is a bug.

[msw, 2012-06-05 03:37:17]

On 2012/06/05 02:28:21, Peter Kasting wrote:
> "BestURLPrefix() should be changed to do case-insensitive comparisons".  It
doesn't do them currently, which is a bug.

Gotcha, fixed. I'll land with green try jobs.

[Peter Kasting, 2012-06-05 03:40:33]

Rad, code is nicely simpler too :)

[commit-bot: I haz the power, 2012-06-05 05:36:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10274023/38003

[commit-bot: I haz the power, 2012-06-05 08:36:14]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-06-05 09:19:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10274023/38003

[commit-bot: I haz the power, 2012-06-05 10:26:46]

Change committed as 140512