[Android] Adding scheduling through GcmNetworkManager

[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager (with junit tests).
* Adds code to covert from task to TaskParams (GCM)
* Adds GcmTaskService implementation with a waiter to handle tasks
  taking time to execute (with junit)
* Adds ability to store tasks in prefs for upgrade scenario (+junit)

BUG=710630
R=nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2779753002
Cr-Commit-Position: refs/heads/master@{#471919}
Committed: https://chromium.googlesource.com/chromium/src/+/7c52dcabdaefff4b583999c786afbefa907564fb

[fgorski, 2017-03-27 18:04:31]

Description was changed from

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager.
* Adds code to covert from task to TaskParams (GCM)
* Adds a waiter to handle tasks taking time.

BUG=680757
==========

to

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager.
* Adds code to covert from task to TaskParams (GCM)
* Adds a waiter to handle tasks taking time.

BUG=680757
R=nyquist@chromium.org
==========

[fgorski, 2017-03-27 18:04:31]

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

[fgorski, 2017-03-28 21:08:20]

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

[commit-bot: I haz the power, 2017-03-28 21:08: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-03-28 21:09:32]

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

[commit-bot: I haz the power, 2017-03-28 21:09:32]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[fgorski, 2017-03-28 21:55:22]

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

[commit-bot: I haz the power, 2017-03-28 21:59:18]

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-03-28 22:17:08]

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

[commit-bot: I haz the power, 2017-03-28 22:17:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fgorski, 2017-04-11 16:50:37]

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

[commit-bot: I haz the power, 2017-04-11 16:51:20]

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-11 19:18:35]

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

[commit-bot: I haz the power, 2017-04-11 19:18:36]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-04-11 19:33:03]

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

[commit-bot: I haz the power, 2017-04-11 19:33:38]

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

[fgorski, 2017-04-11 19:42:01]

Description was changed from

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager.
* Adds code to covert from task to TaskParams (GCM)
* Adds a waiter to handle tasks taking time.

BUG=680757
R=nyquist@chromium.org
==========

to

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager (with junit tests).
* Adds code to covert from task to TaskParams (GCM)
* Adds GcmTaskService implementation with a waiter to handle tasks
  taking time to execute (with junit)
* Adds ability to store tasks in prefs for upgrade scenario (+junit)

BUG=680757
R=nyquist@chromium.org
==========

[fgorski, 2017-04-11 19:42:11]

PTAL

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:72:
// TODO(fgorski): Does this need to happen on UI thread?
Tommy?

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java:89:
// TODO(fgorski): Check what is the default for JobScheduler. TaskInfo may need
to
This is about how we have the defaults set...
We may need to explicitly require this be set, in which case we ignore the API
defaults.

[commit-bot: I haz the power, 2017-04-11 20:45:20]

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

[commit-bot: I haz the power, 2017-04-11 20:45:21]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-04-11 21:58:26]

Description was changed from

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager (with junit tests).
* Adds code to covert from task to TaskParams (GCM)
* Adds GcmTaskService implementation with a waiter to handle tasks
  taking time to execute (with junit)
* Adds ability to store tasks in prefs for upgrade scenario (+junit)

BUG=680757
R=nyquist@chromium.org
==========

to

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager (with junit tests).
* Adds code to covert from task to TaskParams (GCM)
* Adds GcmTaskService implementation with a waiter to handle tasks
  taking time to execute (with junit)
* Adds ability to store tasks in prefs for upgrade scenario (+junit)

BUG=710630
R=nyquist@chromium.org
==========

[nyquist, 2017-04-18 21:58:28]

This looks great!

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:72:
// TODO(fgorski): Does this need to happen on UI thread?
On 2017/04/11 19:42:10, fgorski wrote:
> Tommy?

