predictors: Make the resource_prefetch_predictor accessible from Java.

predictors: Make the resource_prefetch_predictor accessible from Java.

This allows the Android code to start and stop prefetches. Now that the
prefetcher is decoupled from navigation, prefetch requests can be issued
with just a URL.

Also finishes converting the prefetch predictor to start and stop prefetches
from a URL.

BUG=631966
Committed: https://crrev.com/a1a02928b8033a7e413930b28ca1dff8af773eb9
Cr-Commit-Position: refs/heads/master@{#423120}

[Benoit L, 2016-09-27 13:31:35]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 13:31:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 14:27:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 14:27:53]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-09-27 14:31:51]

Description was changed from

==========
predictors: Make the resource_prefetch_predictor accessible from Java.

This allows the Android code to start and stop prefetches. Now that the
prefetcher is decoupled from navigation, prefetch requests can be issued
with just a URL.

BUG=631966
==========

to

==========
predictors: Make the resource_prefetch_predictor accessible from Java.

This allows the Android code to start and stop prefetches. Now that the
prefetcher is decoupled from navigation, prefetch requests can be issued
with just a URL.

Also finishes converting the prefetch predictor to start and stop prefetches
from a URL.

BUG=631966
==========

[Benoit L, 2016-09-27 15:35:37]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2016-09-27 16:54:31]

Overall looks good, but I'd suggest to ask someone more knowledgeable in our JNI
magic to do a sanity check. Does not look very intricate, but just for extra
safety.

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

https://codereview.chromium.org/2377643002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ResourcePrefetchPredictor.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
why is it in customtabs? Such utility could be useful in chrome/ outside
customtabs.

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (left):

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:560: // No prefetching
data at host or URL level.
the old version of putting the comment is correct and less confusing, please
revert to it.

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_android.h (right):

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_android.h:43:
ResourcePrefetchPredictor* predictor_;
What if ResourcePrefetchPredictor is destroyed? Should this be a weakptr
instead?

[Benoit L, 2016-09-28 12:15:25]

Thanks!

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

https://codereview.chromium.org/2377643002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ResourcePrefetchPredictor.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/09/27 16:54:31, pasko wrote:
> why is it in customtabs? Such utility could be useful in chrome/ outside
> customtabs.

Right now it's used only in Custom Tabs, and is still fairly experimental, so I
prefer to keep it there (and the class is package private as consequence).
Moving it elsewhere later is trivial, fortunately.

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (left):

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:560: // No prefetching
data at host or URL level.
On 2016/09/27 16:54:31, pasko wrote:
> the old version of putting the comment is correct and less confusing, please
> revert to it.

Done.

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_android.h (right):

https://codereview.chromium.org/2377643002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_android.h:43:
ResourcePrefetchPredictor* predictor_;
On 2016/09/27 16:54:31, pasko wrote:
> What if ResourcePrefetchPredictor is destroyed? Should this be a weakptr
> instead?

Changed to have the object hold a pointer to the profile, not to the predictor.
In practice this should not have much impact, as the predictor doesn't get
destroyed, but it's cleaner this way.

As for why the native profile pointer will live, that's because the native
object is accessed through a map keyed by Profile on the android side.

[Benoit L, 2016-09-28 14:57:24]

lizeb@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Benoit L, 2016-09-28 14:57:25]

+dfalcantara for JNI wizardry. PTAL, thanks.

This CL is simply about exposing some native code to Java.

[gone, 2016-09-28 18:18:15]

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ResourcePrefetchPredictor.java
(right):

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ResourcePrefetchPredictor.java:35:
if (sServiceMap == null) sServiceMap = new HashMap<Profile,
ResourcePrefetchPredictor>();
new HashMap<>():

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/browser/p...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/browser/p...
chrome/browser/predictors/resource_prefetch_predictor.cc:499: bool
ResourcePrefetchPredictor::GetPrefetchData(const GURL& url,
Can you use something more descriptive than url here, given that there's another
variable called urls afterward?  Don't know what the difference is without
looking over to the previous version or the callers for context.

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/browser/p...
File chrome/browser/predictors/resource_prefetch_predictor_android.cc (right):

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/browser/p...
chrome/browser/predictors/resource_prefetch_predictor_android.cc:48: Profile*
profile = ProfileAndroid::FromProfileAndroid(j_profile);
FromProfileAndroid can return null.

[pasko, 2016-09-29 16:04:58]

LGTM with Dan's suggestions addressed

[Benoit L, 2016-10-03 09:42:05]

Thanks!

PTAL.

https://codereview.chromium.org/2377643002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ResourcePrefetchPredictor.java
(right):

https://codereview.chromium.org/2377643002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ResourcePrefetchPredictor.java:35:
if (sServiceMap == null) sServiceMap = new HashMap<Profile,
ResourcePrefetchPredictor>();
On 2016/09/28 18:18:15, dfalcantara (slow) wrote:
> new HashMap<>():

Done.

https://codereview.chromium.org/2377643002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2377643002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:499: bool
ResourcePrefetchPredictor::GetPrefetchData(const GURL& url,
On 2016/09/28 18:18:15, dfalcantara (slow) wrote:
> Can you use something more descriptive than url here, given that there's
another
> variable called urls afterward?  Don't know what the difference is without
> looking over to the previous version or the callers for context.

Done.

https://codereview.chromium.org/2377643002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_android.cc (right):

https://codereview.chromium.org/2377643002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_android.cc:48: Profile*
profile = ProfileAndroid::FromProfileAndroid(j_profile);
On 2016/09/28 18:18:15, dfalcantara (slow) wrote:
> FromProfileAndroid can return null.

The resulting profile is used to call
ResourcePrefetchPredictorFactory::GetForProfile(). It returns nullptr if the
profile is nullptr, and there are checks in Start/StopPrefetching() that return
in this case. So I don't think there is an issue here, the question being more
to the effect of whether we want to make the NULL check in the native or Java
code.

I'm not sure what's best here. wdyt?

[Benoit L, 2016-10-03 09:42:13]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 09:42:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 09:44:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 09:44:48]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Benoit L, 2016-10-03 14:02:24]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 14:02:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 14:48:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 14:48:40]

