Defines TriggerConditions and plumbs up to BackgroundScheduler

Defines TriggerConditions and plumbs up to BackgroundScheduler
but does not yet use for the GcmNetworkManager Task (which will be in follow-up patch).

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

[dougarnett, 2016-06-20 22:01:32]

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

[fgorski, 2016-06-20 22:46:54]

mostly looks good.

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TriggerConditions.java
(right):

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TriggerConditions.java:14:
* Creates set of device network and power conditions for triggering processing.
device, network

https://codereview.chromium.org/2087633002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2087633002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:46:
scheduler.schedule(mContext, new TriggerConditions(true, 10, false));
nit: this will quickly get unreadable.
Is there a way to explain the boolean values?

This applies to same problem on c++ side.

[Pete Williamson, 2016-06-20 23:14:03]

Overall looks good, a few comments.

I do wonder, though, if we should do this at all until we have a use case for
the conditions to be different (predictive fetching for Now).

One reason we may want to is if we want to be able to change the policy on the
fly, so I would definitely be open to arguments that we need it for that use
case.

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
(right):

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:44:
@CalledByNative
We might document *why* we have a createTriggerContidions function instead of
just calling the constructor (so we can create the java object from native
code).

https://codereview.chromium.org/2087633002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2087633002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:46:
scheduler.schedule(mContext, new TriggerConditions(true, 10, false));
On 2016/06/20 22:46:54, fgorski wrote:
> nit: this will quickly get unreadable.
> Is there a way to explain the boolean values?
> 
> This applies to same problem on c++ side.

To make things a little bit better, we could create the test trigger conditions
once in the setUp method, and document them there.

We can create boolean constants REQUIRE_WALL_POWER (true), NO_WALL_POWER_NEEDED
(false) and use them at the point of creating a TriggerConditions object.

Another potential approach is to use enums instead of boolean values, but I
prefer using boolean values here since we have to send them over JNI.

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:205:
EXPECT_EQ(50, scheduler_stub->conditions()->minimum_battery_percentage);
Where did this 50 come from?  I didn't see a corresponding change in the test to
set the battery percentage to 50.

[dougarnett, 2016-06-22 19:54:18]

<guess it would help if I published my drafts>

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java
(right):

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:44:
@CalledByNative
On 2016/06/20 23:14:03, Pete Williamson wrote:
> We might document *why* we have a createTriggerContidions function instead of
> just calling the constructor (so we can create the java object from native
> code).

Done.

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TriggerConditions.java
(right):

https://codereview.chromium.org/2087633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TriggerConditions.java:14:
* Creates set of device network and power conditions for triggering processing.
On 2016/06/20 22:46:54, fgorski wrote:
> device, network

Done.

https://codereview.chromium.org/2087633002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2087633002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:46:
scheduler.schedule(mContext, new TriggerConditions(true, 10, false));
On 2016/06/20 23:14:03, Pete Williamson wrote:
> On 2016/06/20 22:46:54, fgorski wrote:
> > nit: this will quickly get unreadable.
> > Is there a way to explain the boolean values?
> > 
> > This applies to same problem on c++ side.
> 
> To make things a little bit better, we could create the test trigger
conditions
> once in the setUp method, and document them there.
> 
> We can create boolean constants REQUIRE_WALL_POWER (true),
NO_WALL_POWER_NEEDED
> (false) and use them at the point of creating a TriggerConditions object.
> 
> Another potential approach is to use enums instead of boolean values, but I
> prefer using boolean values here since we have to send them over JNI.

Commented ctor parms and consolidated into single member var for both tests.

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:205:
EXPECT_EQ(50, scheduler_stub->conditions()->minimum_battery_percentage);
On 2016/06/20 23:14:03, Pete Williamson wrote:
> Where did this 50 come from?  I didn't see a corresponding change in the test
to
> set the battery percentage to 50.

This value is from the single constant kUserRequestTriggerConditions added to
coordinator class (line 22). I could expose that constant and check here - I was
hesitant to do that because I expect we might want to not have that constant
there and instead have the Policy object providing the TriggerConditions.

[Pete Williamson, 2016-06-22 20:49:11]

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:205:
EXPECT_EQ(50, scheduler_stub->conditions()->minimum_battery_percentage);
On 2016/06/22 19:54:18, dougarnett wrote:
> On 2016/06/20 23:14:03, Pete Williamson wrote:
> > Where did this 50 come from?  I didn't see a corresponding change in the
test
> to
> > set the battery percentage to 50.
> 
> This value is from the single constant kUserRequestTriggerConditions added to
> coordinator class (line 22). I could expose that constant and check here - I
was
> hesitant to do that because I expect we might want to not have that constant
> there and instead have the Policy object providing the TriggerConditions. 

How about we make the constant a public constant in RequestCoordinator, and use
that for now.

Eventually, when this moves into policy, we can put the number we want into the
policy object we use at test set up time.

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

https://codereview.chromium.org/2087633002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:24: 50 /*
minimum_battery_percentage */,
25 might be better than 50 here.
It might also be good to have a public symbolic constant for the 50, and put it
in the offline_pages namespace instead of the unnamed namespace.  Tests can then
refer to the constant from here instead of using a bare 50 in the test.

[dougarnett, 2016-06-22 21:52:17]

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2087633002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:205:
EXPECT_EQ(50, scheduler_stub->conditions()->minimum_battery_percentage);
On 2016/06/22 20:49:11, Pete Williamson wrote:
> On 2016/06/22 19:54:18, dougarnett wrote:
> > On 2016/06/20 23:14:03, Pete Williamson wrote:
> > > Where did this 50 come from?  I didn't see a corresponding change in the
> test
> > to
> > > set the battery percentage to 50.
> > 
> > This value is from the single constant kUserRequestTriggerConditions added
to
> > coordinator class (line 22). I could expose that constant and check here - I
> was
> > hesitant to do that because I expect we might want to not have that constant
> > there and instead have the Policy object providing the TriggerConditions. 
> 
> How about we make the constant a public constant in RequestCoordinator, and
use
> that for now.
> 
> Eventually, when this moves into policy, we can put the number we want into
the
> policy object we use at test set up time.

added public method with TODO

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

https://codereview.chromium.org/2087633002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:24: 50 /*
minimum_battery_percentage */,
On 2016/06/22 20:49:11, Pete Williamson wrote:
> 25 might be better than 50 here.
> It might also be good to have a public symbolic constant for the 50, and put
it
> in the offline_pages namespace instead of the unnamed namespace.  Tests can
then
> refer to the constant from here instead of using a bare 50 in the test.

Added to the TODO comment to consider lowering it once we limit processing time.
I'm just using this value now from the constant object.

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

lgtm

[fgorski, 2016-06-22 23:34:25]

lgtm

[dougarnett, 2016-06-22 23:36:54]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-06-22 23:37:30]

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

[commit-bot: I haz the power, 2016-06-23 00:37:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-23 00:39:15]

Description was changed from

==========
Defines TriggerConditions and plumbs up to BackgroundScheduler
but does not yet use for the GcmNetworkManager Task (which will be in follow-up
patch).

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

to

==========
Defines TriggerConditions and plumbs up to BackgroundScheduler
but does not yet use for the GcmNetworkManager Task (which will be in follow-up
patch).

BUG=619686

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

[commit-bot: I haz the power, 2016-06-23 00:39:16]

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