[Offline pages] Updates to background scheduling to use BTS

[Offline pages] Updates to background scheduling to use BTS

This patch moves offline page scheduling for pre-M OSs to
background_task_scheduler component and removes a lot of code
in the process:
* switches offline page handling in ChromeBackgroundService to
  reschedule
* removes ChromeBackgroundserviceWaiter
* removes subclasses of BackgroundScheduler (responsible for
  scheduling on different OS versions)
* Moves useful code from BackgroundOfflinerTask to
  OfflineBackgroundTask
* Simplifies BackgroundSchedulerProcessor (and removes
  namespace for interfaces)
* Updates the tests for classes mentioned above.

BUG=699261
Review-Url: https://codereview.chromium.org/2830843002
Cr-Commit-Position: refs/heads/master@{#476352}
Committed: https://chromium.googlesource.com/chromium/src/+/c3cf4c7d10c4796ef841fc181d0207cfdc4ad252

[fgorski, 2017-04-19 19:38:45]

Patchset #1 (id:1) has been deleted

[fgorski, 2017-04-19 21:52:12]

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

[commit-bot: I haz the power, 2017-04-19 21:52:52]

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-04-19 22:33:22]

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

[commit-bot: I haz the power, 2017-04-19 22:33:23]

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_...)

[fgorski, 2017-05-22 22:56:54]

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

[commit-bot: I haz the power, 2017-05-22 22:57:32]

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-05-23 00:33:23]

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

[commit-bot: I haz the power, 2017-05-23 00:33:24]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-05-23 20:02:50]

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

[commit-bot: I haz the power, 2017-05-23 20:03:54]

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-05-23 20:09:15]

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

[commit-bot: I haz the power, 2017-05-23 20:09:16]

Dry run: 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_...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[fgorski, 2017-05-23 20:35:13]

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

[commit-bot: I haz the power, 2017-05-23 20:36:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2017-05-23 21:12:17]

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

[commit-bot: I haz the power, 2017-05-23 21:13:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2017-05-23 22:29:18]

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

[fgorski, 2017-05-23 22:29:18]

PTAL.

This is heads up, as this is targeting 60.
I'll add a few more tests for OfflineBackgroundTask (native portion) and proper
description tomorrow, but you can work things out from titles of patches.

Thanks.
Filip

[commit-bot: I haz the power, 2017-05-24 00:32:02]

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

[commit-bot: I haz the power, 2017-05-24 00:32:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Pete Williamson, 2017-05-24 21:41:03]

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(left):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:37:
* If HOLD_WAKELOCK is set to true in a bundle in the task params, then the
ChromeBackgroundService
How come we don't need a wakelock anymore?  (and what is preventing the
background service from continuing on to the next task?)

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:50:
// Offline pages are migrating to BackgroundTaskScheduler, therefor getting
nit: therefor -> therefore

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:68:
return true;
Should this be doing something besides returning true?

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:110:
return bridge.startScheduledProcessing(deviceConditions, callback);
By the way, not that I'm suggesting it, but one option you have is to modify the
code to not pass the conditions down at all, but make the RC responsible for
that.  The RC already has code to get conditions for subsequent attempts, we
could just make it always get the code.

The drawback of that is that getting the conditions is an async (and perhaps
slow) step as we go out to Java land to get it, so the code is slightly faster
(and has less lag) as it is now.

I'm just mentioning this so that if it would make a lot cleaner on the Java
side, you know you have an option.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:120:
if (!areBatteryConditionsMet(deviceConditions, triggerConditions)) {
This seems to run counter to the comment at the top of this function warning us
that checkConditions should be called first.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java_so...
File chrome/android/java_sources.gni (left):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java_so...
chrome/android/java_sources.gni:1681:
"junit/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiterTest.java",
Do we need a new test to replace this, or is it already tested by the new
library wrapping both DCM and JobScheduler?

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:43:
false /* power */, 0 /* battery percentage */, false /* unmetered */);
Let's clarify this comment - does not require unmetered network

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:73:
assertTrue(info.requiresCharging());
It's a bit unfortunate that we use Charging in some places and Power elsewhere. 
Should we change them all to be the same?

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:88:
public void testScheduleMetered() {
You're really testing metered and power here, not just metered (same with the
previous method which is testing unmetered and no power required).  Consider if
changing the test names to reflect that makes sense.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:116:
public void testScheduleBackup() {
There is a lot of duplication in these tests - can they be refactored?

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java:133:
// Set battery percentage below minimum level, but connect batter.
batter -> power

[fgorski, 2017-05-24 23:09:51]

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

[fgorski, 2017-05-24 23:10:11]

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

[fgorski, 2017-05-24 23:10:13]

Addressed.
Adding Tommy.

PTAL

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(left):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:37:
* If HOLD_WAKELOCK is set to true in a bundle in the task params, then the
ChromeBackgroundService
On 2017/05/24 21:41:02, Pete Williamson wrote:
> How come we don't need a wakelock anymore?  (and what is preventing the
> background service from continuing on to the next task?)