This will set Waiter#mIsRescheduleNeeded, which is checked on the UI thread in
onRunTask(...), so conceptually yes.
But! Good news everyone! AFAIK CountDownLatch has a happens-before guarantee
where actions you take on one thread that calls countDown() will happen-before
returning from await(). So no, this can happen on any thread.

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:99:
if (taskNeedsBackgroundProcessing) {
I did try looking at making this less indented by:


if (!taskNeedsBackgroundProcessing) return GcmNetworkManager.RESULT_SUCCESS;

waiter.startWaiting();

if (waiter.isRescheduleNeeded()) return GcmNetworkManager.RESULT_RESCHEDULE;


And then continuing with the haskTaskTimedOut() block, but I'm not sure what I
think.
It's definitely easy to read. It's just that this inner-if-thing got a bit long
in the current style.

I don't feel strongly though. Feel free to keep as is.

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:100:
waiter.startWaiting();
I can't see from the documentation at
https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...
whether onRunTask(..) is guaranteed to never run on the main thread.

I see that they mention that with multiple tasks executing at the same time,
they are never scheduled on the same thread. But they don't mention whether the
main / UI thread is ever in use.

If that is a guarantee they have, could you point to it? And if it's an
assumption we make from looking at the source or otherwise assuming, could you
add a comment?

For JobService, we get the call to onStartJob(...) on the main thread:
https://developer.android.com/reference/android/app/job/JobService.html
"This service executes each incoming job on a Handler running on your
application's main thread. This means that you must offload your execution logic
to another thread/handler/AsyncTask of your choosing."

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java:89:
// TODO(fgorski): Check what is the default for JobScheduler. TaskInfo may need
to
On 2017/04/11 19:42:11, fgorski wrote:
> This is about how we have the defaults set...
> We may need to explicitly require this be set, in which case we ignore the API
> defaults.

Yeah, would be great to look into this before we announce.

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java:64:
Set<String> prefsEntries = prefs.getStringSet(KEY_SCHEDULED_TASKS, new
HashSet<String>(1));
Why do you create the set with an initial size of 1?

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java:67:
String[] entryParts = entry.split(ENTRY_SEPARATOR);
Could all of |entry| ever be null by accident?

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java:68:
if (entryParts.length != 2 || entryParts[0] == null || entryParts[0].isEmpty())
{
Should we verify whether entryParts[1] is a parsable int?

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefsTest.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefsTest.java:27:
public class BackgroundTaskSchedulerPrefsTest {
Given the format of how this is stored, could you also add a test for same
BackgroundTask.class, but multiple TaskIds?

[fgorski, 2017-04-20 05:34:46]

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

[commit-bot: I haz the power, 2017-04-20 05:35:14]

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

[fgorski, 2017-04-20 05:53:32]

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

[commit-bot: I haz the power, 2017-04-20 05:53:51]

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

[fgorski, 2017-04-20 05:54:16]

PTAL

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:72:
// TODO(fgorski): Does this need to happen on UI thread?
On 2017/04/18 21:58:28, nyquist wrote:
> On 2017/04/11 19:42:10, fgorski wrote:
> > Tommy?
> 
> This will set Waiter#mIsRescheduleNeeded, which is checked on the UI thread in
> onRunTask(...), so conceptually yes.
> But! Good news everyone! AFAIK CountDownLatch has a happens-before guarantee
> where actions you take on one thread that calls countDown() will happen-before
> returning from await(). So no, this can happen on any thread.

I'll keep it consistent with conceptual presumptions and remove TODO

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:99:
if (taskNeedsBackgroundProcessing) {
On 2017/04/18 21:58:28, nyquist wrote:
> I did try looking at making this less indented by:
> 
> 
> if (!taskNeedsBackgroundProcessing) return GcmNetworkManager.RESULT_SUCCESS;
> 
> waiter.startWaiting();
> 
> if (waiter.isRescheduleNeeded()) return GcmNetworkManager.RESULT_RESCHEDULE;
> 
> 
> And then continuing with the haskTaskTimedOut() block, but I'm not sure what I
> think.
> It's definitely easy to read. It's just that this inner-if-thing got a bit
long
> in the current style.
> 
> I don't feel strongly though. Feel free to keep as is.

I split the big if into 3 statements. Let me know if how you feel about that.
(Might be an overkill, but you are right that it is easier to follow the logic
that way).

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:100:
waiter.startWaiting();
On 2017/04/18 21:58:28, nyquist wrote:
> I can't see from the documentation at
>
https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...
> whether onRunTask(..) is guaranteed to never run on the main thread.
> 
> I see that they mention that with multiple tasks executing at the same time,
> they are never scheduled on the same thread. But they don't mention whether
the
> main / UI thread is ever in use.
> 
> If that is a guarantee they have, could you point to it? And if it's an
> assumption we make from looking at the source or otherwise assuming, could you
> add a comment?
> 
> For JobService, we get the call to onStartJob(...) on the main thread:
> https://developer.android.com/reference/android/app/job/JobService.html
> "This service executes each incoming job on a Handler running on your
> application's main thread. This means that you must offload your execution
logic
> to another thread/handler/AsyncTask of your choosing."

I'll test it and will get back to you. Perhaps my presumption of Main thread
execution is wrong.
The difference from job scheduler is the way they can preempt you, and probably
that is why they expect main thread to be available to handle stop event. If the
presumption here is wrong we could run into problems, perhaps around the waiter
release. I'll play with that.

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java:89:
// TODO(fgorski): Check what is the default for JobScheduler. TaskInfo may need
to
On 2017/04/18 21:58:28, nyquist wrote:
> On 2017/04/11 19:42:11, fgorski wrote:
> > This is about how we have the defaults set...
> > We may need to explicitly require this be set, in which case we ignore the
API
> > defaults.
> 
> Yeah, would be great to look into this before we announce.

JobInfo or Builder documentation does not mention it explicitly. I presume it is
off by default and since we don't want to leak stuff, let's document that by
default we don't persist (expecting consumers to set it). WDYT?

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java:64:
Set<String> prefsEntries = prefs.getStringSet(KEY_SCHEDULED_TASKS, new
HashSet<String>(1));
On 2017/04/18 21:58:28, nyquist wrote:
> Why do you create the set with an initial size of 1?

I saw that pattern someplace in code. This is to create the smallest possible
set as default in case the prefs does not have anything, which seems to be rare
in our case and therefore I don't want to be using whatever the default size is.
Does that make sense?

I'll gladly remove that 1 or switch to null and initialize properly as a
separate step.

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java:67:
String[] entryParts = entry.split(ENTRY_SEPARATOR);
On 2017/04/18 21:58:28, nyquist wrote:
> Could all of |entry| ever be null by accident?

That should never happen... (famous last words)
Because I update task prefs handling in rescheduling patch I'll gladly address
it there if you don't mind. (Otherwise I will happily cherry-pick between
patches.
https://codereview.chromium.org/2819703002/diff/20001/components/background_t...

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefs.java:68:
if (entryParts.length != 2 || entryParts[0] == null || entryParts[0].isEmpty())
{
On 2017/04/18 21:58:28, nyquist wrote:
> Should we verify whether entryParts[1] is a parsable int?

Ditto:
https://codereview.chromium.org/2819703002/diff/20001/components/background_t...

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefsTest.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerPrefsTest.java:27:
public class BackgroundTaskSchedulerPrefsTest {
On 2017/04/18 21:58:28, nyquist wrote:
> Given the format of how this is stored, could you also add a test for same
> BackgroundTask.class, but multiple TaskIds?

Done.

[commit-bot: I haz the power, 2017-04-20 07:05:24]

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

[commit-bot: I haz the power, 2017-04-20 07:05:25]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-04-25 20:44:16]

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:100:
waiter.startWaiting();
On 2017/04/20 05:54:15, fgorski wrote:
> On 2017/04/18 21:58:28, nyquist wrote:
> > I can't see from the documentation at
> >
>
https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...
> > whether onRunTask(..) is guaranteed to never run on the main thread.
> > 
> > I see that they mention that with multiple tasks executing at the same time,
> > they are never scheduled on the same thread. But they don't mention whether
> the
> > main / UI thread is ever in use.
> > 
> > If that is a guarantee they have, could you point to it? And if it's an
> > assumption we make from looking at the source or otherwise assuming, could
you
> > add a comment?
> > 
> > For JobService, we get the call to onStartJob(...) on the main thread:
> > https://developer.android.com/reference/android/app/job/JobService.html
> > "This service executes each incoming job on a Handler running on your
> > application's main thread. This means that you must offload your execution
> logic
> > to another thread/handler/AsyncTask of your choosing."
> 
> I'll test it and will get back to you. Perhaps my presumption of Main thread
> execution is wrong.
> The difference from job scheduler is the way they can preempt you, and
probably
> that is why they expect main thread to be available to handle stop event. If
the
> presumption here is wrong we could run into problems, perhaps around the
waiter
> release. I'll play with that.

Did you figure out which thread this runs on?

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java:89:
// TODO(fgorski): Check what is the default for JobScheduler. TaskInfo may need
to
On 2017/04/20 05:54:15, fgorski wrote:
> On 2017/04/18 21:58:28, nyquist wrote:
> > On 2017/04/11 19:42:11, fgorski wrote:
> > > This is about how we have the defaults set...
> > > We may need to explicitly require this be set, in which case we ignore the
> API
> > > defaults.
> > 
> > Yeah, would be great to look into this before we announce.
> 
> JobInfo or Builder documentation does not mention it explicitly. I presume it
is
> off by default and since we don't want to leak stuff, let's document that by
> default we don't persist (expecting consumers to set it). WDYT?

sgtm

[fgorski, 2017-04-26 17:21:32]

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

[commit-bot: I haz the power, 2017-04-26 17:22:01]

Dry run: CQ is trying da patch.

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

[fgorski, 2017-04-26 17:22:35]

PTAL.

Let me know if this is what you had in mind.

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java
(right):

https://codereview.chromium.org/2779753002/diff/120001/components/background_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskService.java:100:
waiter.startWaiting();
On 2017/04/25 20:44:15, nyquist wrote:
> On 2017/04/20 05:54:15, fgorski wrote:
> > On 2017/04/18 21:58:28, nyquist wrote:
> > > I can't see from the documentation at
> > >
> >
>
https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...
> > > whether onRunTask(..) is guaranteed to never run on the main thread.
> > > 
> > > I see that they mention that with multiple tasks executing at the same
time,
> > > they are never scheduled on the same thread. But they don't mention
whether
> > the
> > > main / UI thread is ever in use.
> > > 
> > > If that is a guarantee they have, could you point to it? And if it's an
> > > assumption we make from looking at the source or otherwise assuming, could
> you
> > > add a comment?
> > > 
> > > For JobService, we get the call to onStartJob(...) on the main thread:
> > > https://developer.android.com/reference/android/app/job/JobService.html
> > > "This service executes each incoming job on a Handler running on your
> > > application's main thread. This means that you must offload your execution
> > logic
> > > to another thread/handler/AsyncTask of your choosing."
> > 
> > I'll test it and will get back to you. Perhaps my presumption of Main thread
> > execution is wrong.
> > The difference from job scheduler is the way they can preempt you, and
> probably
> > that is why they expect main thread to be available to handle stop event. If
> the
> > presumption here is wrong we could run into problems, perhaps around the
> waiter
> > release. I'll play with that.
> 
> Did you figure out which thread this runs on?

Done. Running on blocking ui thread now.

[commit-bot: I haz the power, 2017-04-26 19:35:12]

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

[commit-bot: I haz the power, 2017-04-26 19:35:13]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-05-10 05:05:36]

Tommy, can you take a look? I don't know that there are issues that haven't been
addressed yet.

[nyquist, 2017-05-12 22:49:13]

lgtm

[fgorski, 2017-05-12 23:02:29]

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

[fgorski, 2017-05-12 23:02:30]

John, PTAL at testing/buildbot changes, please

[fgorski, 2017-05-12 23:16:01]

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

[commit-bot: I haz the power, 2017-05-12 23:17:02]

Dry run: CQ is trying da patch.

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

[jbudorick, 2017-05-12 23:53:22]

On 2017/05/12 23:02:30, fgorski wrote:
> John, PTAL at testing/buildbot changes, please

lgtm

[commit-bot: I haz the power, 2017-05-13 00:30:28]

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

[commit-bot: I haz the power, 2017-05-13 00:30:30]

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

[fgorski, 2017-05-15 17:35:18]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2017-05-15 17:35:52]

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-15 20:41:35]

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

[commit-bot: I haz the power, 2017-05-15 20:41:37]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fgorski, 2017-05-15 20:57:21]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2017-05-15 20:58:11]

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-15 22:15:24]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494881841655740,
"parent_rev": "58d770aa09ca00cc5efa938255f830eed587cb65", "commit_rev":
"7c52dcabdaefff4b583999c786afbefa907564fb"}

[commit-bot: I haz the power, 2017-05-15 22:15:41]

Description was changed from

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager (with junit tests).
* Adds code to covert from task to TaskParams (GCM)
* Adds GcmTaskService implementation with a waiter to handle tasks
  taking time to execute (with junit)
* Adds ability to store tasks in prefs for upgrade scenario (+junit)

BUG=710630
R=nyquist@chromium.org
==========

to

==========
[Android] Adding scheduling through GcmNetworkManager

This patch:
* Adds ability to schedule using GcmNetworkManager (with junit tests).
* Adds code to covert from task to TaskParams (GCM)
* Adds GcmTaskService implementation with a waiter to handle tasks
  taking time to execute (with junit)
* Adds ability to store tasks in prefs for upgrade scenario (+junit)

BUG=710630
R=nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2779753002
Cr-Commit-Position: refs/heads/master@{#471919}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c52dcabdaefff4b583999c786af...
==========

[commit-bot: I haz the power, 2017-05-15 22:15:42]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/7c52dcabdaefff4b583999c786af...