Dry run: This issue passed the CQ dry run.

[gone, 2016-10-03 21:30:32]

lgtm % question

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/browser/p...
File chrome/browser/predictors/resource_prefetch_predictor_android.cc (right):

https://chromiumcodereview.appspot.com/2377643002/diff/40001/chrome/browser/p...
chrome/browser/predictors/resource_prefetch_predictor_android.cc:48: Profile*
profile = ProfileAndroid::FromProfileAndroid(j_profile);
On 2016/10/03 09:42:05, Benoit L wrote:
> On 2016/09/28 18:18:15, dfalcantara (slow) wrote:
> > FromProfileAndroid can return null.
> 
> The resulting profile is used to call
> ResourcePrefetchPredictorFactory::GetForProfile(). It returns nullptr if the
> profile is nullptr, and there are checks in Start/StopPrefetching() that
return
> in this case. So I don't think there is an issue here, the question being more
> to the effect of whether we want to make the NULL check in the native or Java
> code.
> 
> I'm not sure what's best here. wdyt?

If that and the ResourcePretchPredictorAndroid that you're constructing here are
accounted for then I guess you're fine.  It's deleted at some point, right?

[Benoit L, 2016-10-04 11:47:38]

Hi,

Sorry about the churn, this revision is not entirely trivial, but simpler.
Basically, I initially thought that more state of ResourcePrefetchPredictor
would be exposed to Java. It turns out that this is not going to be the case, so
it makes sense to simplify the interaction between Java and C++.

Basically now there is no longer a native side object wrapped in an Java one.

PTAL, thanks.

[pasko, 2016-10-04 12:19:47]

thanks, that's simpler

still lgtm

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.h (left):

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:106: 
nit: this empty line was nicely separating the commented block from the other
part

[gone, 2016-10-04 17:49:17]

Yar, a lot simpler.

still lgtm

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:243: // in the
|prefetch_data| to resources that need to be prefetched.
|prefetch_data| is no longer a variable here.

[Benoit L, 2016-10-05 08:36:22]

Thanks!

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.h (left):

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:106: 
On 2016/10/04 12:19:47, pasko wrote:
> nit: this empty line was nicely separating the commented block from the other
> part

Done.

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2377643002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:243: // in the
|prefetch_data| to resources that need to be prefetched.
On 2016/10/04 17:49:16, dfalcantara (slow) wrote:
> |prefetch_data| is no longer a variable here.

Done.

[Benoit L, 2016-10-05 08:36:27]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-10-05 08:36:27]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2377643002/#ps120001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-10-05 08:36:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 09:32:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-05 09:34:36]

Description was changed from

==========
predictors: Make the resource_prefetch_predictor accessible from Java.

This allows the Android code to start and stop prefetches. Now that the
prefetcher is decoupled from navigation, prefetch requests can be issued
with just a URL.

Also finishes converting the prefetch predictor to start and stop prefetches
from a URL.

BUG=631966
==========

to

==========
predictors: Make the resource_prefetch_predictor accessible from Java.

This allows the Android code to start and stop prefetches. Now that the
prefetcher is decoupled from navigation, prefetch requests can be issued
with just a URL.

Also finishes converting the prefetch predictor to start and stop prefetches
from a URL.

BUG=631966

Committed: https://crrev.com/a1a02928b8033a7e413930b28ca1dff8af773eb9
Cr-Commit-Position: refs/heads/master@{#423120}
==========

[commit-bot: I haz the power, 2016-10-05 09:34:37]

Patchset 7 (id:??) landed as
https://crrev.com/a1a02928b8033a7e413930b28ca1dff8af773eb9
Cr-Commit-Position: refs/heads/master@{#423120}