[Background Sync] Use GcmNetworkManager to start the browser for sync events.

[Background Sync] Use GcmNetworkManager to start the browser for sync events.

This replaces the old BackgroundSyncLauncherService with one that
extends GcmTaskService, pulls in Play Services for Chrome on Android,
and uses the GCM network manager to schedule a one-off task to restart
the browser when the network comes back online if a one-shot background
sync is registered when the browser is stopped.

It also adds the RECEIVE_BOOT_COMPLETED permission to the Chrome Android
manifest, so that registered sync events can persist across device reboots.
(This is a non-dangerous permission)

This requires Google Play Services 7.5, which is a dependency of Chromium as of 0c50961:
https://crrev.com/0c50961006cd76b2a20f445a6f296535664e777d

BUG=491137
Committed: https://crrev.com/8467f460d6cf1b2b5c71664d952819e2b3b58b85
Cr-Commit-Position: refs/heads/master@{#355284}

[iclelland, 2015-09-02 15:43:53]

iclelland@chromium.org changed reviewers:
+ jkarlin@chromium.org

[iclelland, 2015-09-02 17:55:11]

jkarlin -- take a look, it's still in WIP (tests especially are going to be
fun), but it wakes up the browser when stopped, and even after reboot.

[jkarlin, 2015-09-02 18:16:37]

So cool!

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
Is this needed?

