Allow non default prefetch suggestions to be prefetched

Allow non default prefetch suggestions to be prefetched

The SearchProvider may mark some suggestions to be prefetched based on
instructions from the suggest server. Currently we prefetch it only if such a
match ranks sufficiently highly. This cl allows to prefetch the suggestion
regardless of its autocomplete rank if allow_prefetch_non_default_match field
trial is enabled.

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

[kmadhusu, 2014-06-18 17:07:38]

I filed crbug.com/386203 to track this variation. Please update the CL
description bug details.

Thanks.

https://codereview.chromium.org/324273004/diff/1/chrome/browser/search/search...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/324273004/diff/1/chrome/browser/search/search...
chrome/browser/search/search_unittest.cc:707:
"allow_prefetch_non_default_match:1"));
style nit: This can fit in the previous line.

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_controller.cc:36: // If the
allow_prefetch_non_default_match field trial is enabled we return the
s/allow_prefetch_.../kAllowPrefetchNonDefaultMatch/g

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_controller.cc:48: if (prefetch_match !=
result.end())
nit: You can use a ternary operator here and remove the else block. 

return prefetch_match != result.end() ? &(*prefetch_match) : NULL;

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_controller.cc:49: return &(*prefetch_match);
As you said yesterday, when we type "ya" we are submitting a prefetch request
for the search query "youtube". We should figure out why that's happening. (FYI)
When we sent a request to fetch results for "youtube", "youtube" was not
displayed in the suggestions list.

[sidharthms, 2014-06-18 22:45:22]

https://codereview.chromium.org/324273004/diff/1/chrome/browser/search/search...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/324273004/diff/1/chrome/browser/search/search...
chrome/browser/search/search_unittest.cc:707:
"allow_prefetch_non_default_match:1"));
On 2014/06/18 17:07:38, kmadhusu wrote:
> style nit: This can fit in the previous line.

Done.

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_controller.cc:36: // If the
allow_prefetch_non_default_match field trial is enabled we return the
On 2014/06/18 17:07:38, kmadhusu wrote:
> s/allow_prefetch_.../kAllowPrefetchNonDefaultMatch/g

Done.

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_controller.cc:48: if (prefetch_match !=
result.end())
On 2014/06/18 17:07:38, kmadhusu wrote:
> nit: You can use a ternary operator here and remove the else block. 
> 
> return prefetch_match != result.end() ? &(*prefetch_match) : NULL;

Done.

https://codereview.chromium.org/324273004/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_controller.cc:49: return &(*prefetch_match);
On 2014/06/18 17:07:38, kmadhusu wrote:
> As you said yesterday, when we type "ya" we are submitting a prefetch request
> for the search query "youtube". We should figure out why that's happening.
(FYI)
> When we sent a request to fetch results for "youtube", "youtube" was not
> displayed in the suggestions list. 

Done.

[kmadhusu, 2014-06-19 19:56:32]

https://codereview.chromium.org/324273004/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/324273004/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:116:
chrome::IsInstantExtendedAPIEnabled()) {
Previously, AutocompleteController::OnResultChanged() didn't send an empty
prefetch suggestion when the autocomplete result set is empty, but now this
method does. Is this intentional?

[kmadhusu, 2014-06-19 19:56:58]

pkasting@: Please review chrome/browser/ui/omnibox/omnibox_controller.cc
changes.

Thanks.

[sidharthms, 2014-06-19 20:52:58]

https://codereview.chromium.org/324273004/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/324273004/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:116:
chrome::IsInstantExtendedAPIEnabled()) {
On 2014/06/19 19:56:31, kmadhusu wrote:
> Previously, AutocompleteController::OnResultChanged() didn't send an empty
> prefetch suggestion when the autocomplete result set is empty, but now this
> method does. Is this intentional?

Done.

[Peter Kasting, 2014-06-19 23:03:09]

LGTM

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:49: } else {
Nit: No else after return.

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:95: const AutocompleteResult&
result = this->result();
Nit: I think it would be better to remove this and use result() below as needed.

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:121: if
(!chrome::IsInstantExtendedAPIEnabled())
Nit: Why not include this in the next conditional (if necessary, making that
conditional be an early-return)?

[sidharthms, 2014-06-20 00:30:49]

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:49: } else {
On 2014/06/19 23:03:09, Peter Kasting wrote:
> Nit: No else after return.

Done.

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:95: const AutocompleteResult&
result = this->result();
On 2014/06/19 23:03:09, Peter Kasting wrote:
> Nit: I think it would be better to remove this and use result() below as
needed.

Done.

https://codereview.chromium.org/324273004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_controller.cc:121: if
(!chrome::IsInstantExtendedAPIEnabled())
On 2014/06/19 23:03:08, Peter Kasting wrote:
> Nit: Why not include this in the next conditional (if necessary, making that
> conditional be an early-return)?

Done.

[kmadhusu, 2014-06-20 00:46:40]

lgtm

[sidharthms, 2014-06-20 01:09:38]

The CQ bit was checked by sidharthms@chromium.org

[commit-bot: I haz the power, 2014-06-20 01:13:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sidharthms@chromium.org/324273004/60001

[commit-bot: I haz the power, 2014-06-20 10:54:22]

Change committed as 278668