Always get device conditions from Java for every attempt.

Always get device conditions from Java for every attempt.

Previously we used the NetworkChangeNotifier, but it does not
get network change notifications when Chrome runs in the background,
which is our primary use case.  So, before starting each request,
get the latest device conditions from Android directly via the Java
API.

BUG=682790
Review-Url: https://codereview.chromium.org/2659813006
Cr-Commit-Position: refs/heads/master@{#447201}
Committed: https://chromium.googlesource.com/chromium/src/+/a2c4dd1ab5ba1423bdb46bd4b0111b942ad5f3fb

[Pete Williamson, 2017-01-27 22:50:09]

petewil@chromium.org changed reviewers:
+ dewittj@chromium.org, dougarnett@chromium.org

[Pete Williamson, 2017-01-27 22:50:09]

DewittJ:  Please check the JNI code for correctness, approach, and style.

DougArnett: Please check everything

Romax: Feel free to chime in if the evaluation test is adversely affected.

[Pete Williamson, 2017-01-27 22:53:28]

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:621: //
TODO(petewil): Now that we can get conditions any time, consider getting
I'd like to defer this until another changelist, this change is big enough as it
is, and I'd like to try to get this merged to M57.

[dewittj, 2017-01-27 23:03:43]

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:105: if
(java_conditions_object.is_null()) {
nit: no braces

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:110:
reinterpret_cast<jobject>(java_conditions_object.obj());
this cast is not necessary.

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:118:
env->GetFieldID(device_conditions_clazz.obj(), "mPowerConnected", "Z");
I'd like to recommend you use a more standard Chromium pattern, such as in this
file:
https://cs.chromium.org/chromium/src/base/android/unguessable_token_android.c...

The primary advantage is type checking on both sides - the implementation here
will only fail at runtime if the java fields are renamed.

This is the first usage of this call in all of Chrome besides third_party.  I'd
like you to get someone more experienced with JNI than me to review if you want
to retain bare JNI.  Additionally this is prone to runtime failure if these
members are ever renamed.

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:132: int
network_int = static_cast<int>(jnetwork);
just do the single cast to ConnectionType

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:138:
device_conditions_.reset(new DeviceConditions(device_conditions));
device_conditions_ = base::MakeUnique<DeviceConditions>(power, battery,
network_connection_type)

Saves an extra construction and copy

[dougarnett, 2017-01-27 23:15:56]

lgtm - with deferral of JNI review to dewittj

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:101: //
Convert return value from Java to C++
nit - period

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:105: if
(java_conditions_object.is_null()) {
On 2017/01/27 23:03:43, dewittj wrote:
> nit: no braces

nit: keep braces if comment stays

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:621: //
TODO(petewil): Now that we can get conditions any time, consider getting
On 2017/01/27 22:53:28, Pete Williamson wrote:
> I'd like to defer this until another changelist, this change is big enough as
it
> is, and I'd like to try to get this merged to M57.

Acknowledged.

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/scheduler.h (right):

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/scheduler.h:46: // Get the current
device conditions from the android APIs
nit - period

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/scheduler_stub.h (right):

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/scheduler_stub.h:27: // Get the current
device conditions from the android APIs
nit - period

[Pete Williamson, 2017-01-28 00:02:24]

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

[commit-bot: I haz the power, 2017-01-28 00:03:28]

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

[Pete Williamson, 2017-01-28 00:24:46]

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:105: if
(java_conditions_object.is_null()) {
On 2017/01/27 23:03:43, dewittj wrote:
> nit: no braces

Done.

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:105: if
(java_conditions_object.is_null()) {
On 2017/01/27 23:15:56, dougarnett wrote:
> On 2017/01/27 23:03:43, dewittj wrote:
> > nit: no braces
> 
> nit: keep braces if comment stays 

Moved comment above if statement, and lost the braces.

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:110:
reinterpret_cast<jobject>(java_conditions_object.obj());
On 2017/01/27 23:03:43, dewittj wrote:
> this cast is not necessary.

Done.

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:118:
env->GetFieldID(device_conditions_clazz.obj(), "mPowerConnected", "Z");
On 2017/01/27 23:03:43, dewittj wrote:
> I'd like to recommend you use a more standard Chromium pattern, such as in
this
> file:
>
https://cs.chromium.org/chromium/src/base/android/unguessable_token_android.c...
> 
> The primary advantage is type checking on both sides - the implementation here
> will only fail at runtime if the java fields are renamed.
> 
> This is the first usage of this call in all of Chrome besides third_party. 
I'd
> like you to get someone more experienced with JNI than me to review if you
want
> to retain bare JNI.  Additionally this is prone to runtime failure if these
> members are ever renamed.

Done.

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:132: int
network_int = static_cast<int>(jnetwork);
On 2017/01/27 23:03:43, dewittj wrote:
> just do the single cast to ConnectionType

Done.

https://codereview.chromium.org/2659813006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:138:
device_conditions_.reset(new DeviceConditions(device_conditions));
On 2017/01/27 23:03:43, dewittj wrote:
> device_conditions_ = base::MakeUnique<DeviceConditions>(power, battery,
> network_connection_type)
> 
> Saves an extra construction and copy

Done.

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/scheduler.h (right):

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/scheduler.h:46: // Get the current
device conditions from the android APIs
On 2017/01/27 23:15:56, dougarnett wrote:
> nit - period

Done.

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/scheduler_stub.h (right):

https://codereview.chromium.org/2659813006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/scheduler_stub.h:27: // Get the current
device conditions from the android APIs
On 2017/01/27 23:15:56, dougarnett wrote:
> nit - period

Done.

[Pete Williamson, 2017-01-28 00:25:02]

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

[commit-bot: I haz the power, 2017-01-28 00:25: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, 2017-01-28 00:33:34]

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

[commit-bot: I haz the power, 2017-01-28 00:33:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dewittj, 2017-01-30 19:18:03]

lgtm % nits

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:242:
public static boolean getPowerConditions(Context context) {
Lack of whitespace between these functions doesn't match other places in the
file.

Also needs javadoc

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:622:
public static boolean isPowerConnected(Intent batteryStatus) {
needs javadoc now that it's public

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:630:
public static int batteryPercentage(Intent batteryStatus) {
same

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:59:
BackgroundSchedulerBridge::BackgroundSchedulerBridge() {}
could use = default instead of {}

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:99:
DeviceConditions& BackgroundSchedulerBridge::GetCurrentDeviceConditions() {
const

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:115: return
*(device_conditions_.get());
I think you can just say *device_conditions_

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_scheduler_bridge.h (right):

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.h:27:
DeviceConditions& GetCurrentDeviceConditions() override;
const

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:613:
current_conditions_.reset(new DeviceConditions(conditions));
nit: combine these two lines to avoid a copy, or even:

current_conditions_ =
base::MakeUnique<DeviceConditions>(scheduler_->GetCurrentDeviceConditions());

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:621: //
TODO(petewil): Now that we can get conditions any time, consider getting
Is there (or should there be) a bug for this todo?

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
File components/offline_pages/core/background/scheduler.h (right):

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/scheduler.h:47: virtual
DeviceConditions& GetCurrentDeviceConditions() = 0;
const

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
File components/offline_pages/core/background/scheduler_stub.h (right):

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/scheduler_stub.h:28: DeviceConditions&
GetCurrentDeviceConditions() override;
const

[Pete Williamson, 2017-01-30 21:35:24]

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:242:
public static boolean getPowerConditions(Context context) {
On 2017/01/30 19:18:02, dewittj wrote:
> Lack of whitespace between these functions doesn't match other places in the
> file.
> 
> Also needs javadoc

Done.

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:622:
public static boolean isPowerConnected(Intent batteryStatus) {
On 2017/01/30 19:18:02, dewittj wrote:
> needs javadoc now that it's public

Oops, this didn't need to be public, changed back to private.

https://codereview.chromium.org/2659813006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:630:
public static int batteryPercentage(Intent batteryStatus) {
On 2017/01/30 19:18:02, dewittj wrote:
> same

same

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_scheduler_bridge.cc
(right):

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:59:
BackgroundSchedulerBridge::BackgroundSchedulerBridge() {}
On 2017/01/30 19:18:02, dewittj wrote:
> could use = default instead of {}

Done.

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:99:
DeviceConditions& BackgroundSchedulerBridge::GetCurrentDeviceConditions() {
On 2017/01/30 19:18:02, dewittj wrote:
> const

Done.

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:115: return
*(device_conditions_.get());
On 2017/01/30 19:18:02, dewittj wrote:
> I think you can just say *device_conditions_

Done.

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_scheduler_bridge.h (right):

https://codereview.chromium.org/2659813006/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.h:27:
DeviceConditions& GetCurrentDeviceConditions() override;
On 2017/01/30 19:18:02, dewittj wrote:
> const

Done.

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:613:
current_conditions_.reset(new DeviceConditions(conditions));
On 2017/01/30 19:18:02, dewittj wrote:
> nit: combine these two lines to avoid a copy, or even:
> 
> current_conditions_ =
> base::MakeUnique<DeviceConditions>(scheduler_->GetCurrentDeviceConditions());

Done.

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:621: //
TODO(petewil): Now that we can get conditions any time, consider getting
On 2017/01/30 19:18:02, dewittj wrote:
> Is there (or should there be) a bug for this todo?

There is not.  I'm not particularly concerned about this refactoring, not sure
we need a bug to keep track of it.  LMK if you disagree, and I'll open a bug.

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
File components/offline_pages/core/background/scheduler.h (right):

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/scheduler.h:47: virtual
DeviceConditions& GetCurrentDeviceConditions() = 0;
On 2017/01/30 19:18:02, dewittj wrote:
> const

Done.

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
File components/offline_pages/core/background/scheduler_stub.h (right):

https://codereview.chromium.org/2659813006/diff/20001/components/offline_page...
components/offline_pages/core/background/scheduler_stub.h:28: DeviceConditions&
GetCurrentDeviceConditions() override;
On 2017/01/30 19:18:03, dewittj wrote:
> const

Done.

[Pete Williamson, 2017-01-30 21:50:20]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2017-01-30 21:50:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2659813006/#ps40001
(title: "fixes per DewittJ")

[commit-bot: I haz the power, 2017-01-30 21:50:51]

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, 2017-01-30 23:42:55]

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

[commit-bot: I haz the power, 2017-01-30 23:42:57]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Pete Williamson, 2017-01-31 06:14:18]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2017-01-31 06:14:29]

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, 2017-01-31 06:50:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485843258906310,
"parent_rev": "ccdfe0541dd3f6e7c85fc774772e3ff880730216", "commit_rev":
"a2c4dd1ab5ba1423bdb46bd4b0111b942ad5f3fb"}

[commit-bot: I haz the power, 2017-01-31 06:51:51]

Description was changed from

==========
Always get device conditions from Java for every attempt.

Previously we used the NetworkChangeNotifier, but it does not
get network change notifications when Chrome runs in the background,
which is our primary use case.  So, before starting each request,
get the latest device conditions from Android directly via the Java
API.

BUG=682790
==========

to

==========
Always get device conditions from Java for every attempt.

Previously we used the NetworkChangeNotifier, but it does not
get network change notifications when Chrome runs in the background,
which is our primary use case.  So, before starting each request,
get the latest device conditions from Android directly via the Java
API.

BUG=682790

Review-Url: https://codereview.chromium.org/2659813006
Cr-Commit-Position: refs/heads/master@{#447201}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2c4dd1ab5ba1423bdb46bd4b011...
==========

[commit-bot: I haz the power, 2017-01-31 06:51:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a2c4dd1ab5ba1423bdb46bd4b011...

[RE66, 2017-01-31 13:08:55]

On 2017/01/31 06:51:52, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
>
https://chromium.googlesource.com/chromium/src/+/a2c4dd1ab5ba1423bdb46bd4b011...

Its seems a bad solution ( performance wise ) 
You should understand when and why NetworkChangeNotifier does not work and only
then 
get the latest device conditions

[Pete Williamson, 2017-01-31 17:53:06]

On 2017/01/31 13:08:55, RE66 wrote:
> On 2017/01/31 06:51:52, commit-bot: I haz the power wrote:
> > Committed patchset #3 (id:40001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/a2c4dd1ab5ba1423bdb46bd4b011...
> 
> Its seems a bad solution ( performance wise ) 
> You should understand when and why NetworkChangeNotifier does not work and
only
> then 
> get the latest device conditions

We do understand when NetworkChangeNotifier does not work for us, it does not
work anytime chrome is in the background.  Our feature normally activates when
chrome is in the background.

We are not too concerned about performance since the call happens only about
once a minute or so when our feature is active, which is a very small percentage
of the time.  Also, we anticipate removing two of the three JNI calls in the
near future depending on how the design of another component progresses.