Implement GCMDriver.java using MultiplexingGcmListener

Implement GCMDriver.java using MultiplexingGcmListener

Known caveat:
- Since GCM on Android only supports a single registration per native
  app, only a single web app can register at once, and even then only
  if they pass --disable-sync-gcm-in-order-to-try-push-api on the command
  line which disables Chrome Sync etc from receiving GCM messages.

Depends on https://codereview.chromium.org/316963003/

BUG=350384
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276795

[johnme, 2014-06-06 19:17:31]

Hi folks, this isn't ready for landing yet (due to the FIXME), but I'd still
appreciate it if you could review the approach, such that I can clean it up and
hopefully land it on Monday.

Once this and https://codereview.chromium.org/317823007/ land, we'll have a
working end-to-end implementation of the registration part of the Push API.

[fgorski, 2014-06-09 17:16:27]

https://codereview.chromium.org/314293006/diff/1/components/gcm_driver/androi...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/314293006/diff/1/components/gcm_driver/androi...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:75:
GCMRegistrar.checkDevice(mContext);
Why are you using GCMRegistrar? Shouldn't you use GoogleCloudMessaging API per:
http://developer.android.com/reference/com/google/android/gcm/GCMRegistrar.html?

[johnme, 2014-06-09 21:45:11]

Added a command line switch to guard whether sync gets disabled or not.

https://codereview.chromium.org/314293006/diff/1/components/gcm_driver/androi...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/314293006/diff/1/components/gcm_driver/androi...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:75:
GCMRegistrar.checkDevice(mContext);
On 2014/06/09 17:16:27, fgorski wrote:
> Why are you using GCMRegistrar? Shouldn't you use GoogleCloudMessaging API
per:
>
http://developer.android.com/reference/com/google/android/gcm/GCMRegistrar.html

Various reasons:

- It's what all the existing code in Chrome uses, and is presumably maximally
compatible with MultiplexingGcmListener etc.

- It seems cleanest for all of Chrome to switch over in one go, especially since
it requires a new dependency on the Google Play services SDK library (which
might have binary size impact, and also risks complications since IIRC we use a
non-public version of Play Services in downstream Chrome for Android).

- This code is going to have to be re-written anyway once we have a solution for
passing app ID to GCM, so it seems expedient for now to take the approach which
doesn't require significant refactoring/adding a new library dependency.

But I definitely agree that we should migrate soon. I've added a TODO to migrate
(this and the rest of Chrome) over to GoogleCloudMessaging.

[fgorski, 2014-06-09 23:14:28]

lgtm

https://codereview.chromium.org/314293006/diff/1/components/gcm_driver/androi...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/314293006/diff/1/components/gcm_driver/androi...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:75:
GCMRegistrar.checkDevice(mContext);
On 2014/06/09 21:45:11, johnme wrote:
> On 2014/06/09 17:16:27, fgorski wrote:
> > Why are you using GCMRegistrar? Shouldn't you use GoogleCloudMessaging API
> per:
> >
>
http://developer.android.com/reference/com/google/android/gcm/GCMRegistrar.html
> 
> Various reasons:
> 
> - It's what all the existing code in Chrome uses, and is presumably maximally
> compatible with MultiplexingGcmListener etc.
> 
> - It seems cleanest for all of Chrome to switch over in one go, especially
since
> it requires a new dependency on the Google Play services SDK library (which
> might have binary size impact, and also risks complications since IIRC we use
a
> non-public version of Play Services in downstream Chrome for Android).
> 
> - This code is going to have to be re-written anyway once we have a solution
for
> passing app ID to GCM, so it seems expedient for now to take the approach
which
> doesn't require significant refactoring/adding a new library dependency.
> 
> But I definitely agree that we should migrate soon. I've added a TODO to
migrate
> (this and the rest of Chrome) over to GoogleCloudMessaging.

OK, makes sense.