https://codereview.chromium.org/1324173002/diff/20001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1324173002/diff/20001/content/content.gyp#new...
content/content.gyp:445:
'../third_party/android_tools/android_tools.gyp:google_play_services_javalib',
This should be updated in a gn BUILD file somewhere as well, right?

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:67:
private void scheduleLaunchTask() {
It appears that this can be called multiple times, resulting in multiple tasks.
Should we check for this? Or perhaps removeScheduledTasks() first?

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:125:
.putBoolean(PREF_BACKGROUND_SYNC_LAUNCH_NEXT_ONLINE, shouldLaunch)
Do we still need this preference?

[iclelland, 2015-09-02 20:04:49]

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/02 18:16:36, jkarlin wrote:
> Is this needed?

Yes, in order to be triggered by the GcmNetworkManager after the device reboots,
the browser needs this permission. (I'm not sure about the other manifest
files).

The permission itself is granted silently: there are no additional permission
upgrade dialogs from the play store, for instance; it just has to be requested
here.

https://codereview.chromium.org/1324173002/diff/20001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1324173002/diff/20001/content/content.gyp#new...
content/content.gyp:445:
'../third_party/android_tools/android_tools.gyp:google_play_services_javalib',
On 2015/09/02 18:16:36, jkarlin wrote:
> This should be updated in a gn BUILD file somewhere as well, right?

Very likely. I'll take a look to see what the gn story is here.

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:67:
private void scheduleLaunchTask() {
On 2015/09/02 18:16:36, jkarlin wrote:
> It appears that this can be called multiple times, resulting in multiple
tasks.
> Should we check for this? Or perhaps removeScheduledTasks() first?

If it is called when there is already a task scheduled, the re-use of the same
tag ("BackgroundSync Event") means that the existing task will just be updated.

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:125:
.putBoolean(PREF_BACKGROUND_SYNC_LAUNCH_NEXT_ONLINE, shouldLaunch)
On 2015/09/02 18:16:36, jkarlin wrote:
> Do we still need this preference?

I wasn't certain. I was still tracing through the code to see when and how it
was used. Is there any situation where the user starts the browser (rather than
the GcmNetworkManager starting it) where we'd need to check this preference?

Or in that case, does the background sync manager just always run through its
list of registrations when browser starts?

If the preference can be removed, then that should clear up a lot of other code
as well.

[jkarlin, 2015-09-03 11:11:57]

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/02 20:04:48, iclelland wrote:
> On 2015/09/02 18:16:36, jkarlin wrote:
> > Is this needed?
> 
> Yes, in order to be triggered by the GcmNetworkManager after the device
reboots,
> the browser needs this permission. (I'm not sure about the other manifest
> files).
> 
> The permission itself is granted silently: there are no additional permission
> upgrade dialogs from the play store, for instance; it just has to be requested
> here.

It should be in all of the manifests then.

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:67:
private void scheduleLaunchTask() {
On 2015/09/02 20:04:48, iclelland wrote:
> On 2015/09/02 18:16:36, jkarlin wrote:
> > It appears that this can be called multiple times, resulting in multiple
> tasks.
> > Should we check for this? Or perhaps removeScheduledTasks() first?
> 
> If it is called when there is already a task scheduled, the re-use of the same
> tag ("BackgroundSync Event") means that the existing task will just be
updated.

Acknowledged.

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:125:
.putBoolean(PREF_BACKGROUND_SYNC_LAUNCH_NEXT_ONLINE, shouldLaunch)
On 2015/09/02 20:04:48, iclelland wrote:
> On 2015/09/02 18:16:36, jkarlin wrote:
> > Do we still need this preference?
> 
> I wasn't certain. I was still tracing through the code to see when and how it
> was used. Is there any situation where the user starts the browser (rather
than
> the GcmNetworkManager starting it) where we'd need to check this preference?

Nope, this isn't checked by the C++ code, only by the BackgroundSyncLauncher
code.

> 
> Or in that case, does the background sync manager just always run through its
> list of registrations when browser starts?

Yep.

> 
> If the preference can be removed, then that should clear up a lot of other
code
> as well.

Definitely, I'll be happy to see it go.

[iclelland, 2015-09-03 15:52:06]

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/03 11:11:57, jkarlin wrote:
> On 2015/09/02 20:04:48, iclelland wrote:
> > On 2015/09/02 18:16:36, jkarlin wrote:
> > > Is this needed?
> > 
> > Yes, in order to be triggered by the GcmNetworkManager after the device
> reboots,
> > the browser needs this permission. (I'm not sure about the other manifest
> > files).
> > 
> > The permission itself is granted silently: there are no additional
permission
> > upgrade dialogs from the play store, for instance; it just has to be
requested
> > here.
> 
> It should be in all of the manifests then.

Done.

https://codereview.chromium.org/1324173002/diff/20001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1324173002/diff/20001/content/content.gyp#new...
content/content.gyp:445:
'../third_party/android_tools/android_tools.gyp:google_play_services_javalib',
On 2015/09/02 20:04:48, iclelland wrote:
> On 2015/09/02 18:16:36, jkarlin wrote:
> > This should be updated in a gn BUILD file somewhere as well, right?
> 
> Very likely. I'll take a look to see what the gn story is here.

Done.

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:125:
.putBoolean(PREF_BACKGROUND_SYNC_LAUNCH_NEXT_ONLINE, shouldLaunch)
On 2015/09/03 11:11:57, jkarlin wrote:
> On 2015/09/02 20:04:48, iclelland wrote:
> > On 2015/09/02 18:16:36, jkarlin wrote:
> > > Do we still need this preference?
> > 
> > I wasn't certain. I was still tracing through the code to see when and how
it
> > was used. Is there any situation where the user starts the browser (rather
> than
> > the GcmNetworkManager starting it) where we'd need to check this preference?
> 
> Nope, this isn't checked by the C++ code, only by the BackgroundSyncLauncher
> code.
> 
> > 
> > Or in that case, does the background sync manager just always run through
its
> > list of registrations when browser starts?
> 
> Yep.
> 
> > 
> > If the preference can be removed, then that should clear up a lot of other
> code
> > as well.
> 
> Definitely, I'll be happy to see it go.

Done.

[jkarlin, 2015-09-04 13:21:35]

https://codereview.chromium.org/1324173002/diff/60001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/60001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
I'm pretty sure that this is a new permission for Chrome, meaning it would delay
adoption. My guess is that it's a non-starter. We'll have to remove this in each
manifest file. :[

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:20:
*browser closes via the {@link #setLaunchWhenNextOnline} method.
while you're here, can you add a space after the above *?

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:62:
* {@link BackgroundSyncLauncher}'s singleton is created (the native browser is
started).
Any chance you could rephrase the entire above comment? It's hard to follow.

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:66:
protected void setLaunchWhenNextOnline(final boolean shouldLaunch) {
The name of this function could be clearer, perhaps
"launchBrowserWhenNextOnlineIfStopped"?

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:105:
}
I just saw this in the docs: 
Important: When Google Play Services or the client app is updated, all scheduled
tasks will be removed. GcmNetworkManager invokes the client app’s
onInitializeTasks(). Override this function to reschedule necessary tasks.
https://developers.google.com/cloud-messaging/network-manager

So we might need to keep track of the preference after all so that we can start
up the tasks again.

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java
(right):

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java:32:
// TODO(jkarlin): Protect the browser sync event with a wake lock. See
crbug.com/486020.
This comment is a bit confusing. The "onRunTask" function has a wake lock that I
believe is released as soon as RESULT_SUCCESS is returned. Which is basically
instantly. Meanwhile, the browser processes are started separately and are not
protected by the wake lock. Short events will hopefully complete before Android
sleeps again. That's what push is relying on. We'll need to add a wake lock in
the future. Some UMA would also be good to verify that we're not falling asleep
mid-sync.

[jkarlin, 2015-09-04 13:30:59]

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java
(right):

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java:34:
if (!BackgroundSyncLauncher.hasInstance()) {
hasInstance is being run on the wrong thread. It's intended to be called on the
UI thread but this task is on its own thread.

[iclelland, 2015-09-10 20:40:46]

https://codereview.chromium.org/1324173002/diff/60001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/60001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/04 13:21:35, jkarlin wrote:
> I'm pretty sure that this is a new permission for Chrome, meaning it would
delay
> adoption. My guess is that it's a non-starter. We'll have to remove this in
each
> manifest file. :[

Acknowledged -- I'll run this past some Android folks to make sure that's the
case before I do that, though.

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:62:
* {@link BackgroundSyncLauncher}'s singleton is created (the native browser is
started).
On 2015/09/04 13:21:35, jkarlin wrote:
> Any chance you could rephrase the entire above comment? It's hard to follow.

Rewritten -- hopefully clearer now.

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:66:
protected void setLaunchWhenNextOnline(final boolean shouldLaunch) {
On 2015/09/04 13:21:35, jkarlin wrote:
> The name of this function could be clearer, perhaps
> "launchBrowserWhenNextOnlineIfStopped"?

Done.

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:105:
}
On 2015/09/04 13:21:35, jkarlin wrote:
> I just saw this in the docs: 
> Important: When Google Play Services or the client app is updated, all
scheduled
> tasks will be removed. GcmNetworkManager invokes the client app’s
> onInitializeTasks(). Override this function to reschedule necessary tasks.
> https://developers.google.com/cloud-messaging/network-manager
> 
> So we might need to keep track of the preference after all so that we can
start
> up the tasks again.

I'll add a TODO to handle the update case better. This probably doesn't affect
one-shots so much, since neither play services nor chrome are likely to be
updated when the device is offline. We'll definitely have to handle this before
adding either periodic or power-sensitive one-shots.

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java
(right):

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java:32:
// TODO(jkarlin): Protect the browser sync event with a wake lock. See
crbug.com/486020.
On 2015/09/04 13:21:35, jkarlin wrote:
> This comment is a bit confusing. The "onRunTask" function has a wake lock that
I
> believe is released as soon as RESULT_SUCCESS is returned. Which is basically
> instantly. Meanwhile, the browser processes are started separately and are not
> protected by the wake lock. Short events will hopefully complete before
Android
> sleeps again. That's what push is relying on. We'll need to add a wake lock in
> the future. Some UMA would also be good to verify that we're not falling
asleep
> mid-sync.

Acknowledged. I'll see if I can verify this.

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java:34:
if (!BackgroundSyncLauncher.hasInstance()) {
On 2015/09/04 13:30:58, jkarlin wrote:
> hasInstance is being run on the wrong thread. It's intended to be called on
the
> UI thread but this task is on its own thread.

Done.

[jkarlin, 2015-09-11 18:57:55]

Sorry, I thought I sent this out much earlier

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:105:
}
On 2015/09/10 20:40:46, iclelland wrote:
> On 2015/09/04 13:21:35, jkarlin wrote:
> > I just saw this in the docs: 
> > Important: When Google Play Services or the client app is updated, all
> scheduled
> > tasks will be removed. GcmNetworkManager invokes the client app’s
> > onInitializeTasks(). Override this function to reschedule necessary tasks.
> > https://developers.google.com/cloud-messaging/network-manager
> > 
> > So we might need to keep track of the preference after all so that we can
> start
> > up the tasks again.
> 
> I'll add a TODO to handle the update case better. This probably doesn't affect
> one-shots so much, since neither play services nor chrome are likely to be
> updated when the device is offline. We'll definitely have to handle this
before
> adding either periodic or power-sensitive one-shots.

We'll also need it for retries as retry has an exponential backoff so it's
possible for a one-shot to not fire for some time after going online. Given that
we already have the code to store the preference, it probably makes sense to
restore it and add the onInitializeTasks handler in this CL.

[iclelland, 2015-09-24 13:40:53]

Preference code reinstated -- PTAL, thanks!

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:105:
}
On 2015/09/11 18:57:55, jkarlin wrote:
> On 2015/09/10 20:40:46, iclelland wrote:
> > On 2015/09/04 13:21:35, jkarlin wrote:
> > > I just saw this in the docs: 
> > > Important: When Google Play Services or the client app is updated, all
> > scheduled
> > > tasks will be removed. GcmNetworkManager invokes the client app’s
> > > onInitializeTasks(). Override this function to reschedule necessary tasks.
> > > https://developers.google.com/cloud-messaging/network-manager
> > > 
> > > So we might need to keep track of the preference after all so that we can
> > start
> > > up the tasks again.
> > 
> > I'll add a TODO to handle the update case better. This probably doesn't
affect
> > one-shots so much, since neither play services nor chrome are likely to be
> > updated when the device is offline. We'll definitely have to handle this
> before
> > adding either periodic or power-sensitive one-shots.
> 
> We'll also need it for retries as retry has an exponential backoff so it's
> possible for a one-shot to not fire for some time after going online. Given
that
> we already have the code to store the preference, it probably makes sense to
> restore it and add the onInitializeTasks handler in this CL.

Done.

[iclelland, 2015-09-24 13:41:19]

iclelland@chromium.org changed reviewers:
+ yfriedman@chromium.org

[iclelland, 2015-09-24 13:42:11]

+r yfriedman -- PTAL, thanks!

[Yaron, 2015-09-24 16:13:18]

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/20001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/03 15:52:06, iclelland wrote:
> On 2015/09/03 11:11:57, jkarlin wrote:
> > On 2015/09/02 20:04:48, iclelland wrote:
> > > On 2015/09/02 18:16:36, jkarlin wrote:
> > > > Is this needed?
> > > 
> > > Yes, in order to be triggered by the GcmNetworkManager after the device
> > reboots,
> > > the browser needs this permission. (I'm not sure about the other manifest
> > > files).
> > > 
> > > The permission itself is granted silently: there are no additional
> permission
> > > upgrade dialogs from the play store, for instance; it just has to be
> requested
> > > here.
> > 
> > It should be in all of the manifests then.
> 
> Done.

Is that only for M+ or in all cases? I see it isn't marked as dangerous so
you're probably right but adding any permissions makes me anxious given what
we've seen with update rates.

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
Also, where is it used? Within play services?

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:102:
// Google Play Services may not be up to date, if the application was not
installed through
What version of Play Services do these new dependencies require (e.g.
GcmNetworkManager)? Please also update the CL description with that.

[iclelland, 2015-09-24 17:11:50]

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/24 16:13:17, Yaron wrote:
> Also, where is it used? Within play services?

Yes, it's only used within play services -- the NetworkScheduler verifies that
the app has this permission before it allows a task which persists across
reboots to be scheduled.

See
https://developers.google.com/android/reference/com/google/android/gms/gcm/On...

(The permission itself is usually used to get an app to receive the
ACTION_BOOT_COMPLETED intent, but we're not using it for that. I presume that
the check inside of GCM is to ensure that you're not trying to "cheat" by having
play services tell you when the device rebooted, if you don't have that
permission yourself.)

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:102:
// Google Play Services may not be up to date, if the application was not
installed through
On 2015/09/24 16:13:17, Yaron wrote:
> What version of Play Services do these new dependencies require (e.g.
> GcmNetworkManager)? Please also update the CL description with that.
Done.

This requires Google Play Services 7.5, which is a dependency of Chromium as of
0c50961:

https://crrev.com/0c50961006cd76b2a20f445a6f296535664e777d
Review URL: https://codereview.chromium.org/1292753002

This comment applies only to users who have side-loaded a Chromium browser onto
their device. We could choose to display some dialog in that case, prompting
them to install Play Services, or we could just not schedule the tasks (as
happens in this CL). In that case, the BackgroundSync API will function
correctly as long as the browser is running, but if it is terminated, sync
events will not fire until it is opened again.

[jkarlin, 2015-09-24 18:08:47]

Please let me know once the tests pass and I'll take a closer look.

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/24 16:13:17, Yaron wrote:
> Also, where is it used? Within play services?

Ian: I thought we agreed that this permission had to go?

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:132:
new AsyncTask<Void, Void, Boolean>() {
I'd prefer reusing the shouldLaunchWhenNextOnline function. See
BackgroundSyncLauncherService.onOnline for an example.

[iclelland, 2015-09-24 18:33:04]

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/24 18:08:47, jkarlin wrote:
> On 2015/09/24 16:13:17, Yaron wrote:
> > Also, where is it used? Within play services?
> 
> Ian: I thought we agreed that this permission had to go?

Maybe I misunderstood -- I thought we agreed that we needed to get someone with
deeper Android understanding to review it, before deciding whether or not to
dump it.

Without this permission, we *cannot* register GCM tasks that persist across a
reboot. If a sync is scheduled, with the device offline, and the user restarts
the phone, it will be lost (applies to periodic, eventually, as well as
one-shot). We won't be able to trigger the sync event until the user manually
starts the browser again.

If that's acceptable (or a different CL), or it's at least preferable to adding
the permission, then we can remove it, and the setPersistent() call that
requires it.

[jkarlin, 2015-09-24 18:38:34]

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/24 18:33:04, iclelland wrote:
> On 2015/09/24 18:08:47, jkarlin wrote:
> > On 2015/09/24 16:13:17, Yaron wrote:
> > > Also, where is it used? Within play services?
> > 
> > Ian: I thought we agreed that this permission had to go?
> 
> Maybe I misunderstood -- I thought we agreed that we needed to get someone
with
> deeper Android understanding to review it, before deciding whether or not to
> dump it.

That's right, my mistake.

> 
> Without this permission, we *cannot* register GCM tasks that persist across a
> reboot. If a sync is scheduled, with the device offline, and the user restarts
> the phone, it will be lost (applies to periodic, eventually, as well as
> one-shot). We won't be able to trigger the sync event until the user manually
> starts the browser again.
> 
> If that's acceptable (or a different CL), or it's at least preferable to
adding
> the permission, then we can remove it, and the setPersistent() call that
> requires it.

I think it'll come down to whether or not the permission is part of a bucket
that we already have permission for.

[iclelland, 2015-09-24 18:56:19]

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/160001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:47: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/09/24 18:38:34, jkarlin wrote:
> On 2015/09/24 18:33:04, iclelland wrote:
> > On 2015/09/24 18:08:47, jkarlin wrote:
> > > On 2015/09/24 16:13:17, Yaron wrote:
> > > > Also, where is it used? Within play services?
> > > 
> > > Ian: I thought we agreed that this permission had to go?
> > 
> > Maybe I misunderstood -- I thought we agreed that we needed to get someone
> with
> > deeper Android understanding to review it, before deciding whether or not to
> > dump it.
> 
> That's right, my mistake.
> 
> > 
> > Without this permission, we *cannot* register GCM tasks that persist across
a
> > reboot. If a sync is scheduled, with the device offline, and the user
restarts
> > the phone, it will be lost (applies to periodic, eventually, as well as
> > one-shot). We won't be able to trigger the sync event until the user
manually
> > starts the browser again.
> > 
> > If that's acceptable (or a different CL), or it's at least preferable to
> adding
> > the permission, then we can remove it, and the setPersistent() call that
> > requires it.
> 
> I think it'll come down to whether or not the permission is part of a bucket
> that we already have permission for.

My understanding is that that permission does not trigger any permission dialog,
on install or on upgrade; it's a non-dangerous permission, and doesn't fall into
any of the buckets.

[iclelland, 2015-10-14 17:24:25]

I've moved the BackgroundSyncLauncher, and service, and related code into
/chrome, making a new method on the Controller. Chrome's ControllerImpl is the
only one which implements that method, so WebView and other embedders are
isolated from GCM code (although they can still use the network observer to make
use of the API in the foreground)

Tests still need to be moved, but this is manually testable with
chrome_public_apk.

jkarlin - PTAL, thanks!

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:132:
new AsyncTask<Void, Void, Boolean>() {
On 2015/09/24 18:08:47, jkarlin wrote:
> I'd prefer reusing the shouldLaunchWhenNextOnline function. See
> BackgroundSyncLauncherService.onOnline for an example.

Done. I've reinstated that method, and am using it here now.

[jkarlin, 2015-10-14 18:30:29]

Yay! Looking good so far.

https://codereview.chromium.org/1324173002/diff/280001/build/android_sdk_extr...
File build/android_sdk_extras.json (right):

https://codereview.chromium.org/1324173002/diff/280001/build/android_sdk_extr...
build/android_sdk_extras.json:5: "zip":
"google_google_play_services_27.0.0.zip",
is 27 still necessary? I thought you bumped it to 27 as an experiment.

https://codereview.chromium.org/1324173002/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:27:
@JNINamespace("chrome")
chrome code exists in the top level namespace

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
File chrome/browser/android/background_sync_launcher_android.cc (right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.cc:14: 
top level namespace

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
File chrome/browser/android/background_sync_launcher_android.h (right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.h:17: };
remove semicolon

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.h:21: 
Should be in top level namespace

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/backgro...
File chrome/browser/background_sync/background_sync_controller_impl.cc (right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/backgro...
chrome/browser/background_sync/background_sync_controller_impl.cc:36: #if
defined(OS_ANDROID)
You'll need a non-android definition of this function as well.

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/backgro...
chrome/browser/background_sync/background_sync_controller_impl.cc:39: bool
launch_when_next_online) {
DCHECK thread

https://codereview.chromium.org/1324173002/diff/280001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1324173002/diff/280001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1003:
storage_partition_impl->browser_context()->GetBackgroundSyncController();
It seems like the above code should be consolidated into a
BackgroundSyncManager::GetBackgroundSyncController function that can return
nullptr

https://codereview.chromium.org/1324173002/diff/280001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1324173002/diff/280001/content/content.gyp#ne...
content/content.gyp:445:
'../third_party/android_tools/android_tools.gyp:google_play_services_javalib',
this should go

https://codereview.chromium.org/1324173002/diff/280001/content/public/android...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/1324173002/diff/280001/content/public/android...
content/public/android/BUILD.gn:45:
"//third_party/android_tools:google_play_services_default_java",
remove

https://codereview.chromium.org/1324173002/diff/280001/content/public/android...
content/public/android/BUILD.gn:194:
"//third_party/android_tools:google_play_services_default_java",
remove

[iclelland, 2015-10-14 19:31:25]

https://codereview.chromium.org/1324173002/diff/280001/build/android_sdk_extr...
File build/android_sdk_extras.json (right):

https://codereview.chromium.org/1324173002/diff/280001/build/android_sdk_extr...
build/android_sdk_extras.json:5: "zip":
"google_google_play_services_27.0.0.zip",
On 2015/10/14 18:30:28, jkarlin wrote:
> is 27 still necessary? I thought you bumped it to 27 as an experiment.

Thanks for catching that; you're right, it shouldn't be necessary. Removed.

https://codereview.chromium.org/1324173002/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:27:
@JNINamespace("chrome")
On 2015/10/14 18:30:28, jkarlin wrote:
> chrome code exists in the top level namespace

Done.

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
File chrome/browser/android/background_sync_launcher_android.cc (right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.cc:14: 
On 2015/10/14 18:30:28, jkarlin wrote:
> top level namespace

Done.

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
File chrome/browser/android/background_sync_launcher_android.h (right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.h:17: };
On 2015/10/14 18:30:28, jkarlin wrote:
> remove semicolon

Done.

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.h:21: 
On 2015/10/14 18:30:28, jkarlin wrote:
> Should be in top level namespace

Done.

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/backgro...
File chrome/browser/background_sync/background_sync_controller_impl.cc (right):

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/backgro...
chrome/browser/background_sync/background_sync_controller_impl.cc:36: #if
defined(OS_ANDROID)
On 2015/10/14 18:30:29, jkarlin wrote:
> You'll need a non-android definition of this function as well.

Currently the non-Android version is the default null implementation from the
parent class. Since we don't have code yet to start the desktop browsers (and
that wouldn't be part of this CL anyway) should I leave a TODO here?

https://codereview.chromium.org/1324173002/diff/280001/chrome/browser/backgro...
chrome/browser/background_sync/background_sync_controller_impl.cc:39: bool
launch_when_next_online) {
On 2015/10/14 18:30:29, jkarlin wrote:
> DCHECK thread

Done.

https://codereview.chromium.org/1324173002/diff/280001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1324173002/diff/280001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1003:
storage_partition_impl->browser_context()->GetBackgroundSyncController();
On 2015/10/14 18:30:29, jkarlin wrote:
> It seems like the above code should be consolidated into a
> BackgroundSyncManager::GetBackgroundSyncController function that can return
> nullptr

Done. That's a good idea. I've refactored the other code that accesses the
controller (in NotifyBackgroundSyncRegisteredOnUIThread) to use that as well.
(And made it a function in the anonymous namespace that takes a
scoped_refptr<ServiceWorkerContextWrapper> as argument)

https://codereview.chromium.org/1324173002/diff/280001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1324173002/diff/280001/content/content.gyp#ne...
content/content.gyp:445:
'../third_party/android_tools/android_tools.gyp:google_play_services_javalib',
On 2015/10/14 18:30:29, jkarlin wrote:
> this should go

Done.

https://codereview.chromium.org/1324173002/diff/280001/content/public/android...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/1324173002/diff/280001/content/public/android...
content/public/android/BUILD.gn:45:
"//third_party/android_tools:google_play_services_default_java",
On 2015/10/14 18:30:29, jkarlin wrote:
> remove

Done.

https://codereview.chromium.org/1324173002/diff/280001/content/public/android...
content/public/android/BUILD.gn:194:
"//third_party/android_tools:google_play_services_default_java",
On 2015/10/14 18:30:29, jkarlin wrote:
> remove

Done.

[iclelland, 2015-10-15 15:36:34]

yfriedman - Can you PTAL?

[Yaron, 2015-10-15 22:18:21]

yfriedman@chromium.org changed reviewers:
+ dgn@chromium.org

[Yaron, 2015-10-15 22:18:22]

In general this looks good to me but you're stuck with the release bot and
proguard. I have to head out and can't figure out yet why proguard is breaking
things depending on gms.

+dgn who might know what's going on

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:48: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
Can you also mention the new permission in description (and identify it as
non-dangerous :)

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:135:
.setExecutionWindow(0, 1)
I would give it more than 1 second until it should launch. It's not urgent that
it runs immediately on device boot, right?

[dgn, 2015-10-16 09:05:29]

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:331:
"//third_party/android_tools:google_play_services_default_java",
please use $google_play_services_library from //build/config/android/config.gni
instead.

Shouldn't the same (gyp equivalent target is
android_tools.gyp:google_play_services_javalib) be added to
//chrome/android/chrome_apk.gyp:chrome_shared_test_java ?

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
You should use ExternalAuthUtils#canUseGooglePlayServices for that.

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java:63:
public void testNoFireWhenInstanceExists() {
Wouldn't that work as a Robolectric test? It allows advancing background tasks,
etc. I'm not sure about how it plays with play services though.

[iclelland, 2015-10-16 14:52:34]

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:331:
"//third_party/android_tools:google_play_services_default_java",
On 2015/10/16 09:05:29, dgn wrote:
> please use $google_play_services_library from
//build/config/android/config.gni
> instead.
> 
> Shouldn't the same (gyp equivalent target is
> android_tools.gyp:google_play_services_javalib) be added to
> //chrome/android/chrome_apk.gyp:chrome_shared_test_java ?

Done. and Done. Thanks for catching that.

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:48: <uses-permission
android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
On 2015/10/15 22:18:22, Yaron wrote:
> Can you also mention the new permission in description (and identify it as
> non-dangerous :)

Done.

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
On 2015/10/16 09:05:29, dgn wrote:
> You should use ExternalAuthUtils#canUseGooglePlayServices for that.

Acknowledged. I tested that API out before -- the bigger question in this case
is what an appropriate response is. We could launch a dialog, or direct the user
to the play store, but that is very intrusive UI for a feature which is supposed
to gracefully degrade. This also likely will only affect users who did not
install Chrome through the play store mechanism in the first place, and may not
even have access to it on their device. I think that the best answer here may
still be to silently fail, but I'll create a bug to consider reconsidering that.

I've added a link to crbug.com/544113

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:135:
.setExecutionWindow(0, 1)
On 2015/10/15 22:18:22, Yaron wrote:
> I would give it more than 1 second until it should launch. It's not urgent
that
> it runs immediately on device boot, right?

The execution window isn't actually used (except that it has to be of some
non-zero size) for network-sensitive-tasks, as far as I can tell. The task will
launch as soon as the GCMNetworkManager decides that the device is ready.

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java:63:
public void testNoFireWhenInstanceExists() {
On 2015/10/16 09:05:29, dgn wrote:
> Wouldn't that work as a Robolectric test? It allows advancing background
tasks,
> etc. I'm not sure about how it plays with play services though.

Acknowledged. I haven't written Robolectric tests for Chrome, but I'll look into
whether we should have a set of them for BgSync. (and whether it will work with
play services at all)

[dgn, 2015-10-16 15:31:01]

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
You can still use it and say that you want to handle error silently
(UserRecoverableErrorHandler.Silent). That way it's explicit what's happening
and you don't rely on a behaviour in some other place of the code that makes it
fail silently, or might change and crash.
About which type of handling, I commented on the bug.

[iclelland, 2015-10-16 19:12:40]

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
On 2015/10/16 15:31:01, dgn wrote:
> You can still use it and say that you want to handle error silently
> (UserRecoverableErrorHandler.Silent). That way it's explicit what's happening
> and you don't rely on a behaviour in some other place of the code that makes
it
> fail silently, or might change and crash.
> About which type of handling, I commented on the bug.

Done. The code is explicit now, silently failing, but logging to logcat.
Unfortunately, this actually reduces the functionality for users who might have
a version of play services which includes the GcmNetworkManager, but isn't
completely up-to-date. If the version doesn't match the manifest, then the call
to canUseGooglePlayServices returns false, even if the call to
GcmNetworkManager.schedule would have succeeded.

Also, that call appears to cause instrumentation test failures, saying
org.chromium.base.metrics.RecordHistogram.nativeRecordSparseHistogram isn't
available. :( I'll have to factor that out into a more testable method.

[iclelland, 2015-10-19 16:03:35]

Patchset #25 (id:480001) has been deleted

[iclelland, 2015-10-19 17:02:23]

Patchset #25 (id:500001) has been deleted

[iclelland, 2015-10-19 19:21:41]

jkarlin, yfriedmal: PTAL again, now that the tests are passing, thanks!

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
On 2015/10/16 19:12:40, iclelland wrote:
> On 2015/10/16 15:31:01, dgn wrote:
> > You can still use it and say that you want to handle error silently
> > (UserRecoverableErrorHandler.Silent). That way it's explicit what's
happening
> > and you don't rely on a behaviour in some other place of the code that makes
> it
> > fail silently, or might change and crash.
> > About which type of handling, I commented on the bug.
> 
> Done. The code is explicit now, silently failing, but logging to logcat.
> Unfortunately, this actually reduces the functionality for users who might
have
> a version of play services which includes the GcmNetworkManager, but isn't
> completely up-to-date. If the version doesn't match the manifest, then the
call
> to canUseGooglePlayServices returns false, even if the call to
> GcmNetworkManager.schedule would have succeeded.
> 
> Also, that call appears to cause instrumentation test failures, saying
> org.chromium.base.metrics.RecordHistogram.nativeRecordSparseHistogram isn't
> available. :( I'll have to factor that out into a more testable method.

Done for real. I've refactored it such that canUseGooglePlayServices is only
called when the object is created, and then only if it hasn't been explicitly
disabled for testing.

[dgn, 2015-10-19 20:01:31]

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:42:
private static boolean sGCMEnabled = true;
Nit: Why are those not javadoc comments (/** */)? Eclipse shows them on hover
when they are.

[Yaron, 2015-10-19 20:34:36]

lgtm - if you're adding more than ~100 methods (somewhat arbitrary but we're
close to the limit for debug) I'd scope the proguard change to be stricter

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
On 2015/10/16 19:12:40, iclelland wrote:
> On 2015/10/16 15:31:01, dgn wrote:
> > You can still use it and say that you want to handle error silently
> > (UserRecoverableErrorHandler.Silent). That way it's explicit what's
happening
> > and you don't rely on a behaviour in some other place of the code that makes
> it
> > fail silently, or might change and crash.
> > About which type of handling, I commented on the bug.
> 
> Done. The code is explicit now, silently failing, but logging to logcat.
> Unfortunately, this actually reduces the functionality for users who might
have
> a version of play services which includes the GcmNetworkManager, but isn't
> completely up-to-date. If the version doesn't match the manifest, then the
call
> to canUseGooglePlayServices returns false, even if the call to
> GcmNetworkManager.schedule would have succeeded.
> 
> Also, that call appears to cause instrumentation test failures, saying
> org.chromium.base.metrics.RecordHistogram.nativeRecordSparseHistogram isn't
> available. :( I'll have to factor that out into a more testable method.

Ya, you've introduced native into the mix. Either factor out the dep or change
the test's base class to
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
and load native

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/pr...
File chrome/android/java/proguard.flags (right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/pr...
chrome/android/java/proguard.flags:116: -keepnames class
com.google.android.gms.gcm.** {
How many methods does this add? You can use this tool to check:
https://github.com/mihaip/dex-method-counts

[Yaron, 2015-10-19 20:35:43]

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
On 2015/10/19 20:34:36, Yaron wrote:
> On 2015/10/16 19:12:40, iclelland wrote:
> > On 2015/10/16 15:31:01, dgn wrote:
> > > You can still use it and say that you want to handle error silently
> > > (UserRecoverableErrorHandler.Silent). That way it's explicit what's
> happening
> > > and you don't rely on a behaviour in some other place of the code that
makes
> > it
> > > fail silently, or might change and crash.
> > > About which type of handling, I commented on the bug.
> > 
> > Done. The code is explicit now, silently failing, but logging to logcat.
> > Unfortunately, this actually reduces the functionality for users who might
> have
> > a version of play services which includes the GcmNetworkManager, but isn't
> > completely up-to-date. If the version doesn't match the manifest, then the
> call
> > to canUseGooglePlayServices returns false, even if the call to
> > GcmNetworkManager.schedule would have succeeded.
> > 
> > Also, that call appears to cause instrumentation test failures, saying
> > org.chromium.base.metrics.RecordHistogram.nativeRecordSparseHistogram isn't
> > available. :( I'll have to factor that out into a more testable method.
> 
> Ya, you've introduced native into the mix. Either factor out the dep or change
> the test's base class to
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
> and load native

Err didn't mean to send this - it was from an older version.

[jkarlin, 2015-10-20 17:45:56]

lgtm with some comments.

The description needs to be updated (e.g., it pulls play services into chrome
not content).

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:24:
*browser closes via the {@link #setLaunchWhenNextOnline} method.
nit, add space between * and browser

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:117:
if (sGCMEnabled) {
I think this condition should be run after the preferences have been written to
to keep things synchronized. AsyncTask has onPostExecute for that.

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java
(right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java:76:
public void testFiresWhenInstanceNotExists() {
s/NotExists/DoesNotExist/

https://codereview.chromium.org/1324173002/diff/520001/chrome/browser/android...
File chrome/browser/android/background_sync_launcher_android.h (right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.h:16: class
BackgroundSyncManager;
nit: Add newline

[iclelland, 2015-10-20 18:55:26]

Description was changed from

==========
[Background Sync] Use GcmNetworkManager to start the browser for sync events.

This replaces the old BackgroundSyncLauncherService with one that
extends GcmTaskService, pulls in Play Services for the content module,
and uses the GCM network manager to schedule a one-off task to restart
the browser when the network comes back online if a one-shot background
sync is registered when the browser is stopped.

It also adds the RECEIVE_BOOT_COMPLETED permission to the Chrome Android
manifest, so that registered sync events can persist across device reboots.
(This is a non-dangerous permission)

This requires Google Play Services 7.5, which is a dependency of Chromium as of
0c50961:
https://crrev.com/0c50961006cd76b2a20f445a6f296535664e777d

BUG=491137
==========

to

==========
[Background Sync] Use GcmNetworkManager to start the browser for sync events.

This replaces the old BackgroundSyncLauncherService with one that
extends GcmTaskService, pulls in Play Services for Chrome on Android,
and uses the GCM network manager to schedule a one-off task to restart
the browser when the network comes back online if a one-shot background
sync is registered when the browser is stopped.

It also adds the RECEIVE_BOOT_COMPLETED permission to the Chrome Android
manifest, so that registered sync events can persist across device reboots.
(This is a non-dangerous permission)

This requires Google Play Services 7.5, which is a dependency of Chromium as of
0c50961:
https://crrev.com/0c50961006cd76b2a20f445a6f296535664e777d

BUG=491137
==========

[iclelland, 2015-10-20 18:56:06]

Review nits addressed, and the description has been updated.

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:130:
// TODO(iclelland): Check whether the Play Services client library matches the
requirements
On 2015/10/19 20:35:43, Yaron wrote:
> On 2015/10/19 20:34:36, Yaron wrote:
> > On 2015/10/16 19:12:40, iclelland wrote:
> > > On 2015/10/16 15:31:01, dgn wrote:
> > > > You can still use it and say that you want to handle error silently
> > > > (UserRecoverableErrorHandler.Silent). That way it's explicit what's
> > happening
> > > > and you don't rely on a behaviour in some other place of the code that
> makes
> > > it
> > > > fail silently, or might change and crash.
> > > > About which type of handling, I commented on the bug.
> > > 
> > > Done. The code is explicit now, silently failing, but logging to logcat.
> > > Unfortunately, this actually reduces the functionality for users who might
> > have
> > > a version of play services which includes the GcmNetworkManager, but isn't
> > > completely up-to-date. If the version doesn't match the manifest, then the
> > call
> > > to canUseGooglePlayServices returns false, even if the call to
> > > GcmNetworkManager.schedule would have succeeded.
> > > 
> > > Also, that call appears to cause instrumentation test failures, saying
> > > org.chromium.base.metrics.RecordHistogram.nativeRecordSparseHistogram
isn't
> > > available. :( I'll have to factor that out into a more testable method.
> > 
> > Ya, you've introduced native into the mix. Either factor out the dep or
change
> > the test's base class to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
> > and load native
> 
> Err didn't mean to send this - it was from an older version.

Acknowledged.

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/pr...
File chrome/android/java/proguard.flags (right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/pr...
chrome/android/java/proguard.flags:116: -keepnames class
com.google.android.gms.gcm.** {
On 2015/10/19 20:34:36, Yaron wrote:
> How many methods does this add? You can use this tool to check:
> https://github.com/mihaip/dex-method-counts

According to that tool, there are 137 methods included in the APK, under
com.google.android.gms.gcm, but only 27 of those are newly added.

With this CL, I see 45347 total methods in ChromePublicApk, including 137 under
c.g.a.gms.gcm.
Without it, I see 45323 methods, including 110 under c.g.a.gms.gcm.

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:24:
*browser closes via the {@link #setLaunchWhenNextOnline} method.
On 2015/10/20 17:45:56, jkarlin wrote:
> nit, add space between * and browser

Done.

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:42:
private static boolean sGCMEnabled = true;
On 2015/10/19 20:01:31, dgn wrote:
> Nit: Why are those not javadoc comments (/** */)? Eclipse shows them on hover
> when they are.

No good reason. Thanks for pointing it out, will fix before committing.

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:117:
if (sGCMEnabled) {
On 2015/10/20 17:45:56, jkarlin wrote:
> I think this condition should be run after the preferences have been written
to
> to keep things synchronized. AsyncTask has onPostExecute for that.

Done.

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java
(right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/BackgroundSyncLauncherServiceTest.java:76:
public void testFiresWhenInstanceNotExists() {
On 2015/10/20 17:45:56, jkarlin wrote:
> s/NotExists/DoesNotExist/

Done.

https://codereview.chromium.org/1324173002/diff/520001/chrome/browser/android...
File chrome/browser/android/background_sync_launcher_android.h (right):

https://codereview.chromium.org/1324173002/diff/520001/chrome/browser/android...
chrome/browser/android/background_sync_launcher_android.h:16: class
BackgroundSyncManager;
On 2015/10/20 17:45:56, jkarlin wrote:
> nit: Add newline 

Done.

[iclelland, 2015-10-20 19:05:33]

iclelland@chromium.org changed reviewers:
+ jochen@chromium.org

[iclelland, 2015-10-20 19:05:34]

+r jochen -- can you PTAL at the change to
content/public/browser/background_sync_controller.h ?

Thanks!

[jochen (gone - plz use gerrit), 2015-10-21 13:30:25]

lgtm

[iclelland, 2015-10-21 13:31:13]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-10-21 13:31:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1324173002/#ps560001
(title: "Addressing comments from PS#25")

[commit-bot: I haz the power, 2015-10-21 13:31:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324173002/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324173002/560001

[commit-bot: I haz the power, 2015-10-21 14:03:21]

Committed patchset #27 (id:560001)

[commit-bot: I haz the power, 2015-10-21 14:04:12]

Patchset 27 (id:??) landed as
https://crrev.com/8467f460d6cf1b2b5c71664d952819e2b3b58b85
Cr-Commit-Position: refs/heads/master@{#355284}