android: Add a method to let Java know whether a prerender has finished loading.

android: Add a method to let Java know whether a prerender has finished loading.

This is required to properly test corner cases in the uses of prerendering by
Custom Tabs. For instance, in crbug.com/554236, the issue only happens when the
prerender completes before the tab is shown. This CL updates the non-regression
test for this bug.

BUG=574864, 554236
Committed: https://crrev.com/405f35f44332ec468a25940bf300f436031f3368
Cr-Commit-Position: refs/heads/master@{#377868}

[Benoit L, 2016-02-22 12:56:23]

Description was changed from

==========
android: Add a method to let Java know whether a prerender has finished loading.

BUG=574864
==========

to

==========
android: Add a method to let Java know whether a prerender has finished loading.

This is required to properly test corner cases in the uses of prerendering by
Custom Tabs. For instance, in crbug.com/554236, the issue only happens when the
prerender completes before the tab is shown. This CL updates the non-regression
test for this bug.

BUG=574864,554236
==========

[Benoit L, 2016-02-22 12:59:42]

lizeb@chromium.org changed reviewers:
+ dfalcantara@chromium.org, gavinp@chromium.org, yusufo@chromium.org

[Benoit L, 2016-02-22 12:59:42]

Hello,

Here is a small change to expose a method to Java. There is not change in
behavior, but this improves a non-regression test, and will be used in
forthcoming CLs to make sure that we properly test the uses of prerendering we
have in Custom Tabs.

dfalcantara@chromium.org: chrome/android 

gavinp@chromium.org: prerender

PTAL, Thanks!

[gone, 2016-02-22 18:50:15]

lgtm % Yusuf's review

https://chromiumcodereview.appspot.com/1710853002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://chromiumcodereview.appspot.com/1710853002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:104:
PrerenderedUrlParams mPrerender;
Move up a line; I think package protected variables go above private ones

https://chromiumcodereview.appspot.com/1710853002/diff/20001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://chromiumcodereview.appspot.com/1710853002/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:792:
&& ExternalPrerenderHandler.hasPrerenderedAndFinishedLoadingUrl(
indentation seems really off

[Yusuf, 2016-02-22 22:11:32]

lgtm with a nite

https://chromiumcodereview.appspot.com/1710853002/diff/20001/chrome/browser/p...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://chromiumcodereview.appspot.com/1710853002/diff/20001/chrome/browser/p...
chrome/browser/prerender/external_prerender_handler_android.cc:27: static bool
ConvertParams(JNIEnv* env,
maybe a better name for this as we don't convert all but sanity check some?

maybe just checkAndConvertPrerenderingParams?

[Benoit L, 2016-02-25 16:03:21]

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:104:
PrerenderedUrlParams mPrerender;
On 2016/02/22 18:50:15, dfalcantara wrote:
> Move up a line; I think package protected variables go above private ones

Done.

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:792:
&& ExternalPrerenderHandler.hasPrerenderedAndFinishedLoadingUrl(
On 2016/02/22 18:50:15, dfalcantara wrote:
> indentation seems really off

It's weird indeed, but "git cl format" said so... :-/

https://codereview.chromium.org/1710853002/diff/20001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://codereview.chromium.org/1710853002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.cc:27: static bool
ConvertParams(JNIEnv* env,
On 2016/02/22 22:11:31, Yusuf wrote:
> maybe a better name for this as we don't convert all but sanity check some?
> 
> maybe just checkAndConvertPrerenderingParams?

Done.

[gone, 2016-02-25 18:02:01]

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:792:
&& ExternalPrerenderHandler.hasPrerenderedAndFinishedLoadingUrl(
On 2016/02/25 16:03:21, Benoit L wrote:
> On 2016/02/22 18:50:15, dfalcantara wrote:
> > indentation seems really off
> 
> It's weird indeed, but "git cl format" said so... :-/

git cl format never really works well on java... at least indent this by a
multiple of 8 or something...

[gavinp, 2016-02-25 18:44:55]

lgtm.

https://codereview.chromium.org/1710853002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://codereview.chromium.org/1710853002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.cc:27: static bool
CheckAndConvertParams(
I have trouble reviewing this java interface code; however I have confirmed that
you're definitely just cutting and pasting to share a common path; so I can
agree on that basis.

If someone else knows these java interfaces better, it wouldn't be a bad idea to
have them give it a second look.

https://codereview.chromium.org/1710853002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1710853002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:665: for (const auto&
prerender_data : active_prerenders_) {
Thanks. This is closely related and so refactoring this function is a good idea.

https://codereview.chromium.org/1710853002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/1710853002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:195: // SessionStorageNamespace
associated with the given WebContents.
Thanks. This has bugged me for a while, but never had a change close enough.

[Benoit L, 2016-02-26 10:25:19]

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://codereview.chromium.org/1710853002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:792:
&& ExternalPrerenderHandler.hasPrerenderedAndFinishedLoadingUrl(
On 2016/02/25 18:02:00, dfalcantara wrote:
> On 2016/02/25 16:03:21, Benoit L wrote:
> > On 2016/02/22 18:50:15, dfalcantara wrote:
> > > indentation seems really off
> > 
> > It's weird indeed, but "git cl format" said so... :-/
> 
> git cl format never really works well on java... at least indent this by a
> multiple of 8 or something...

Done.

[Benoit L, 2016-02-26 10:25:27]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-02-26 10:25:27]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org,
dfalcantara@chromium.org, gavinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1710853002/#ps60001
(title: "Indentation.")

[commit-bot: I haz the power, 2016-02-26 10:25:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710853002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710853002/60001

[commit-bot: I haz the power, 2016-02-26 11:14:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-26 11:17:12]

Description was changed from

==========
android: Add a method to let Java know whether a prerender has finished loading.

This is required to properly test corner cases in the uses of prerendering by
Custom Tabs. For instance, in crbug.com/554236, the issue only happens when the
prerender completes before the tab is shown. This CL updates the non-regression
test for this bug.

BUG=574864,554236
==========

to

==========
android: Add a method to let Java know whether a prerender has finished loading.

This is required to properly test corner cases in the uses of prerendering by
Custom Tabs. For instance, in crbug.com/554236, the issue only happens when the
prerender completes before the tab is shown. This CL updates the non-regression
test for this bug.

BUG=574864,554236

Committed: https://crrev.com/405f35f44332ec468a25940bf300f436031f3368
Cr-Commit-Position: refs/heads/master@{#377868}
==========

[commit-bot: I haz the power, 2016-02-26 11:17:13]

Patchset 4 (id:??) landed as
https://crrev.com/405f35f44332ec468a25940bf300f436031f3368
Cr-Commit-Position: refs/heads/master@{#377868}

[Patrick Monette, 2017-01-04 20:26:01]

On 2016/02/26 11:17:13, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/405f35f44332ec468a25940bf300f436031f3368
> Cr-Commit-Position: refs/heads/master@{#377868}

I came across 2 similar versions of HasPrerenderedUrl(). Can they be merged?
(Remove the tab_android.cc version).
https://cs.chromium.org/chromium/src/chrome/browser/android/tab_android.cc?rc...
https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_manag...