Defines initial DeviceConditions and and plumbs down through StartProcessing() call.

Defines initial DeviceConditions and and plumbs down through StartProcessing() call.

BUG=619686
Committed: https://crrev.com/445fd2b04e36fc3df963b42a07636d48fe552aad
Cr-Commit-Position: refs/heads/master@{#400455}

[dougarnett, 2016-06-15 21:29:02]

Description was changed from

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=612325
==========

to

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=607346
==========

[dougarnett, 2016-06-15 21:30:05]

Description was changed from

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=607346
==========

to

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=619686
==========

[dougarnett, 2016-06-15 22:06:20]

dougarnett@chromium.org changed reviewers:
+ petewil@google.com

[dougarnett, 2016-06-15 22:06:21]

Please give initial look (having trouble with trybots patching at the moment).

[Pete Williamson, 2016-06-16 17:46:25]

petewil@chromium.org changed reviewers:
+ petewil@chromium.org

[Pete Williamson, 2016-06-16 17:46:26]

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

https://codereview.chromium.org/2064323004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:213:
NetworkChangeNotifier.getInstance().getCurrentConnectionType());
So what Does the NetworkChangeNotifier return if we have both 2G and WiFi? 
Maybe the most recently connected of the two?  I suspect we'd really prefer to
know about WiFi if both are connected, since it will be the more limiting case,
can we do that?  (OK to add a TODO if it is hard, it looks like it might require
modifying NetworkChangeNotifier).

https://codereview.chromium.org/2064323004/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java
(right):