[nyquist, 2014-06-11 07:52:14]

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver.gy...
components/gcm_driver.gypi:110: '../sync/sync.gyp:sync_java',
Looking forward to invalidations being completely separate from sync! But, oh
well, not yet...

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:86:
GCMRegistrar.register(mContext, senderIds);
I was under the impression that the registration ID should be stored to a
preference together with the current app version, and that we must
force-re-register on app upgrade. See
http://developer.android.com/google/gcm/client.html
Or are we requiring all websites to remember to do this?

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:96:
return; // Wait for onRegistered to be called.
Nit: Shouldn't this be on the line above in Java? Also; {@link
#onRegisterFinished}

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:114:
return true; // Indicates success
Nit: Shouldn't this be on the line above in Java?

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:123:
return; // Wait for onUnregistered to be called.
Nit: Shouldn't this be on the line above in Java?

https://codereview.chromium.org/314293006/diff/20001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/SyncSwitches.java (right):

https://codereview.chromium.org/314293006/diff/20001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/SyncSwitches.java:11: public abstract
class SyncSwitches {
Does this need to be abstract? What about just using final?

[johnme, 2014-06-11 13:53:12]

Thanks for the review Tommy, replies below.

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:86:
GCMRegistrar.register(mContext, senderIds);
On 2014/06/11 07:52:13, nyquist wrote:
> I was under the impression that the registration ID should be stored to a
> preference together with the current app version, and that we must
> force-re-register on app upgrade. See
> http://developer.android.com/google/gcm/client.html
> Or are we requiring all websites to remember to do this?

I talked with costin@, and he explained that the GCM docs recommend this in
order to work around a bug in Froyo (where GCM is in /system, so can't be
updated OTA), but that since Chrome only ships to ICS and above, we don't need
to do this. Phew :-)

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:96:
return; // Wait for onRegistered to be called.
On 2014/06/11 07:52:13, nyquist wrote:
> Nit: Shouldn't this be on the line above in Java? Also; {@link
> #onRegisterFinished}

Added link. See below re line above.

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:114:
return true; // Indicates success
On 2014/06/11 07:52:13, nyquist wrote:
> Nit: Shouldn't this be on the line above in Java?

The style guide seems happy with end-of-line comments, and even gives rules for
spaces before them[1] and aligning them[2]. In this case, it seems clearer to
tie the (short) explanation to the (also short) code?

[1]:
http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.2-horiz...
[2]:
http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.3-horiz...

I did however notice that you're supposed to use braces for conditionals etc
even if the body is a single statement[3], so I've added braces around these
conditionals.

[3]:
http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.1.1-brace...

https://codereview.chromium.org/314293006/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:123:
return; // Wait for onUnregistered to be called.
On 2014/06/11 07:52:13, nyquist wrote:
> Nit: Shouldn't this be on the line above in Java?

Ditto.

https://codereview.chromium.org/314293006/diff/20001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/SyncSwitches.java (right):

https://codereview.chromium.org/314293006/diff/20001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/SyncSwitches.java:11: public abstract
class SyncSwitches {
On 2014/06/11 07:52:13, nyquist wrote:
> Does this need to be abstract? What about just using final?

All the existing switches files use an abstract class. Specifically:

android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellSwitches.java
base/android/java/src/org/chromium/base/BaseSwitches.java
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSwitches.java
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java

I guess the idea is that there's no point instantiating these classes, they're
just supposed to be used to statically access the switch names. Seems cleanest
do the same for SyncSwitches.

[nyquist, 2014-06-11 15:02:08]

lgtm

[johnme, 2014-06-11 15:38:45]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-06-11 15:41:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnme@chromium.org/314293006/40001

[johnme, 2014-06-11 19:29:15]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2014-06-12 17:07:29]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-06-12 17:08:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnme@chromium.org/314293006/60001

[commit-bot: I haz the power, 2014-06-12 20:51:21]

Change committed as 276795