[Offline pages] Implementation of OfflineBackgroundTask using background_task_scheduler

[Offline pages] Implementation of OfflineBackgroundTask using background_task_scheduler

This patch makes updates to offline pages background scheduling to
enabel use of background_task_scheduler when scheduling for M+ (using JobScheduler):
* BackgroundJobScheduler uses the new component to schedule task, 
* BackgroundOfflinerTask#startBackgroundRequest is refactored to
  enable handling both JobScheduler, and GCMNetworkManager task
  handling.
* Implementation is provided in:
  BackgroundOfflinerTask#startBackgroundRequestImpl
* BackgroundSchedulerBridge adds a method to stopProcessing
* The call is passed to RequestCoordinator, using a newly added
  Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED
* Handling of PersistableBundle is removed from TaskExtrasPacker
  as this functionality is handled by background_task_scheduler

BUG=682802
R=petewil@chromium.org,nyquist@chromium.org
TBR=holte@chromium.org

Review-Url: https://codereview.chromium.org/2697493002
Cr-Commit-Position: refs/heads/master@{#453966}
Committed: https://chromium.googlesource.com/chromium/src/+/40b86413cfabfcb66588b7d6f5fe430c19403659

[Pete Williamson, 2017-02-21 18:43:43]

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

https://codereview.chromium.org/2697493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:57:
/** TODO(fgorski): add documentation
Let's add the doc now instead of later.  Why do we overload this?  Maybe one is
for use by GCM, one for JS?

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

https://codereview.chromium.org/2697493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerJobService.java:47:
* that we should be rescheduling the task
Rescheduling is a good idea, but the RC is giving the thread back.  We'd have to
set up a timer in the RC to wake up and reschedule.  We should add a TODO for
that in the RC code.

[fgorski, 2017-02-28 01:16:08]

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

[fgorski, 2017-02-28 01:16:42]

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

[commit-bot: I haz the power, 2017-02-28 01:16:43]

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

[fgorski, 2017-02-28 02:42:48]

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

[commit-bot: I haz the power, 2017-02-28 02:43:29]

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

[fgorski, 2017-02-28 02:45:40]

PTAL.

I'll fix the DEPS and clang compilation of junit tests tomorrow, but overall the
code is there to be looked at.

[commit-bot: I haz the power, 2017-02-28 04:17:53]

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

[commit-bot: I haz the power, 2017-02-28 04:17:54]

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

[nyquist, 2017-02-28 06:17:53]

It looks like some of the CL description refers to JobScheduler. Would you want
to see if you want the new concept instead?

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

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:24:
public static final int JOB_ID = 774322033;
Can you rename this and move it to TaskIds in the component? That way we can
ensure there is no overlap.
Of course, feel free to make this one just refer to the one in the TaskIds so
you don't have to change the GCM-based implementation right now.

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:38:
if (!overwrite) {
Could you pass this in as taskInfo.setUpdateCurrent(overwrite) instead of doing
this manually here?

It reads that flag and cancels the job if it's set. Would that work for you?

https://cs.chromium.org/chromium/src/components/background_task_scheduler/and...

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

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:63:
// assert taskParameters.getTaskId() == BackgroundJobScheduler.JOB_ID;
Remove or uncomment?

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:71:
return true;
Returning true here means that you will always be rescheduled. Is that
intentional? I mean, it's fine if it is, but I just wanted to make sure.

Also, did you mean to call and return
BackgroundSchedulerBridge#stopScheduledProcessing() here?

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:121:
private Callback<Boolean> wrapCallback(final TaskFinishedCallback callback) {
I wonder if we in the future should change the API of the
BackgroundTaskScheduler to in fact use Callback<Boolean> instead. It's just that
you can't give it a good name with a typedef like in C++, which is why I went
with a unique name for now.

[fgorski, 2017-02-28 14:40:29]

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

[commit-bot: I haz the power, 2017-02-28 14:40:44]

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

[fgorski, 2017-02-28 14:40:46]

Updated. PTAL.

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

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:24:
public static final int JOB_ID = 774322033;
On 2017/02/28 06:17:53, nyquist wrote:
> Can you rename this and move it to TaskIds in the component? That way we can
> ensure there is no overlap.
> Of course, feel free to make this one just refer to the one in the TaskIds so
> you don't have to change the GCM-based implementation right now.

Done.

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:38:
if (!overwrite) {
On 2017/02/28 06:17:53, nyquist wrote:
> Could you pass this in as taskInfo.setUpdateCurrent(overwrite) instead of
doing
> this manually here?
> 
> It reads that flag and cancels the job if it's set. Would that work for you?
> 
>
https://cs.chromium.org/chromium/src/components/background_task_scheduler/and...

Done.

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

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:63:
// assert taskParameters.getTaskId() == BackgroundJobScheduler.JOB_ID;
On 2017/02/28 06:17:53, nyquist wrote:
> Remove or uncomment?

Done.

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:71:
return true;
On 2017/02/28 06:17:53, nyquist wrote:
> Returning true here means that you will always be rescheduled. Is that
> intentional? I mean, it's fine if it is, but I just wanted to make sure.
> 
> Also, did you mean to call and return
> BackgroundSchedulerBridge#stopScheduledProcessing() here?

Correct on both accounts.
1. if we don't give up on our own, it means there is more work and we'll always
want to schedule a new task.
2. I meant to call stopScheduledProcessing and missed that.

Done.

https://codereview.chromium.org/2697493002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:121:
private Callback<Boolean> wrapCallback(final TaskFinishedCallback callback) {
On 2017/02/28 06:17:53, nyquist wrote:
> I wonder if we in the future should change the API of the
> BackgroundTaskScheduler to in fact use Callback<Boolean> instead. It's just
that
> you can't give it a good name with a typedef like in C++, which is why I went
> with a unique name for now.

As you can see there are ways around it like here. I am fine both ways since I
solve it without a surgery on all the files.
I would be happy with Callback<Boolean> of course.

[commit-bot: I haz the power, 2017-02-28 16:02:46]

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

[commit-bot: I haz the power, 2017-02-28 16:02:47]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-02-28 17:50:37]

Description was changed from

==========
[Offline pages] Adding a JobService implementaiton of request scheduling

This patch adds implementation of JobService handling offline page
jobs dealing with requests to offline Web pages.
* Jobs are received by BackgroundSchedulerJobService#onStartJob, and
* Handled by BackgroundOfflinerTask#startBackgroundRequest, which
  unwraps the arguments and passes them to
* BackgroundOfflinerTask#startBackgroundRequestImpl, which was updated
  to handle common cases between GcmNetworkManager and JobScheduler,
  e.g. (Persistent)Bundle needs to be unwrapped outside of the method
  because it is OS version dependent.

BUG=682802
R=petewil@chromium.org,nyquist@chromium.org
==========

to

==========
[Offline pages] Implementation of OfflineBackgroundTask using
background_task_scheduler

This patch makes updates to offline pages background scheduling to
enabel use of background_task_scheduler when scheduling for M+ (using
JobScheduler):
* BackgroundJobScheduler uses the new component to schedule task, 
* BackgroundOfflinerTask#startBackgroundRequest is refactored to
  enable handling both JobScheduler, and GCMNetworkManager task
  handling.
* Implementation is provided in:
  BackgroundOfflinerTask#startBackgroundRequestImpl
* BackgroundSchedulerBridge adds a method to stopProcessing
* The call is passed to RequestCoordinator, using a newly added
  Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED
* Handling of PersistableBundle is removed from TaskExtrasPacker
  as this functionality is handled by background_task_scheduler

BUG=682802
R=petewil@chromium.org,nyquist@chromium.org
==========

[nyquist, 2017-02-28 18:25:44]

lgtm

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
Nit: 2017

[Pete Williamson, 2017-02-28 20:17:49]

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:34:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
Weren't we planning on starting with O, and then backing off to M sometime
later?

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:37:
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
How come we don't need these anymore?  Just curiuos.

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:70:
Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED);
After thinking about it and looking at the current UMA, this looks good. 
Remember to update our Background Loading UMA pointers doc:
https://docs.google.com/document/d/1b__WjQPPnltJWHgCi-KoOC4ucEGRewQiCLPkM3CA-TE

https://codereview.chromium.org/2697493002/diff/60001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java
(right):

https://codereview.chromium.org/2697493002/diff/60001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java:17:
public static final int OFFLINE_PAGES_BACKGROUND_JOB_ID = 77;
The one we had before was very large and random - does that mean that the job id
needs to be unique chrome-wide?

[fgorski, 2017-02-28 21:23:20]

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

[fgorski, 2017-02-28 21:24:08]

fgorski@chromium.org changed reviewers:
+ holte@chromium.org

[fgorski, 2017-02-28 21:24:09]

Addressed.

Adding Steven for histograms.

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:34:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
On 2017/02/28 20:17:48, Pete Williamson wrote:
> Weren't we planning on starting with O, and then backing off to M sometime
> later?

Updated to N based on discussion with you and Tommy.

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/02/28 18:25:44, nyquist wrote:
> Nit: 2017

Done.

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:37:
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
On 2017/02/28 20:17:48, Pete Williamson wrote:
> How come we don't need these anymore?  Just curiuos.

The consumed component abstracts that and we get the Bundle instead of
PersistableBundle. I added that information to the CL description.

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

https://codereview.chromium.org/2697493002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_scheduler_bridge.cc:70:
Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED);
On 2017/02/28 20:17:49, Pete Williamson wrote:
> After thinking about it and looking at the current UMA, this looks good. 
> Remember to update our Background Loading UMA pointers doc:
>
https://docs.google.com/document/d/1b__WjQPPnltJWHgCi-KoOC4ucEGRewQiCLPkM3CA-TE

Done.

https://codereview.chromium.org/2697493002/diff/60001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java
(right):

https://codereview.chromium.org/2697493002/diff/60001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java:17:
public static final int OFFLINE_PAGES_BACKGROUND_JOB_ID = 77;
On 2017/02/28 20:17:49, Pete Williamson wrote:
> The one we had before was very large and random - does that mean that the job
id
> needs to be unique chrome-wide?

Yes, and all of them will be here. 77 sounds great.

[commit-bot: I haz the power, 2017-02-28 21:24:19]

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-02-28 21:25:39]

lgtm

[commit-bot: I haz the power, 2017-02-28 22:45:48]

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

[commit-bot: I haz the power, 2017-02-28 22:45:49]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-03-01 17:48:49]

Description was changed from

==========
[Offline pages] Implementation of OfflineBackgroundTask using
background_task_scheduler

This patch makes updates to offline pages background scheduling to
enabel use of background_task_scheduler when scheduling for M+ (using
JobScheduler):
* BackgroundJobScheduler uses the new component to schedule task, 
* BackgroundOfflinerTask#startBackgroundRequest is refactored to
  enable handling both JobScheduler, and GCMNetworkManager task
  handling.
* Implementation is provided in:
  BackgroundOfflinerTask#startBackgroundRequestImpl
* BackgroundSchedulerBridge adds a method to stopProcessing
* The call is passed to RequestCoordinator, using a newly added
  Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED
* Handling of PersistableBundle is removed from TaskExtrasPacker
  as this functionality is handled by background_task_scheduler

BUG=682802
R=petewil@chromium.org,nyquist@chromium.org
==========

to

==========
[Offline pages] Implementation of OfflineBackgroundTask using
background_task_scheduler

This patch makes updates to offline pages background scheduling to
enabel use of background_task_scheduler when scheduling for M+ (using
JobScheduler):
* BackgroundJobScheduler uses the new component to schedule task, 
* BackgroundOfflinerTask#startBackgroundRequest is refactored to
  enable handling both JobScheduler, and GCMNetworkManager task
  handling.
* Implementation is provided in:
  BackgroundOfflinerTask#startBackgroundRequestImpl
* BackgroundSchedulerBridge adds a method to stopProcessing
* The call is passed to RequestCoordinator, using a newly added
  Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED
* Handling of PersistableBundle is removed from TaskExtrasPacker
  as this functionality is handled by background_task_scheduler

BUG=682802
R=petewil@chromium.org,nyquist@chromium.org
TBR=holte@chromium.org
==========

[fgorski, 2017-03-01 17:49:54]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2017-03-01 17:49:55]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2697493002/#ps80001
(title: "Addressing more feedback")

[commit-bot: I haz the power, 2017-03-01 17:50: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, 2017-03-01 17:56:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488390594857240,
"parent_rev": "eddd0d4f361b16d62eae38b198d6cedde1626186", "commit_rev":
"40b86413cfabfcb66588b7d6f5fe430c19403659"}

[commit-bot: I haz the power, 2017-03-01 17:57:18]

Description was changed from

==========
[Offline pages] Implementation of OfflineBackgroundTask using
background_task_scheduler

This patch makes updates to offline pages background scheduling to
enabel use of background_task_scheduler when scheduling for M+ (using
JobScheduler):
* BackgroundJobScheduler uses the new component to schedule task, 
* BackgroundOfflinerTask#startBackgroundRequest is refactored to
  enable handling both JobScheduler, and GCMNetworkManager task
  handling.
* Implementation is provided in:
  BackgroundOfflinerTask#startBackgroundRequestImpl
* BackgroundSchedulerBridge adds a method to stopProcessing
* The call is passed to RequestCoordinator, using a newly added
  Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED
* Handling of PersistableBundle is removed from TaskExtrasPacker
  as this functionality is handled by background_task_scheduler

BUG=682802
R=petewil@chromium.org,nyquist@chromium.org
TBR=holte@chromium.org
==========

to

==========
[Offline pages] Implementation of OfflineBackgroundTask using
background_task_scheduler

This patch makes updates to offline pages background scheduling to
enabel use of background_task_scheduler when scheduling for M+ (using
JobScheduler):
* BackgroundJobScheduler uses the new component to schedule task, 
* BackgroundOfflinerTask#startBackgroundRequest is refactored to
  enable handling both JobScheduler, and GCMNetworkManager task
  handling.
* Implementation is provided in:
  BackgroundOfflinerTask#startBackgroundRequestImpl
* BackgroundSchedulerBridge adds a method to stopProcessing
* The call is passed to RequestCoordinator, using a newly added
  Offliner::RequestStatus::BACKGROUND_SCHEDULER_CANCELED
* Handling of PersistableBundle is removed from TaskExtrasPacker
  as this functionality is handled by background_task_scheduler

BUG=682802
R=petewil@chromium.org,nyquist@chromium.org
TBR=holte@chromium.org

Review-Url: https://codereview.chromium.org/2697493002
Cr-Commit-Position: refs/heads/master@{#453966}
Committed:
https://chromium.googlesource.com/chromium/src/+/40b86413cfabfcb66588b7d6f5fe...
==========

[commit-bot: I haz the power, 2017-03-01 17:57:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/40b86413cfabfcb66588b7d6f5fe...