https://codereview.chromium.org/2064323004/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java:21:
public boolean startProcessing(DeviceConditions deviceConditions,
Callback<Boolean> callback) {
Might be a good idea to save the device conditions in a member variable, and
have a getter function so we can test them.

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

https://codereview.chromium.org/2064323004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:49:
j_power_connected, j_battery_percentage,
Do these j_ types convert seamlessly and properly (edge cases included) to their
associated C++ types?  I suspect so, since these seem to have compiled, but I
thought I'd check and make sure they are identical instead of just close.

https://codereview.chromium.org/2064323004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:50:
static_cast<net::NetworkChangeNotifier::ConnectionType>(
Is this cast safe?  (ie, are we guaranteed the enums are identical in Java and
C++ lands?)

https://cs.chromium.org/chromium/src/third_party/webrtc/api/java/android/org/...

does not seem to match

https://cs.chromium.org/chromium/src/net/base/network_change_notifier.h?q=net...

2G comes first in one scheme, last in another...

[dougarnett, 2016-06-16 18:47:24]

dougarnett@chromium.org changed reviewers:
+ fgorski@chromium.org
- petewil@google.com

[dougarnett, 2016-06-16 18:47:25]

Adding fgorski to review.

[dougarnett, 2016-06-16 21:03:07]

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

https://codereview.chromium.org/2064323004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:213:
NetworkChangeNotifier.getInstance().getCurrentConnectionType());
On 2016/06/16 17:46:26, Pete Williamson wrote:
> So what Does the NetworkChangeNotifier return if we have both 2G and WiFi? 
> Maybe the most recently connected of the two?  I suspect we'd really prefer to
> know about WiFi if both are connected, since it will be the more limiting
case,
> can we do that?  (OK to add a TODO if it is hard, it looks like it might
require
> modifying NetworkChangeNotifier).

It returns the default connection type - eg, the one used unless for network
request unless a specific network type is requested for that request (something
possible with Lollipop forward). Pretty sure this is what we want for now.

For Lollipop+, it is possible to get list of connected network types
(GetConnectedNetworks) if we do want to do something fancy in the future (but
likely means having some affect over choosing what network would be used to be
interesting).

https://codereview.chromium.org/2064323004/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java
(right):

https://codereview.chromium.org/2064323004/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java:21:
public boolean startProcessing(DeviceConditions deviceConditions,
Callback<Boolean> callback) {
On 2016/06/16 17:46:26, Pete Williamson wrote:
> Might be a good idea to save the device conditions in a member variable, and
> have a getter function so we can test them.

Done.

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

https://codereview.chromium.org/2064323004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:49:
j_power_connected, j_battery_percentage,
On 2016/06/16 17:46:26, Pete Williamson wrote:
> Do these j_ types convert seamlessly and properly (edge cases included) to
their
> associated C++ types?  I suspect so, since these seem to have compiled, but I
> thought I'd check and make sure they are identical instead of just close.

Doc: http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/types.html

And an example jint => int use:
https://cs.chromium.org/chromium/src/remoting/client/jni/jni_client.cc?rcl=0&...
https://cs.chromium.org/chromium/src/remoting/client/jni/chromoting_jni_insta...

https://codereview.chromium.org/2064323004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:50:
static_cast<net::NetworkChangeNotifier::ConnectionType>(
On 2016/06/16 17:46:26, Pete Williamson wrote:
> Is this cast safe?  (ie, are we guaranteed the enums are identical in Java and
> C++ lands?)
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/api/java/android/org/...
> 
> does not seem to match
> 
>
https://cs.chromium.org/chromium/src/net/base/network_change_notifier.h?q=net...
> 
> 2G comes first in one scheme, last in another...

This reference generated java org.chromium.net.ConnectionType enum is what I
mean to be used on the java side:
https://cs.chromium.org/chromium/src/net/base/network_change_notifier.h?rcl=0...
and OfflinePageUtils.java is retrieving the type value from
NetworkChangeNotifier.getInstance().getCurrentConnectionType() so it should
match exactly the NetworkChangeNotifier::ConnectionType here.

Note sure how the webrtc one that you first linked is related.

[Pete Williamson, 2016-06-16 22:43:57]

lgtm

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

https://codereview.chromium.org/2064323004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:213:
NetworkChangeNotifier.getInstance().getCurrentConnectionType());
On 2016/06/16 21:03:07, dougarnett wrote:
> On 2016/06/16 17:46:26, Pete Williamson wrote:
> > So what Does the NetworkChangeNotifier return if we have both 2G and WiFi? 
> > Maybe the most recently connected of the two?  I suspect we'd really prefer
to
> > know about WiFi if both are connected, since it will be the more limiting
> case,
> > can we do that?  (OK to add a TODO if it is hard, it looks like it might
> require
> > modifying NetworkChangeNotifier).
> 
> It returns the default connection type - eg, the one used unless for network
> request unless a specific network type is requested for that request
(something
> possible with Lollipop forward). Pretty sure this is what we want for now.
> 
> For Lollipop+, it is possible to get list of connected network types
> (GetConnectedNetworks) if we do want to do something fancy in the future (but
> likely means having some affect over choosing what network would be used to be
> interesting).

Acknowledged.

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

https://codereview.chromium.org/2064323004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:50:
static_cast<net::NetworkChangeNotifier::ConnectionType>(
On 2016/06/16 21:03:07, dougarnett wrote:
> On 2016/06/16 17:46:26, Pete Williamson wrote:
> > Is this cast safe?  (ie, are we guaranteed the enums are identical in Java
and
> > C++ lands?)
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/api/java/android/org/...
> > 
> > does not seem to match
> > 
> >
>
https://cs.chromium.org/chromium/src/net/base/network_change_notifier.h?q=net...
> > 
> > 2G comes first in one scheme, last in another...
> 
> This reference generated java org.chromium.net.ConnectionType enum is what I
> mean to be used on the java side:
>
https://cs.chromium.org/chromium/src/net/base/network_change_notifier.h?rcl=0...
> and OfflinePageUtils.java is retrieving the type value from
> NetworkChangeNotifier.getInstance().getCurrentConnectionType() so it should
> match exactly the NetworkChangeNotifier::ConnectionType here.
> 
> Note sure how the webrtc one that you first linked is related.

Ah, I didn't notice that it was from webrtc, I thought it was the Java version
of the same thing.  Ack.

[dougarnett, 2016-06-17 16:55:46]

Going to try to land. Filip, happy to follow up on any feedback in another patch
if you get chance to look at this.

[dougarnett, 2016-06-17 16:55:56]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-06-17 16:56:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064323004/80001

[commit-bot: I haz the power, 2016-06-17 17:02:49]

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

[commit-bot: I haz the power, 2016-06-17 17:02:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dougarnett, 2016-06-17 17:22:16]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-06-17 17:22:16]

The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2064323004/#ps100001
(title: "merge")

[commit-bot: I haz the power, 2016-06-17 17:22:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064323004/100001

[commit-bot: I haz the power, 2016-06-17 18:05:13]

Description was changed from

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=619686
==========

to

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=619686
==========

[commit-bot: I haz the power, 2016-06-17 18:05:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-17 18:06:46]

Description was changed from

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=619686
==========

to

==========
Defines initial DeviceConditions and and plumbs down through StartProcessing()
call.

BUG=619686

Committed: https://crrev.com/445fd2b04e36fc3df963b42a07636d48fe552aad
Cr-Commit-Position: refs/heads/master@{#400455}
==========

[commit-bot: I haz the power, 2016-06-17 18:06:48]

Patchset 6 (id:??) landed as
https://crrev.com/445fd2b04e36fc3df963b42a07636d48fe552aad
Cr-Commit-Position: refs/heads/master@{#400455}