Uses TriggerConditions in the BackgroundScheduler.

Uses TriggerConditions in the BackgroundScheduler.

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

[dougarnett, 2016-06-27 20:38:09]

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

[dougarnett, 2016-06-27 20:47:56]

Description was changed from

==========
Uses TriggerConditions in the BackgroundScheduler.

Does not yet check actual battery level in Task because that needs some unit
test support for specifying the level.

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

to

==========
Uses TriggerConditions in the BackgroundScheduler.


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

[Pete Williamson, 2016-06-28 00:03:03]

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:47:
if (currentConditions.getBatteryPercentage()
We should also consider if the device is plugged in.  If power is connected, we
don't care what the battery percentage is.  This will be OK if the
battery_percentage were to always be 100% when plugged in, but I don't know for
a fact if that happens.

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:50:
return false;
I wonder if this might trigger a race condition - suppose I just put a task with
a lower min battery percentage into the queue, but haven't yet sent it to the
scheduler when the last known task arrives.

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:27:
schedule(context, triggerConditions, 0 /* delayStartSecs */);
How about we make delayStartSecs a constant instead of documenting it this way?

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:3:
// found in the LICENSE file.
TaskExtrasPacker is getting more complex now, probably time to add a unit test.

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:34:
/** Puts trigger conditions into the input bundle. */
Perfect, this was exactly what I had in mind when I designed the class.

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TriggerConditions.java:42:
public int hashCode() {
This brings up an interesting point - I don't think any of our other classes
have hash codes, do they need them?  If so, they should go in a separate change
list.

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:42:
new TriggerConditions(false /* power */, 33 /* battery */, true /* unmetered
*/);
Can we use constants here instead of documenting the values like this?

[dougarnett, 2016-06-28 17:13:11]

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:47:
if (currentConditions.getBatteryPercentage()
On 2016/06/28 00:03:02, Pete Williamson wrote:
> We should also consider if the device is plugged in.  If power is connected,
we
> don't care what the battery percentage is.  This will be OK if the
> battery_percentage were to always be 100% when plugged in, but I don't know
for
> a fact if that happens.

Done.

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:50:
return false;
On 2016/06/28 00:03:02, Pete Williamson wrote:
> I wonder if this might trigger a race condition - suppose I just put a task
with
> a lower min battery percentage into the queue, but haven't yet sent it to the
> scheduler when the last known task arrives.

To be solid, maybe what we want is for the resulting reschedule to not overwrite
an existing schedule entry - that way, a new event scheduling its task in this
window won't be overwritten (can also be weaker network constraint as well as
weaker battery constraint).

Added TODO for now. I'm skeptical that we will have task triggered if the user
is active to add a new request. If some background processing can trigger new
request with weaker constraints, then more of a concern.

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:27:
schedule(context, triggerConditions, 0 /* delayStartSecs */);
On 2016/06/28 00:03:02, Pete Williamson wrote:
> How about we make delayStartSecs a constant instead of documenting it this
way?

This one seems pretty self contained - 0 secs is clear vs. some non-zero number
that is either tunable of a bit more complicated. You don't have to track down
what the actual value of a symbol is. I switched to this style a couple years
ago with my team as we jointly found much easier to read code for such
situations. I can put in constant if you insist but want to double check as I
think this is better.

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:3:
// found in the LICENSE file.
On 2016/06/28 00:03:02, Pete Williamson wrote:
> TaskExtrasPacker is getting more complex now, probably time to add a unit
test.

Done

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:34:
/** Puts trigger conditions into the input bundle. */
On 2016/06/28 00:03:02, Pete Williamson wrote:
> Perfect, this was exactly what I had in mind when I designed the class.

Acknowledged.

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TriggerConditions.java:42:
public int hashCode() {
On 2016/06/28 00:03:02, Pete Williamson wrote:
> This brings up an interesting point - I don't think any of our other classes
> have hash codes, do they need them?  If so, they should go in a separate
change
> list.

Only if we want to compare two instances for equality or put them in a hashed
structure. Here, I wanted equals support - but you must also implement
hashCode() if you implement equals(). I think we should implement these on
as-needed basis.

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

https://codereview.chromium.org/2094013003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:42:
new TriggerConditions(false /* power */, 33 /* battery */, true /* unmetered
*/);
On 2016/06/28 00:03:03, Pete Williamson wrote:
> Can we use constants here instead of documenting the values like this?

Done

[Pete Williamson, 2016-06-28 20:17:49]

Most changes look good, one request.

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java:27:
assertTrue(TaskExtrasPacker.unpackTimeFromBundle(taskExtras) > 0L);
Can we be a bit more rigorous here?  Maybe grab time before and after the call,
and assert that the time is between the two?

[fgorski, 2016-06-28 20:27:21]

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:44:
// TODO(dougarnett): Consider making this not overwrite an existing scheduled
nit: I know this is more of a note-to-self, but I don't understand it. Will
future you be able to?

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

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:218:
if (sDeviceConditionsForTesting != null) {
can we put it on sInstance instead? That way we only have one static thing to
work with.

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:42:
public static TriggerConditions unpackTriggerConditionsFromBundle(Bundle bundle)
{
Defaults you are using are very aggressive:
* Don't require power connection
* go as long as there is any battery
* even on a metered network...

This might backfire when things go wrong.

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:128:
public void testCallback() {
Can you remove this? It does not seem like we are doing anything inside.

TODO can stay, but may need an update in wording.

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java:33:
public void testTriggerConditionsExtra() {
please add a test for default values

[dougarnett, 2016-06-28 21:34:20]

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:44:
// TODO(dougarnett): Consider making this not overwrite an existing scheduled
On 2016/06/28 20:27:21, fgorski wrote:
> nit: I know this is more of a note-to-self, but I don't understand it. Will
> future you be able to?

ok, TODO => JUSTDOIT

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

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:218:
if (sDeviceConditionsForTesting != null) {
On 2016/06/28 20:27:21, fgorski wrote:
> can we put it on sInstance instead? That way we only have one static thing to
> work with.

ok, but PTAL to be sure you actually like that better or have some further
improvement thoughts

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:42:
public static TriggerConditions unpackTriggerConditionsFromBundle(Bundle bundle)
{
On 2016/06/28 20:27:21, fgorski wrote:
> Defaults you are using are very aggressive:
> * Don't require power connection
> * go as long as there is any battery
> * even on a metered network...
> 
> This might backfire when things go wrong.

Yes, great point

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:128:
public void testCallback() {
On 2016/06/28 20:27:21, fgorski wrote:
> Can you remove this? It does not seem like we are doing anything inside.
> 
> TODO can stay, but may need an update in wording.

Done. There is already TODO in testIncomingTask that covers this anyway.

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java
(right):

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java:27:
assertTrue(TaskExtrasPacker.unpackTimeFromBundle(taskExtras) > 0L);
On 2016/06/28 20:17:49, Pete Williamson wrote:
> Can we be a bit more rigorous here?  Maybe grab time before and after the
call,
> and assert that the time is between the two?

Done.

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java:33:
public void testTriggerConditionsExtra() {
On 2016/06/28 20:27:21, fgorski wrote:
> please add a test for default values

Done.

[fgorski, 2016-06-28 21:44:40]

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

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:218:
if (sDeviceConditionsForTesting != null) {
On 2016/06/28 21:34:19, dougarnett wrote:
> On 2016/06/28 20:27:21, fgorski wrote:
> > can we put it on sInstance instead? That way we only have one static thing
to
> > work with.
> 
> ok, but PTAL to be sure you actually like that better or have some further
> improvement thoughts

This is what I meant:
public static DeviceConditions getDeviceConditions(
  Context context) {
    getInstance().getDeviceConditions(context);
}

protected DeviceConditions getDeviceConditions(
  Context context) {
    // Your current implementation here.
}

In test you simply provide a mock offlinepageutils instance that returns what
you tell it to return. (Using setInstanceForTesting())

[Pete Williamson, 2016-06-28 21:50:57]

https://codereview.chromium.org/2094013003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java
(right):

https://codereview.chromium.org/2094013003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:44:
BackgroundScheduler.reschedule(context, previousTriggerConditions,
DEFER_START_SECONDS);
We might need to be a bit more complicated than this - What if the new task in
the queue is *more* restrictive?  By not overwriting, we end up with more
restrictions than we need.  I think perhaps what we really want here is to take
the less-restrictive of what may already be in the scheduler with rescheduling
this...

[dougarnett, 2016-06-28 22:31:38]

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

https://codereview.chromium.org/2094013003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:218:
if (sDeviceConditionsForTesting != null) {
On 2016/06/28 21:44:40, fgorski wrote:
> On 2016/06/28 21:34:19, dougarnett wrote:
> > On 2016/06/28 20:27:21, fgorski wrote:
> > > can we put it on sInstance instead? That way we only have one static thing
> to
> > > work with.
> > 
> > ok, but PTAL to be sure you actually like that better or have some further
> > improvement thoughts
> 
> This is what I meant:
> public static DeviceConditions getDeviceConditions(
>   Context context) {
>     getInstance().getDeviceConditions(context);
> }
> 
> protected DeviceConditions getDeviceConditions(
>   Context context) {
>     // Your current implementation here.
> }
> 
> In test you simply provide a mock offlinepageutils instance that returns what
> you tell it to return. (Using setInstanceForTesting())

Done.

https://codereview.chromium.org/2094013003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java
(right):

https://codereview.chromium.org/2094013003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:44:
BackgroundScheduler.reschedule(context, previousTriggerConditions,
DEFER_START_SECONDS);
On 2016/06/28 21:50:57, Pete Williamson wrote:
> We might need to be a bit more complicated than this - What if the new task in
> the queue is *more* restrictive?  By not overwriting, we end up with more
> restrictions than we need.  I think perhaps what we really want here is to
take
> the less-restrictive of what may already be in the scheduler with rescheduling
> this...

No, this is purely a backup here in case Android kills our process. The
Coordinator is responsible for setting the correct conditions for what is
prioritized in the queue (not just the most recent queue entry). [If it sounds
like I'm missing your concern here, lets get on VC so I can better understand
your point.]

[Pete Williamson, 2016-06-28 22:40:17]

lgtm

https://codereview.chromium.org/2094013003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java
(right):

https://codereview.chromium.org/2094013003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:44:
BackgroundScheduler.reschedule(context, previousTriggerConditions,
DEFER_START_SECONDS);
On 2016/06/28 22:31:38, dougarnett wrote:
> On 2016/06/28 21:50:57, Pete Williamson wrote:
> > We might need to be a bit more complicated than this - What if the new task
in
> > the queue is *more* restrictive?  By not overwriting, we end up with more
> > restrictions than we need.  I think perhaps what we really want here is to
> take
> > the less-restrictive of what may already be in the scheduler with
rescheduling
> > this...
> 
> No, this is purely a backup here in case Android kills our process. The
> Coordinator is responsible for setting the correct conditions for what is
> prioritized in the queue (not just the most recent queue entry). [If it sounds
> like I'm missing your concern here, lets get on VC so I can better understand
> your point.] 

Ah, right, I forgot that the coordinator is combining conditions already, sol
anything in the queue should be current.

[fgorski, 2016-06-28 22:44:18]

lgtm. Thanks for updating OfflinePageUtils.

[dougarnett, 2016-06-28 22:48:26]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-06-28 22:51:22]

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-06-29 00:29:19]

Description was changed from

==========
Uses TriggerConditions in the BackgroundScheduler.


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

to

==========
Uses TriggerConditions in the BackgroundScheduler.


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

[commit-bot: I haz the power, 2016-06-29 00:29:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-29 00:31:21]

Description was changed from

==========
Uses TriggerConditions in the BackgroundScheduler.


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

to

==========
Uses TriggerConditions in the BackgroundScheduler.

BUG=619686

Committed: https://crrev.com/9cd96b27236383f64203b41df75b86fb7b841e23
Cr-Commit-Position: refs/heads/master@{#402613}
==========

[commit-bot: I haz the power, 2016-06-29 00:31:23]

Patchset 8 (id:??) landed as
https://crrev.com/9cd96b27236383f64203b41df75b86fb7b841e23
Cr-Commit-Position: refs/heads/master@{#402613}