Offline pages was the only code that used it and corresponding wake lock is
available through BackgroundTaskScheduler.
See:
https://cs.chromium.org/chromium/src/components/background_task_scheduler/and...
for reference. Code was inspired by this code.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:50:
// Offline pages are migrating to BackgroundTaskScheduler, therefor getting
On 2017/05/24 21:41:02, Pete Williamson wrote:
> nit: therefor -> therefore

Done.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:68:
return true;
On 2017/05/24 21:41:03, Pete Williamson wrote:
> Should this be doing something besides returning true?

Nope. There is nothing to do if native was not implemented.
Given that the task was started and haven't finished, we presume that we'll need
to reschedule.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:110:
return bridge.startScheduledProcessing(deviceConditions, callback);
On 2017/05/24 21:41:03, Pete Williamson wrote:
> By the way, not that I'm suggesting it, but one option you have is to modify
the
> code to not pass the conditions down at all, but make the RC responsible for
> that.  The RC already has code to get conditions for subsequent attempts, we
> could just make it always get the code.
> 
> The drawback of that is that getting the conditions is an async (and perhaps
> slow) step as we go out to Java land to get it, so the code is slightly faster
> (and has less lag) as it is now.
> 
> I'm just mentioning this so that if it would make a lot cleaner on the Java
> side, you know you have an option.

This is probably a good material for a follow up, but not critical in this
refactoring.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java:120:
if (!areBatteryConditionsMet(deviceConditions, triggerConditions)) {
On 2017/05/24 21:41:03, Pete Williamson wrote:
> This seems to run counter to the comment at the top of this function warning
us
> that checkConditions should be called first.

We are inside of checkConditions() here, starting line 115. If that is not what
you were concerned about, let me know.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java_so...
File chrome/android/java_sources.gni (left):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/java_so...
chrome/android/java_sources.gni:1681:
"junit/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiterTest.java",
On 2017/05/24 21:41:03, Pete Williamson wrote:
> Do we need a new test to replace this, or is it already tested by the new
> library wrapping both DCM and JobScheduler?

Already covered by new stuff.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:43:
false /* power */, 0 /* battery percentage */, false /* unmetered */);
On 2017/05/24 21:41:03, Pete Williamson wrote:
> Let's clarify this comment - does not require unmetered network

Done.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:73:
assertTrue(info.requiresCharging());
On 2017/05/24 21:41:03, Pete Williamson wrote:
> It's a bit unfortunate that we use Charging in some places and Power
elsewhere. 
> Should we change them all to be the same?

Good idea. Added a task for that.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:88:
public void testScheduleMetered() {
On 2017/05/24 21:41:03, Pete Williamson wrote:
> You're really testing metered and power here, not just metered (same with the
> previous method which is testing unmetered and no power required).  Consider
if
> changing the test names to reflect that makes sense.

Done.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:116:
public void testScheduleBackup() {
On 2017/05/24 21:41:03, Pete Williamson wrote:
> There is a lot of duplication in these tests - can they be refactored?

Done.

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java:133:
// Set battery percentage below minimum level, but connect batter.
On 2017/05/24 21:41:03, Pete Williamson wrote:
> batter -> power

Done.

[commit-bot: I haz the power, 2017-05-24 23:10:48]

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-05-25 03:01:38]

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

