Prefetch high-confidence search suggestions in Android Chrome.

To support Instant search in Android Chrome, this CL
(1) Prerenders an Instant search base page on the receipt of a high-confidence search suggestion.
(2) Prefetches the search results for the high-confidence suggestion using the prerendered page.
(3) When the user commits the search query, Android Chrome submits the query to the prerendered page and swaps the active tab contents with the prerendered contents.

BUG=382694
TEST= Apply this patch, build Android Chrome and run it against the server url specified in go/yzcku. Type a search query in the LocationBar. Android Chrome should identify the high-confidence suggestion for the typed query and prefetch the results for that suggestion. Commit the typed query. Android Chrome should swap the active tab contents with the prerendered page contents. You should see the search results instantaneously.

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

[kmadhusu, 2014-06-09 23:02:26]

Please review.

Thanks.

[Ted C, 2014-06-10 20:56:30]

Overall seems reasonable, but hopefully samarth can check more on the logic side
of the prerendering behavior

https://codereview.chromium.org/321943002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):

https://codereview.chromium.org/321943002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:336: if
(chrome::IsInstantExtendedAPIEnabled() &&
should we also check default_match_changed since it appears to only operate on
that?

https://codereview.chromium.org/321943002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:355:
prerenderer->Prerender(prefetch_suggestion);
Is this something that needs to be cancelled?  Like if the user calls stop (i.e.
they hide the suggestions popup but does not issue a navigation).

[kmadhusu, 2014-06-10 22:04:49]

https://codereview.chromium.org/321943002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):

https://codereview.chromium.org/321943002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:336: if
(chrome::IsInstantExtendedAPIEnabled() &&
On 2014/06/10 20:56:29, Ted C wrote:
> should we also check default_match_changed since it appears to only operate on
> that?

Done.

https://codereview.chromium.org/321943002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:355:
prerenderer->Prerender(prefetch_suggestion);
On 2014/06/10 20:56:29, Ted C wrote:
> Is this something that needs to be cancelled?  Like if the user calls stop
(i.e.
> they hide the suggestions popup but does not issue a navigation).

When an user navigates to an URL using TabAndroid::LoadUrl(), we will either
swap the active tab contents with the prerendered contents or cancel the
prerender request.

Based on my testing, I can do the following without any issues:
1. Open 2 new tabs and focus the first new tab.
2. Type "southwest" in the LocationBar.
3. (Chrome should have prerendered Instant search base page url and prefetched
search results for "southwest")
4. Hide the suggestions popup.
5. Switch to the other new tab.
6. Type "paypal" and submit the search query.

When I did (6), we are actually reusing the instant search base page created by
the previous tab. When the user commits the search query, Chrome can
successfully swap the active tab contents with the prerendered contents.
Therefore, we don't need to cancel the prerender request when an user hides the
suggestion popup. 

If we didn't use the prerendered contents for a long time, PrerenderManager will
take care of cancelling that request.

[Ted C, 2014-06-10 23:24:17]

lgtm

[samarth, 2014-06-13 00:58:00]

You should probably have Jered take a look at this.  He's much more familiar
with the InstantSearchPrerenderer code than I am.

Samarth

[kmadhusu, 2014-06-13 01:04:02]

On 2014/06/13 00:58:00, samarth wrote:
> You should probably have Jered take a look at this.  He's much more familiar
> with the InstantSearchPrerenderer code than I am.
> 
> Samarth

Sure. Thanks.

[kmadhusu, 2014-06-13 01:04:17]

jered@: PTAL.

Thanks.

[Jered, 2014-06-13 16:53:20]

https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):

https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:336: if
(default_match_changed &&
AutocompleteController::OnResultChanged() does not prerender if the autocomplete
result set is empty, but this method does. Is that behavior difference
intentional or should it be the same in both places?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[kmadhusu, 2014-06-13 17:42:14]

jered@: Addressed comments.

Thanks.

https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):

https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:336: if
(default_match_changed &&
On 2014/06/13 16:53:20, Jered wrote:
> AutocompleteController::OnResultChanged() does not prerender if the
autocomplete
> result set is empty, but this method does. Is that behavior difference
> intentional or should it be the same in both places?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

It wasn't intentional. Updated the code to look the same in both places. Good
catch.

[Jered, 2014-06-13 17:58:18]

On 2014/06/13 17:42:14, kmadhusu wrote:
> jered@: Addressed comments.
> 
> Thanks.
> 
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> File chrome/browser/android/omnibox/autocomplete_controller_android.cc
(right):
> 
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> chrome/browser/android/omnibox/autocomplete_controller_android.cc:336: if
> (default_match_changed &&
> On 2014/06/13 16:53:20, Jered wrote:
> > AutocompleteController::OnResultChanged() does not prerender if the
> autocomplete
> > result set is empty, but this method does. Is that behavior difference
> > intentional or should it be the same in both places?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> It wasn't intentional. Updated the code to look the same in both places. Good
> catch.

lgtm

Are there no tests for this code? :-/

[kmadhusu, 2014-06-13 18:33:42]

On 2014/06/13 17:58:18, Jered wrote:
> On 2014/06/13 17:42:14, kmadhusu wrote:
> > jered@: Addressed comments.
> > 
> > Thanks.
> > 
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > File chrome/browser/android/omnibox/autocomplete_controller_android.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > chrome/browser/android/omnibox/autocomplete_controller_android.cc:336: if
> > (default_match_changed &&
> > On 2014/06/13 16:53:20, Jered wrote:
> > > AutocompleteController::OnResultChanged() does not prerender if the
> > autocomplete
> > > result set is empty, but this method does. Is that behavior difference
> > > intentional or should it be the same in both places?
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > 
> > It wasn't intentional. Updated the code to look the same in both places.
Good
> > catch.
> 
> lgtm
> 
> Are there no tests for this code? :-/

I don't want to write a flaky interactive ui test/browser test. I thought about
writing unit tests. But this class notifies java code and I am not sure how that
will work in those test cases. If you really want a test, I can move the new
code to a helper function and write a unit test for that helper function. What
do you think?

[Jered, 2014-06-13 18:37:25]

On 2014/06/13 18:33:42, kmadhusu wrote:
> On 2014/06/13 17:58:18, Jered wrote:
> > On 2014/06/13 17:42:14, kmadhusu wrote:
> > > jered@: Addressed comments.
> > > 
> > > Thanks.
> > > 
> > >
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > > File chrome/browser/android/omnibox/autocomplete_controller_android.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > > chrome/browser/android/omnibox/autocomplete_controller_android.cc:336: if
> > > (default_match_changed &&
> > > On 2014/06/13 16:53:20, Jered wrote:
> > > > AutocompleteController::OnResultChanged() does not prerender if the
> > > autocomplete
> > > > result set is empty, but this method does. Is that behavior difference
> > > > intentional or should it be the same in both places?
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > > 
> > > It wasn't intentional. Updated the code to look the same in both places.
> Good
> > > catch.
> > 
> > lgtm
> > 
> > Are there no tests for this code? :-/
> 
> I don't want to write a flaky interactive ui test/browser test. I thought
about
> writing unit tests. But this class notifies java code and I am not sure how
that
> will work in those test cases. If you really want a test, I can move the new
> code to a helper function and write a unit test for that helper function. What
> do you think?

If it's easy to add a unit test, that would be a good start. If it's not, don't
spend too
much time on it, since that's a deeper problem---I am fine with submitting this
as-is.

[kmadhusu, 2014-06-17 01:20:36]

On 2014/06/13 18:37:25, Jered wrote:
> On 2014/06/13 18:33:42, kmadhusu wrote:
> > On 2014/06/13 17:58:18, Jered wrote:
> > > On 2014/06/13 17:42:14, kmadhusu wrote:
> > > > jered@: Addressed comments.
> > > > 
> > > > Thanks.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > > > File chrome/browser/android/omnibox/autocomplete_controller_android.cc
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > > > chrome/browser/android/omnibox/autocomplete_controller_android.cc:336:
if
> > > > (default_match_changed &&
> > > > On 2014/06/13 16:53:20, Jered wrote:
> > > > > AutocompleteController::OnResultChanged() does not prerender if the
> > > > autocomplete
> > > > > result set is empty, but this method does. Is that behavior difference
> > > > > intentional or should it be the same in both places?
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > > > 
> > > > It wasn't intentional. Updated the code to look the same in both places.
> > Good
> > > > catch.
> > > 
> > > lgtm
> > > 
> > > Are there no tests for this code? :-/
> > 
> > I don't want to write a flaky interactive ui test/browser test. I thought
> about
> > writing unit tests. But this class notifies java code and I am not sure how
> that
> > will work in those test cases. If you really want a test, I can move the new
> > code to a helper function and write a unit test for that helper function.
What
> > do you think?
> 
> If it's easy to add a unit test, that would be a good start. If it's not,
don't
> spend too
> much time on it, since that's a deeper problem---I am fine with submitting
this
> as-is.

Jered, I tried to write an unit test for this class. Unfortunately, it is not
that simple. It heavily depends on BrowserContextKeyedService and I am hitting a
fatal error in BrowserContextDependencyManager. Already I spent 1.5 days on this
test. Due to M37 branch cut, I am going to commit this CL as such. I will add a
test after this branch cut. 

Thanks.

[Jered, 2014-06-17 14:59:49]

On 2014/06/17 01:20:36, kmadhusu wrote:
> On 2014/06/13 18:37:25, Jered wrote:
> > On 2014/06/13 18:33:42, kmadhusu wrote:
> > > On 2014/06/13 17:58:18, Jered wrote:
> > > > On 2014/06/13 17:42:14, kmadhusu wrote:
> > > > > jered@: Addressed comments.
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > > > > File chrome/browser/android/omnibox/autocomplete_controller_android.cc
> > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/321943002/diff/40001/chrome/browser/android/o...
> > > > > chrome/browser/android/omnibox/autocomplete_controller_android.cc:336:
> if
> > > > > (default_match_changed &&
> > > > > On 2014/06/13 16:53:20, Jered wrote:
> > > > > > AutocompleteController::OnResultChanged() does not prerender if the
> > > > > autocomplete
> > > > > > result set is empty, but this method does. Is that behavior
difference
> > > > > > intentional or should it be the same in both places?
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > > > > 
> > > > > It wasn't intentional. Updated the code to look the same in both
places.
> > > Good
> > > > > catch.
> > > > 
> > > > lgtm
> > > > 
> > > > Are there no tests for this code? :-/
> > > 
> > > I don't want to write a flaky interactive ui test/browser test. I thought
> > about
> > > writing unit tests. But this class notifies java code and I am not sure
how
> > that
> > > will work in those test cases. If you really want a test, I can move the
new
> > > code to a helper function and write a unit test for that helper function.
> What
> > > do you think?
> > 
> > If it's easy to add a unit test, that would be a good start. If it's not,
> don't
> > spend too
> > much time on it, since that's a deeper problem---I am fine with submitting
> this
> > as-is.
> 
> Jered, I tried to write an unit test for this class. Unfortunately, it is not
> that simple. It heavily depends on BrowserContextKeyedService and I am hitting
a
> fatal error in BrowserContextDependencyManager. Already I spent 1.5 days on
this
> test. Due to M37 branch cut, I am going to commit this CL as such. I will add
a
> test after this branch cut. 
> 
> Thanks.

Ugh, yeah, that is what I was afraid of. It is difficult to retrofit tests for
code that wasn't designed to be tested. Thanks for trying, though.

[kmadhusu, 2014-06-17 15:33:27]

The CQ bit was checked by kmadhusu@chromium.org

[commit-bot: I haz the power, 2014-06-17 15:35:30]

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

[commit-bot: I haz the power, 2014-06-17 18:42:05]

Change committed as 277818

[luckyalvarez47, 2015-01-07 16:59:07]

luckyalvarez47@gmail.com changed reviewers:
+ luckyalvarez47@gmail.com

[luckyalvarez47, 2015-01-07 16:59:07]

lgtm

[luckyalvarez47, 2015-01-07 17:00:13]

lgtm

[luckyalvarez47, 2015-01-07 17:00:14]

lgtm