Set up a field trial flag to prerender Instant search base url on omnibox focus event.

Set up a field trial flag to prerender Instant search base url on omnibox focus event.

BUG=384106
TEST=none

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

[kmadhusu, 2014-06-12 21:12:39]

Please file a bug and update the description.

Thanks.

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

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:70: "prerender_instant_search_base_on_focus";
How about prerender_instant_search_page_on_omnibox_focus?

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:582: FieldTrialFlags flags;
We should prerender Instant Search base page only when "prefetch-search-results"
command line flag is specified or prefetch_results field trial flag is enabled.

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:584: GetBoolValueForFlagWithDefault(
style nit: 4 space indentation.

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search.h
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.h:171: bool
ShouldPrerenderInstantSearchBaseOnFocus();
Please add a new unit test for this function.

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.h:171: bool
ShouldPrerenderInstantSearchBaseOnFocus();
How about "bool ShouldPrerenderInstantSearchBaseURLOnOmniboxFocus()"?

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

https://codereview.chromium.org/330123003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:950:
delegate_->DoPrerender(AutocompleteMatch());
We should not pass an new AutocompleteMatch() here. It makes me think may be
there is a better place to init InstantSearchPrerenderer. Can we do the
initialization in OmniboxCurrentPageDelegateImpl::OnFocusChanged()?

[sidharthms, 2014-06-12 23:33:34]

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

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:70: "prerender_instant_search_base_on_focus";
On 2014/06/12 21:12:38, kmadhusu wrote:
> How about prerender_instant_search_page_on_omnibox_focus?

Done.

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:582: FieldTrialFlags flags;
On 2014/06/12 21:12:38, kmadhusu wrote:
> We should prerender Instant Search base page only when
"prefetch-search-results"
> command line flag is specified or prefetch_results field trial flag is
enabled.

Done.

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:584: GetBoolValueForFlagWithDefault(
On 2014/06/12 21:12:38, kmadhusu wrote:
> style nit: 4 space indentation.

Done.

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search.h
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.h:171: bool
ShouldPrerenderInstantSearchBaseOnFocus();
On 2014/06/12 21:12:38, kmadhusu wrote:
> How about "bool ShouldPrerenderInstantSearchBaseURLOnOmniboxFocus()"?

Done.

https://codereview.chromium.org/330123003/diff/1/chrome/browser/search/search...
chrome/browser/search/search.h:171: bool
ShouldPrerenderInstantSearchBaseOnFocus();
On 2014/06/12 21:12:38, kmadhusu wrote:
> Please add a new unit test for this function.

Done.

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

https://codereview.chromium.org/330123003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:950:
delegate_->DoPrerender(AutocompleteMatch());
Moved to separate cl

[kmadhusu, 2014-06-12 23:59:15]

Please update the CL description.

Thanks.

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
chrome/browser/search/search.cc:587: GetBoolValueForFlagWithDefault(
nit: This can go the previous line as follows:

return GetFieldTrialInfo(&flags) && GetBoolValueForFlagWithDefault(
     kPrerenderInstantUrlOnOmniboxFocus, false, flags);

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
chrome/browser/search/search.h:170: // to prerender instant search base page on
focusing omnibox.
Returns true if 'prerender_instant_url_on_omnibox_focus' flag is enabled in
field trials to prerender Instant search base page when the omnibox is focused.

[sidharthms, 2014-06-13 00:17:55]

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
chrome/browser/search/search.cc:587: GetBoolValueForFlagWithDefault(
On 2014/06/12 23:59:15, kmadhusu wrote:
> nit: This can go the previous line as follows:
> 
> return GetFieldTrialInfo(&flags) && GetBoolValueForFlagWithDefault(
>      kPrerenderInstantUrlOnOmniboxFocus, false, flags);

Done.

[sidharthms, 2014-06-13 00:29:01]

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/330123003/diff/40001/chrome/browser/search/se...
chrome/browser/search/search.h:170: // to prerender instant search base page on
focusing omnibox.
On 2014/06/12 23:59:15, kmadhusu wrote:
> Returns true if 'prerender_instant_url_on_omnibox_focus' flag is enabled in
> field trials to prerender Instant search base page when the omnibox is
focused.

Done.

[kmadhusu, 2014-06-13 00:40:28]

https://codereview.chromium.org/330123003/diff/60001/chrome/browser/search/se...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/330123003/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.cc:581: bool
ShouldPrerenderInstantUrlOnOmniboxFocus() {
On Android Chrome, Embedded Search API is not enabled for all users. Therefore,
it's better to add the following check before reading the field trial flags:
  if (!IsInstantExtendedAPIEnabled())
    return false;

Sorry I missed this during the previous review.

[sidharthms, 2014-06-13 17:46:44]

https://codereview.chromium.org/330123003/diff/60001/chrome/browser/search/se...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/330123003/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.cc:581: bool
ShouldPrerenderInstantUrlOnOmniboxFocus() {
On 2014/06/13 00:40:28, kmadhusu wrote:
> On Android Chrome, Embedded Search API is not enabled for all users.
Therefore,
> it's better to add the following check before reading the field trial flags:
>   if (!IsInstantExtendedAPIEnabled())
>     return false;
> 
> Sorry I missed this during the previous review.

Done.

[kmadhusu, 2014-06-13 17:50:03]

lgtm.

Thanks.

[sidharthms, 2014-06-13 17:52:14]

The CQ bit was checked by sidharthms@chromium.org

[commit-bot: I haz the power, 2014-06-13 17:53:44]

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

[commit-bot: I haz the power, 2014-06-13 21:29:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[sidharthms, 2014-06-13 21:41:22]

The CQ bit was unchecked by sidharthms@chromium.org

[sidharthms, 2014-06-14 00:08:36]

The CQ bit was checked by sidharthms@chromium.org

[commit-bot: I haz the power, 2014-06-14 00:12:35]

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

[commit-bot: I haz the power, 2014-06-14 07:28:47]

Change committed as 277192