[commit-bot: I haz the power, 2017-05-25 03:01:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Pete Williamson, 2017-05-25 17:30:54]

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:116:
public void testScheduleBackup() {
On 2017/05/24 23:10:13, fgorski wrote:
> On 2017/05/24 21:41:03, Pete Williamson wrote:
> > There is a lot of duplication in these tests - can they be refactored?
> 
> Done.

The refactoring is good, thanks!

If you wanted, you might be able to take the refactoring one step further - if
you pass in as arguments to VerifyTaskInfoValues whether or not you require
charging or network, you could have VerifyTaskInfoValues check those too.

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:103:
// These 2 lines are different.
This comment can probably be removed now that a common function takes care of
all the stuff that is the same (same for line 129)

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java
(right):

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java:151:
new OfflineBackgroundTask().onStartTaskBeforeNativeLoaded(
It feels a bit odd to me to have the function under test called inside the
assert statement, which is supposed to check the output of the statement under
test.  I can understand wanting to save having an extra variable by not having
to store the return value in a variable, but in my opinion, all these tests
would be much easier to read if you structured it like this:

TaskResultStatusEnum result = new
OfflineBackgroundTask().onStartTaskBeforeNativeLoaded(...);
assertEquals(NativeBackgroundTask.RESCHEDULE, result);

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java:153:
verify(mTaskFinishedCallback, times(0)).taskFinished(anyBoolean());
It seems a bit counter intuitive that you are verifying the callback didn't run.
Maybe add comment:
// If native is not loaded, ensure the task never tried to run.

[fgorski, 2017-05-25 21:41:35]

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

[fgorski, 2017-05-25 21:41:50]

PTAL

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2830843002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:116:
public void testScheduleBackup() {
On 2017/05/25 17:30:53, Pete Williamson wrote:
> On 2017/05/24 23:10:13, fgorski wrote:
> > On 2017/05/24 21:41:03, Pete Williamson wrote:
> > > There is a lot of duplication in these tests - can they be refactored?
> > 
> > Done.
> 
> The refactoring is good, thanks!
> 
> If you wanted, you might be able to take the refactoring one step further - if
> you pass in as arguments to VerifyTaskInfoValues whether or not you require
> charging or network, you could have VerifyTaskInfoValues check those too.

I thought about it and decided not to, because I prefer to see the assert
failing inside of the test.
It is not about having as little code as possible, but rather to be able to
pin-point the failure.
Are you OK with that?

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java
(right):

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:103:
// These 2 lines are different.
On 2017/05/25 17:30:53, Pete Williamson wrote:
> This comment can probably be removed now that a common function takes care of
> all the stuff that is the same (same for line 129)

Done.

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java
(right):

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java:151:
new OfflineBackgroundTask().onStartTaskBeforeNativeLoaded(
On 2017/05/25 17:30:53, Pete Williamson wrote:
> It feels a bit odd to me to have the function under test called inside the
> assert statement, which is supposed to check the output of the statement under
> test.  I can understand wanting to save having an extra variable by not having
> to store the return value in a variable, but in my opinion, all these tests
> would be much easier to read if you structured it like this:
> 
> TaskResultStatusEnum result = new
> OfflineBackgroundTask().onStartTaskBeforeNativeLoaded(...);
> assertEquals(NativeBackgroundTask.RESCHEDULE, result);

I applied that, but only to the calls returning a result other than boolean.

https://codereview.chromium.org/2830843002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java:153:
verify(mTaskFinishedCallback, times(0)).taskFinished(anyBoolean());
On 2017/05/25 17:30:53, Pete Williamson wrote:
> It seems a bit counter intuitive that you are verifying the callback didn't
run.
> Maybe add comment:
> // If native is not loaded, ensure the task never tried to run.

Done.

[commit-bot: I haz the power, 2017-05-25 21:42:12]

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-05-25 23:32:31]

All changes look good, there is one remaining issue: How do we ensure that if
chrome is killed at any point in the sequence, we don't lose the fact that we
are trying to offline a page (we always have a backup task scheduled).

[commit-bot: I haz the power, 2017-05-26 01:56:55]

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

[commit-bot: I haz the power, 2017-05-26 01:56:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fgorski, 2017-05-26 22:18:59]

On 2017/05/25 23:32:31, Pete Williamson wrote:
> All changes look good, there is one remaining issue: How do we ensure that if
> chrome is killed at any point in the sequence, we don't lose the fact that we
> are trying to offline a page (we always have a backup task scheduled).

For pre-M scheduling the backup Task does not affect us, regardless of when
it is scheduled, because it is guaranteed not to execute.

https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...

"""
Once a task is running, it will not be stopped. You can change the schedule for
a task by rescheduling using the same tag and service class, however if a task
with the same identity is currently running then it will be allowed to finish.
The new schedule then takes place, regardless of the value that the old task
returned.

Per service, two tasks with the same tag will never execute concurrently. Should
a newly-scheduled task arrive while an existing task is still running, the new
task will be dropped. In addition, different tasks may be scheduled
concurrently, each invoked in a separate thread, if their schedules overlap. It
is up to you to ensure thread safety when scheduling multiple tasks.
"""

For M+ scheduling we have a problem, because scheduling with the ID of executing
job may acutally force stop the current task, per:

https://developer.android.com/reference/android/app/job/JobScheduler.html#sch...

"""
Schedule a job to be executed. Will replace any currently scheduled job with the
same ID with the new information in the JobInfo. If a job with the given ID is
currently running, it will be stopped.
"""

Having observed the latter we filed http://crbug.com/726860 and fix for it is
testing on the bots now.

The fix is to actually ignore the calls that overwrite, if there is a pending
job.
I still don't know if running job is considered to be a pending job, but I will
test it.

Either way, it does not seem like we can consistently solve the problems of
crashing/swiping out/memory-crunch-eviction when background processing using a
backup schedule (at least with the same job id).

I believe the best long term option is to explore alternative solutions.
(Including at the BTS component level.)

I can move the code up to always schedule backup for now, which will guarantee
at least the same behavior as today.

WDYT?

[nyquist, 2017-05-30 18:45:26]

lgtm, but could you expand the CL description?

For bug fixes I often suggest something along the lines of one paragraph each
for answers to the following questions:

- What is currently happening?
- Why is that bad?
- How is this CL making it better?

Of course, you're changing the underlying framework for scheduling here, but in
general, maybe they could provide some guidance?

https://codereview.chromium.org/2830843002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2830843002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:26:
public static BackgroundScheduler getInstance() {
Nit: I know this is just refactored code, and not new, but it feels like this
should be name createInstance(), or we should change it to only ever create it
once, for example with the LazyHolder pattern.

[Pete Williamson, 2017-05-30 19:54:31]

lgtm

We chatted offline, and this looks good, with one potential issue, that looking
at the docs, JobScheduler will kill the current job if we try to reschedule a
backup task, although it doesn't happen in practice, we should understand why. 
FGorski will open a bug for that.

[fgorski, 2017-05-30 20:15:06]

Opened bug 727832 and bug 727841 to track the extra work items.

[fgorski, 2017-05-31 16:17:50]

Description was changed from

==========
[Offline pages] Updates to background scheduling to use BTS

BUG=
==========

to

==========
[Offline pages] Updates to background scheduling to use BTS

This patch moves offline page scheduling for pre-M OSs to
background_task_scheduler component and removes a lot of code
in the process:
* switches offline page handling in ChromeBackgroundService to
  reschedule
* removes ChromeBackgroundserviceWaiter
* removes subclasses of BackgroundScheduler (responsible for
  scheduling on different OS versions)
* Moves useful code from BackgroundOfflinerTask to
  OfflineBackgroundTask
* Simplifies BackgroundSchedulerProcessor (and removes
  namespace for interfaces)
* Updates the tests for classes mentioned above.

BUG=699261
==========

[fgorski, 2017-05-31 16:37:01]

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

[commit-bot: I haz the power, 2017-05-31 16:38:02]

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-05-31 19:23:20]

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

[commit-bot: I haz the power, 2017-05-31 19:23:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[fgorski, 2017-06-01 16:24:10]

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

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

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2017-06-01 16:26:26]

Pete, I made a small tweak to DeviceConditions, as I discovered in manual
testing that it could cause a crash.

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

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

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

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-06-01 18:03:20]

lgtm

still lgtm

[fgorski, 2017-06-01 18:03:53]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2017-06-01 18:03:53]

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/2830843002/#ps240001
(title: "Fixing the crash on NCN not being initialized")

[commit-bot: I haz the power, 2017-06-01 18:04:28]

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-06-01 18:10:20]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1496340233078860,
"parent_rev": "4f63947164ec18f2eaed90dd1192d593dad51781", "commit_rev":
"c3cf4c7d10c4796ef841fc181d0207cfdc4ad252"}

[commit-bot: I haz the power, 2017-06-01 18:10:29]

Description was changed from

==========
[Offline pages] Updates to background scheduling to use BTS

This patch moves offline page scheduling for pre-M OSs to
background_task_scheduler component and removes a lot of code
in the process:
* switches offline page handling in ChromeBackgroundService to
  reschedule
* removes ChromeBackgroundserviceWaiter
* removes subclasses of BackgroundScheduler (responsible for
  scheduling on different OS versions)
* Moves useful code from BackgroundOfflinerTask to
  OfflineBackgroundTask
* Simplifies BackgroundSchedulerProcessor (and removes
  namespace for interfaces)
* Updates the tests for classes mentioned above.

BUG=699261
==========

to

==========
[Offline pages] Updates to background scheduling to use BTS

This patch moves offline page scheduling for pre-M OSs to
background_task_scheduler component and removes a lot of code
in the process:
* switches offline page handling in ChromeBackgroundService to
  reschedule
* removes ChromeBackgroundserviceWaiter
* removes subclasses of BackgroundScheduler (responsible for
  scheduling on different OS versions)
* Moves useful code from BackgroundOfflinerTask to
  OfflineBackgroundTask
* Simplifies BackgroundSchedulerProcessor (and removes
  namespace for interfaces)
* Updates the tests for classes mentioned above.

BUG=699261

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

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

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/c3cf4c7d10c4796ef841fc181d02...