1179015 - Add SimplePush GCM router bridge to Fennec installation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[JR Conlin [:jrconlin,:jconlin]]

Mobile devices significantly benefit from having only one system channel open from servers to the device. While SimplePush can maintain this for FirefoxOS devices, it's wasteful and redundant for browsers that are not running on FirefoxOS. 

This patch should: 
1) register Firefox for Android with GCM
2) register the returned GCM token with the SimplePush server
3) Use GCM as the routing bridge for Push Notifications from SimplePush.

Comment 1

[JR Conlin [:jrconlin,:jconlin]]

*NOTE*: I expect that this patch will require mentoring, since I'm not fully versed on writing Android code. The first several patches will be Works in Progress as the finer details get hammered out.

Comment 2

[JR Conlin [:jrconlin,:jconlin]]

PATCH:

https://drive.google.com/file/d/0By2F493aIVXYaHBRcjhnbVFub0E/view?usp=sharing

TODO: 
This code still needs a way to call back to registered functions inside of Android.
Obviously, the android code will also need to be modified to call the registration functions.

Comment 3

[Richard Newman [:rnewman]]

JR: do you want to hand this off and get it in tree, behind a build flag?

Comment 4

[JR Conlin [:jrconlin,:jconlin]]

Due to a system failure and a few other things, I'm rebuilding this patch. Will repost once it's ready.

Comment 5

[JR Conlin [:jrconlin,:jconlin]]

Attachment: WIP gcm_patch.diff (Yes, this will be correctly formatted later)

The attached patch is a WIP patch while I sort out build and patch issues with my config.

Comment 6

[JR Conlin [:jrconlin,:jconlin]]

Attachment: gcm.patch WIP: Added activation hook, cleanup.

TODO: publish endpoint to sync server with call on sync event.

Comment 7

[JR Conlin [:jrconlin,:jconlin]]

Attachment: MozReview Request: Bug 1179015 WIP Review: Add GCM Push for sync

Bug 1179015 WIP Review: Add GCM Push for sync

TODO:
* Verify Device Manager integration
* Verify GCM intent processor registration
* Build out server polling for user.
Questions:
* Device Manager needs an FxA OAuthToken, Need to find where that's stored/generated.
* Is there a better way to do the REST calls? (Async handler?)

Comment 8

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 8647166 [details]
MozReview Request: Bug 1179015 WIP Review: Add GCM Push for sync

Bug 1179015 WIP Review: Add GCM Push for sync

TODO:
* Verify Device Manager integration
* Verify GCM intent processor registration
* Build out server polling for user.
Questions:
* Device Manager needs an FxA OAuthToken, Need to find where that's stored/generated.
* Is there a better way to do the REST calls? (Async handler?)

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8647166 [details]
MozReview Request: Bug 1179015 WIP Review: Add GCM Push for sync

https://reviewboard.mozilla.org/r/15907/#review14199

::: configure.in:3847
(Diff revision 1)
> +MOZ_ANDROID_GCM_SENDERID=377588749100

This should be a mozconfig var.

::: mobile/android/base/BrowserApp.java:751
(Diff revision 1)
> +        // TODO: Prevent if in Guest Mode?

Involve :sebastian for guest mode/restricted profile questions.

::: mobile/android/base/BrowserApp.java:755
(Diff revision 1)
> +                this.gcmBridge.onCreate(appContext, getActivity(), savedInstanceState);

Does this really need to happen in `onCreate`?

::: mobile/android/base/android-services.mozbuild:908
(Diff revision 1)
> +    'sync/bridge/BridgeException.java',

These shouldn't be inside `sync`.

::: mobile/android/base/fxa/sync/FxAccountSyncAdapter.java:549
(Diff revision 1)
> +            SharedPreferences prefs = GeckoSharedPrefs.forProfile(getContext());

This is not the place to be doing this work; you're hooking into the Sync SyncAdapter.

::: mobile/android/base/sync/bridge/BridgeException.java:1
(Diff revision 1)
> +package org.mozilla.gecko.sync.bridge;

MPL2.0 license block.

::: mobile/android/base/sync/bridge/DeviceManager.java:55
(Diff revision 1)
> +    HttpClient client = new DefaultHttpClient();

You might want to use BaseResource here. Otherwise, think about User-Agent etc.

::: mobile/android/base/sync/repositories/domain/ClientRecord.java:54
(Diff revision 1)
> +  public String pushUrl;             // WebPush URL for Sync events

s/Url/URL (throughout)

::: mobile/android/base/sync/repositories/domain/ClientRecord.java:158
(Diff revision 1)
> +    if (this.pushUrl != null) {

I thought we decided not to do this approach? Or am I misremembering?

I think this is worth splitting up into a few different concepts. It looks like you have:

* Code for talking to the registration endpoint.
* Something hooked into BrowserApp for grabbing a URL and writing it to prefs. This blocks first run.
* Some hooks into client sync to include push URLs in client records.
* Hooks into Sync itself to... I'm not sure.

Comment 10

[JR Conlin [:jrconlin,:jconlin]]

https://reviewboard.mozilla.org/r/15905/#review14203

::: mobile/android/base/android-services.mozbuild:913
(Diff revision 1)
> +    'sync/bridge/IGCMCallback.java',

This file is not needed, but was not properly removed from this source before committing.

Comment 11

[JR Conlin [:jrconlin,:jconlin]]

https://reviewboard.mozilla.org/r/15907/#review14199

> This should be a mozconfig var.

The GCM senderid is a bit sensitive. Right now, i'm using a test sender id, but we don't really want to make this value publicly exposed. If mozconfig allows for that, I'm fine with using mozconfig.

> Does this really need to happen in `onCreate`?

gcmBridge.onCreate wraps the gcm.register function which usually wants to be run in the onCreate. gcm.register needs to have the context, activity and the savedInstanceState for the app. gcm.register is fairly light weight since all it does is fetch the GCM ID from the device service, however gcmBridge.onCreate does communicate with the push servers in order to fetch an endpoint if the GCM ID has changed.

> These shouldn't be inside `sync`.

Ok. Where should it be located then?

> This is not the place to be doing this work; you're hooking into the Sync SyncAdapter.

I need to have the FxA OAuth token at some point where the user is logged in and before sync happens. I'm fine moving the code to a better location.

> I thought we decided not to do this approach? Or am I misremembering?

Ah, we did. I'll need to pull this code.

The way that the bridge needs to work is:
1) Call GCM to get an ID to be sent to the Push Service (needs to be done at each app start).
1a) Store the GCMID into app preferences to check if it's changed since the last time the app has started.
2) Register (or update) the GCM ID with the Push Service and get a Push Endpoint.
3) Register (or update) the Push Endpoint to the Device Manager. 
4) Wait for incoming GCM intents in order to fire off a Sync action.

Registration functions should be done before the app goes idle or gets swapped out, but they could occur with a lower priority. 

I'm absolutely fine with restructuring code to fit a better model on how to accomplish that.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #11)
> https://reviewboard.mozilla.org/r/15907/#review14199
> 
> > This should be a mozconfig var.
> 
> The GCM senderid is a bit sensitive. Right now, i'm using a test sender id,
> but we don't really want to make this value publicly exposed. If mozconfig
> allows for that, I'm fine with using mozconfig.

Nick has recently done something very similar with the Adjust SDK "tokenid" so he can help navigate those waters.

Comment 13

[JR Conlin [:jrconlin,:jconlin]]

Attachment: hg.diff

Further WIP. Posted here because `hg push review` won't allow merge patches. 

Refactor push and device manager calls to be async

* Added license headers to files
* Added explanation comment to onCreate() call in BrowserApp
* Removed hooks from FxAccountSyncAdapter & SyncClient
* Added preliminary OAuth Token fetch calls to device manager
** TODO: Need to add Token storage and wire up DeviceManager calls.
* Removed getPeers call, since client won't be using it.
* Convert JSON to simpleJson calls
* Convert prefs to GeckoSharedPrefs
* Logging fixes, method name normalization, etc.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8649579 [details] [diff] [review]
hg.diff

Let's get this into my queue!

Comment 15

[JR Conlin [:jrconlin,:jconlin]]

Attachment: MozReview Request: WIP: Bug 1179015 - Add Push GCM support to Android sync client

WIP: Bug 1179015 - Add Push GCM support to Android sync client

* Added Device Manager hooks to sync handler
** Account is registered with device manager at end of first sync.
* Fixed methods to reduce required arguments where possible.
* Removed some unused functions
* Fixed GCM update command to use auth signatures.
* Added push and device manager server urls as config options.
TODO:
* Testing. Lots of testing.

Comment 16

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 8650774 [details]
MozReview Request: WIP: Bug 1179015 - Add Push GCM support to Android sync client

WIP: Bug 1179015 - Add Push GCM support to Android sync client

* Added Device Manager hooks to sync handler
** Account is registered with device manager at end of first sync.
* Fixed methods to reduce required arguments where possible.
* Removed some unused functions
* Fixed GCM update command to use auth signatures.
* Added push and device manager server urls as config options.
TODO:
* Testing. Lots of testing.

NOTE: due to my screw up, this patch is a refresh from
https://reviewboard.mozilla.org/r/15907/

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8650774 [details]
MozReview Request: WIP: Bug 1179015 - Add Push GCM support to Android sync client

https://reviewboard.mozilla.org/r/16677/#review15011

::: configure.in:3836
(Diff revision 2)
> +MOZ_ANDROID_GCM_PUSH=1

This should be just defined here and then conditionally enabled in mobile/android/confvars.sh.  Lots of examples throughout.

::: configure.in:3840
(Diff revision 2)
> +MOZ_ANDROID_GCM_SENDERID=377588749100

Is this sensitive?  My guess is it is.  You want to add a flag like --with-adjust-sdk-keyfile so that the value stays hidden at build time.  See the example of MOZ_INSTALL_TRACKING_ADJUST_SDK_APP_TOKEN from https://hg.mozilla.org/integration/fx-team/rev/f76f02793f7a

::: mobile/android/base/AndroidManifest.xml.in:109
(Diff revision 2)
> +    <permission android:name="@ANDROID_PACKAGE_NAME@.C2D_MESSAGE"

Worth noting that this permission is /required/ by GCM but unused elsewhere in the application.

::: mobile/android/base/AndroidManifest.xml.in:114
(Diff revision 2)
> +        android:name="com.google.android.c2dm.permission.RECEIVE" />

Is this sufficient?  The wording of https://developers.google.com/cloud-messaging/android/client is very unclear: do we need .SEND as well?

::: mobile/android/base/AndroidManifest.xml.in:399
(Diff revision 2)
> +                <action android:name="com.google.android.c2dm.intent.RECEIVE"/>

There's a note about including

<action android:name="com.google.android.c2dm.intent.REGISTRATION" />

at https://developers.google.com/cloud-messaging/android/client.  Explain why it does not apply.

::: mobile/android/base/AndroidManifest.xml.in:400
(Diff revision 2)
> +                <category android:name="@ANDROID_PACKAGE_NAME@" />

This category is very surprising; I expect a DEFAULT category, a GCM related category, or no category.  Explain why we have a package-specific category.

::: mobile/android/base/AppConstants.java.in:133
(Diff revision 2)
> +

nit: kill double newline.

::: mobile/android/base/BrowserApp.java:213
(Diff revision 2)
> +    private GCM gcmBridge;

GCM is a poor name for a class.

::: mobile/android/base/BrowserApp.java:745
(Diff revision 2)
> +        SharedPreferences prefs = GeckoSharedPrefs.forProfile(appContext);

You don't use prefs at all.

::: mobile/android/base/BrowserApp.java:753
(Diff revision 2)
> +                // A thread has been spawned to put the push endpoint into the preferences.

Why is this comment relevant here?

::: mobile/android/base/BrowserApp.java:754
(Diff revision 2)
> +            } catch (IOException x) {

Consider catching BridgeException first, then catching *all* exceptions afterward.  I see no reason why IOException implies only that Google Play is not present.

::: mobile/android/base/BrowserApp.java:763
(Diff revision 2)
> +           /* At this point, we have a GCM endpoint registered with the Push Service, but we

nit: consider just removing this comment; if you want to keep it, format it.

::: mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
(Diff revision 2)
> -

Get rid of extraneous changes.

::: mobile/android/base/sync/bridge/BridgeException.java:8
(Diff revision 2)
> + * Bridge exception returns if there is a GCM routing bridge issue.

Improve this description or drop it.

::: mobile/android/base/sync/bridge/BridgeException.java:20
(Diff revision 2)
> +  private static final long serialVersionUID = 1;

Generate this or copy-paste-modify.

::: mobile/android/base/sync/bridge/DeviceManager.java:45
(Diff revision 2)
> + * Note: The DeviceManager is not yet production ready and is expected to be part of the FxA suite.

What is the status of this code?  It doesn't appear ready for review.

::: mobile/android/base/sync/bridge/DeviceManager.java:73
(Diff revision 2)
> +    class DM_AuthHeader implements AuthHeaderProvider {

Extract an AuthHeaderProvider that does what you want, encompassing the SHA256 signature and the auth token.  See HMACAuthHeaderProvider and AbstractBearerTokenAuthHeaderProvider.

A pre: commit would be good.

::: mobile/android/base/sync/bridge/DeviceManager.java:103
(Diff revision 2)
> +        msg = GCM.entityToString(response.getEntity());

Create a MozResponse (IIRC) and you get such conversions.

::: mobile/android/base/sync/bridge/DeviceManager.java:119
(Diff revision 2)
> +        SharedPreferences prefs = GeckoSharedPrefs.forApp(this.context);

Is this per-App or per-Profile?  We blur the lines in background services because Android Accounts are per-App, but only speak to one profile.  It's not clear to me what is appropriate here.

::: mobile/android/base/sync/bridge/DeviceManager.java:174
(Diff revision 2)
> +    return this.accountManager.peekAuthToken(this.account,

And if it's not?

::: mobile/android/base/sync/bridge/GCM.java:61
(Diff revision 2)
> +public class GCM {

This is quite tangled: there's a GCM thing that also shows UI and updates the DM.  Tease these apart, so that there's a GCMClient that handles the HTTP bits, and then wrap a delegate pattern that does the pref storaged, and a consumer that ensures that GCM is enabled before starting anything.

::: mobile/android/base/sync/bridge/GCM.java:67
(Diff revision 2)
> +  private static final String PUSHSERVER_DEFAULT_ENDPOINT = "https://updates.services.mozilla.com";

Extract to a constants file.  AppConstants.java.in?

::: mobile/android/base/sync/bridge/GCM.java:82
(Diff revision 2)
> +  private String SENDER_ID = "";

Why are these not final?  When would you update a GCM configuration?

::: mobile/android/base/sync/bridge/GCM.java:101
(Diff revision 2)
> +    GCM gcm;

final absolutely everything, always.

::: mobile/android/base/sync/bridge/GCM.java:185
(Diff revision 2)
> +  public boolean checkPlayServices(Context context, Activity activity) throws IOException {

I think this should probably be part of Fennec's start up.  Can you explain what can happen and why?

::: mobile/android/base/sync/bridge/GCM.java:197
(Diff revision 2)
> +        GooglePlayServicesUtil.getErrorDialog(resultCode, activity,

Are their thread requirements here?  Do you need to be on the main thread?

If you're taking a context (BTW, an Activity is a Context), can this be static?

::: mobile/android/base/sync/bridge/GCM.java:237
(Diff revision 2)
> +        Logger.error(TAG + "getRegistrationId", x.getLocalizedMessage());

Generally it's best to just log the error, like:

Logger.error(TAG, "Message", x)

It's best to keep TAG less than 23 characters (Android platform limitation).

::: mobile/android/base/sync/bridge/GCM.java:237
(Diff revision 2)
> +        Logger.error(TAG + "getRegistrationId", x.getLocalizedMessage());

Generally it's best to just log the error, like:

Logger.error(TAG, "Message", x)

It's best to keep TAG less than 23 characters (Android platform limitation).

::: mobile/android/base/sync/bridge/GCM.java:284
(Diff revision 2)
> +    prefs.edit().putString(UAID_PREF, this.UserAgentId);

I don't think this works like you think.  You want:

prefs.edit()
.putString(...)
.putString(...)
.commit()

::: mobile/android/base/sync/bridge/GCM.java:415
(Diff revision 2)
> +    new Thread(new Runner(this, msg)).start();

Consider accepting an Executor.  The Profile client is a good model.

::: mobile/android/base/sync/bridge/GCM.java:525
(Diff revision 2)
> +      return this.gcm.register(senderID);

This is a mess.  Can we extract this test into the creation phase and just throw an exception when Christmas is ruined?  I don't want to have "I'm horked" state.

::: mobile/android/base/sync/bridge/GCM.java:559
(Diff revision 2)
> +  public void onCreate(Context context, Activity activity, Bundle savedInstanceState)

Continuing with the separation, get this out of the HTTP/signature bits and into the consumer/bridge bits.

::: mobile/android/base/sync/bridge/GCMIntentService.java:47
(Diff revision 2)
> +    if (!extras.isEmpty()) {

Early abort in your failure case.

::: mobile/android/base/sync/bridge/GCMIntentService.java:56
(Diff revision 2)
> +        final Account account = FirefoxAccounts.getFirefoxAccount(context);

This could be null depending on timing.

::: mobile/android/base/sync/bridge/GCMIntentService.java:72
(Diff revision 2)
> +    GCMBridgeBroadcastReceiver.completeWakefulIntent(intent);

Put this in a finally clause.  Don't want to keep a lock!

::: mobile/android/base/sync/syncadapter/SyncAdapter.java:34
(Diff revision 2)
> +import org.mozilla.gecko.sync.bridge.DeviceManager;

This does the business for Sync 1.1 only.  I expect you want Sync 1.5 only; try FxAccountSyncAdapter.

::: mobile/android/base/sync/syncadapter/SyncAdapter.java:429
(Diff revision 2)
> +    if (this.deviceManager != null && ! this.isRegistered) {

There's no expectation that the SyncAdapter instance will persist across syncs.  (In practice, it does; but you should rely on it; and registration is expensive enough that we may not want to do it every sync.)

::: mobile/android/gradle/app/build.gradle:51
(Diff revision 2)
> +    compile 'com.google.android.gms:play-services:7.5.0'

I didn't expect this.  Explain why it's necessary?  base should be the only consumer; app will inherit the dependency.

::: mobile/android/gradle/base/build.gradle:76
(Diff revision 2)
> +    compile 'com.android.support:appcompat-v7:22.2.1'

This isn't right.  You want to improve the conditional above that includes various versions.

I've added a number of comments, ranging from nits to significant issues.

The next steps are for you to tease apart this patch.  I think:

Part 1 should just be build and manifest stuff, with stub implementations.  You're not far off having this in hand, and I provided links to examples of keeping releng secrets hidden.

Part 2 should just be the GCM HTTP client and authentication layer.  No state!  Right now this code is tangled and not re-usable; extracting the AuthHeaderProvider and a client (following our patterns in the code base) should make this trivial.

Part 3 should develop the GCM consumer layer, possibly including maintaining state.  The major issue to address here is how you connect/disconnect from GCM as Android accounts come and go.  I'm not sure what the best pattern looks like here, so a problem statement would be reasonable.

Part 4 should get the GCM consumer integrated into the FxAccountSyncAdapter.  This should be filling in blanks, mostly.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8650774 [details]
MozReview Request: WIP: Bug 1179015 - Add Push GCM support to Android sync client

Thanks, Nick, for the thorough review. JR, please punt this back to me in ~4 parts when you've reflected his comments!

Ping either of us on IRC if you need to bounce ideas off someone.

Comment 19

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 8650774 [details]
MozReview Request: WIP: Bug 1179015 - Add Push GCM support to Android sync client

WIP: Bug 1179015 - Add Push GCM support to Android sync client

* Added Device Manager hooks to sync handler
** Account is registered with device manager at end of first sync.
* Fixed methods to reduce required arguments where possible.
* Removed some unused functions
* Fixed GCM update command to use auth signatures.
* Added push and device manager server urls as config options.
TODO:
* Testing. Lots of testing.

NOTE: due to my screw up, this patch is a refresh from
https://reviewboard.mozilla.org/r/15907/

Comment 20

[JR Conlin [:jrconlin,:jconlin]]

Attachment: MozReview Request: Bug 1179015 - part 1a r?nalexander

Bug 1179015 - part 1a r?nalexander

* Moved GCM to GCMBridge
* Broke out Push Delegate to own class
* First go at moving SenderID to imported key *Broken*
** Added flag value for AppConstants.java.in to spot above brokeness.
* More general Exception trapping for GCMBridge failures
* Added svUID to BridgeException
* Converted HttpResponse handlers to MozResponse/ removed json/string
  conv
* Moved Push flag to confvars.sh

Comment 21

[JR Conlin [:jrconlin,:jconlin]]

https://reviewboard.mozilla.org/r/17497/#review15621

::: configure.in:3943
(Diff revision 1)
> +AC_MSG_WARN([ ############## Using SENDERID of $MOZ_ANDROID_GCM_SENDERID ])

Obvious debugging line is obvious.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java:8
(Diff revision 1)
> +import android.content.Context;     //MODIFIED

Required for my local build, will be extracted for final push.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java:67
(Diff revision 1)
> -    public void onAttach(Activity activity) {
> +    // public void onAttach(Activity activity) { // MODIFIED

Required for my local build, will be extracted for final push.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java:71
(Diff revision 1)
> -    public void onAttach(Activity activity) {
> +    public void onAttach(Context activity) {

Required for my local build, will be extracted for final push.

Comment 22

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/16675/#review15625

The build bits should work fine.  To test locally, add a dummy $TOPSRCDIR/token file with your value, and then add
```
--with-gcm-senderid-keyfile=$TOPSRCDIR/dummy
```
expanding the path as necessary.  Then run ``mach configure`` and inspect the values in ``$OBJDIR/config.status`` to verify things are getting through.  Then ``mach build mobile/android/base`` as usual and see if you get what you want.

::: configure.in:3797
(Diff revision 3)
> +MOZ_ANDROID_GCM_PUSH=$MOZ_ANDROID_GCM_PUSH

Just =.

::: configure.in:3940
(Diff revision 3)
> +                    [ --with-gcm-senderid-keyfile=file GCM SenderID for use by the Push bridge],

nit: phrasing.

::: mobile/android/base/Makefile.in:262
(Diff revision 3)
> +# Likewise the GCM Senderid should not be exposed.

nit: capitalization.

::: mobile/android/base/Makefile.in:265
(Diff revision 3)
> +	@echo '//#define MOZ_ANDROID_GCM_SENDERID $(MOZ_ANDROID_GCM_SENDERID)' > $@

Just like so.

Comment 23

[JR Conlin [:jrconlin,:jconlin]]

https://reviewboard.mozilla.org/r/16677/#review15269

::: mobile/android/base/sync/bridge/DeviceManager.java:73
(Diff revision 2)
> +    class DM_AuthHeader implements AuthHeaderProvider {

I'm not sure I follow. DM_AuthHeader is only used by DeviceManager, and only adds the token (the OAuth Token generated externally) as the "Authorization" header if it's set. Including it as part of the DeviceManager class ensures that it stays in the DeviceManager context, as part of the DeviceManager delegate. It has two methods. Nothing else uses it, so I'm not sure why breaking it out makes sense.

Frankly, if there was a way to drop the class and simply modify the outbound delegate header, I'd prefer to do that. 

Do you feel that this code should be broken out for consistency or am I missing something obvious again?

::: mobile/android/base/sync/bridge/DeviceManager.java:119
(Diff revision 2)
> +        SharedPreferences prefs = GeckoSharedPrefs.forApp(this.context);

Ah, thanks! Missed converting these to perProfile.

::: mobile/android/base/sync/bridge/DeviceManager.java:174
(Diff revision 2)
> +    return this.accountManager.peekAuthToken(this.account,

The remote server would return an error. I'll catch it in the client to reduce the IO.

::: mobile/android/base/sync/bridge/GCM.java:185
(Diff revision 2)
> +  public boolean checkPlayServices(Context context, Activity activity) throws IOException {

This is a system call to see if Play Services are available on the current device. If we do decide that this is something we check once, we should probably patch https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/ChromeCast.java#162

::: mobile/android/base/sync/bridge/GCM.java:197
(Diff revision 2)
> +        GooglePlayServicesUtil.getErrorDialog(resultCode, activity,

This is the recommended recovery step if the returned code is not SUCCESS. https://github.com/googlesamples/google-services/blob/master/android/gcm/app/src/main/java/gcm/play/android/samples/com/gcmquickstart/MainActivity.java#L92 notes that it's possible to recover from this error, but if it's a problem we don't have to.

Comment 24

[JR Conlin [:jrconlin,:jconlin]]

https://reviewboard.mozilla.org/r/16677/#review15011

> Worth noting that this permission is /required/ by GCM but unused elsewhere in the application.

Actually, it is. GCM requires this permission internally.

> Is this sufficient?  The wording of https://developers.google.com/cloud-messaging/android/client is very unclear: do we need .SEND as well?

We only need send if we are sending GCM messages back back across the bridge. We're not. We're only receiving them. If we ever do decide to use the bidirectional aspect, we will have to add that permission.

> There's a note about including
> 
> <action android:name="com.google.android.c2dm.intent.REGISTRATION" />
> 
> at https://developers.google.com/cloud-messaging/android/client.  Explain why it does not apply.

A better question is what is the base version of Android that we will support?

> This category is very surprising; I expect a DEFAULT category, a GCM related category, or no category.  Explain why we have a package-specific category.

As I understand how GCM works, an app may be alerted for any message delivered to a given category. It's a good idea to limit the reciever only to a known category. In this case, limiting to a particular category based on the package would allow us to use the same GCM route for stage, prod, or dev. I'm fine with using a default channel if we have Google developer accounts or separate project IDs for these distinct channels.

> This is a mess.  Can we extract this test into the creation phase and just throw an exception when Christmas is ruined?  I don't want to have "I'm horked" state.

It is. the call structure is onCreate() > getRegistrationID() > gcmRegister(senderID). getRegistrationID caches the registrationID , and gcmRegister caches the GoogleCloudMessaging instance. I can collapse those into a single funtion, but I thought having each broken out was more readable.

> I didn't expect this.  Explain why it's necessary?  base should be the only consumer; app will inherit the dependency.

Hmm, this may be a product of an Android Studio fix. I'll investigate and pare it out.

> This isn't right.  You want to improve the conditional above that includes various versions.

Agreed. Not sure why this gets forced in. Sorry about that.

Comment 25

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 8653736 [details]
MozReview Request: Bug 1179015 - part 1a r?nalexander

Bug 1179015 - part 1a r?nalexander

* Moved GCM to GCMBridge
* Broke out Push Delegate to own class
* First go at moving SenderID to imported key
  (NOTE: If template files are not showing new values, remove
   $TARGET/config.statusc and re-run mach configure)
* More general Exception trapping for GCMBridge failures
* Added svUID to BridgeException
* Converted HttpResponse handlers to MozResponse/ removed json/string
  conv
* Moved Push flag to confvars.sh
* added null checks

Comment 26

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1179015 - Part 1: Add MOZ_ANDROID_GCM{_SENDERID} build flags. r=nalexander

Bug 1179015 - Part 1: Add MOZ_ANDROID_GCM{_SENDERID} build flags. r=nalexander

These flags are not intended to be feature specific.  On day one, we
intend to support a single GCM-backed feature -- Push Notifications --
but the set of GCM-consuming features is potentially large (e.g.,
possibly Firefox Sync tickles and Send Tab to Device alerts).  Such
features can and will have their own build flags.

Comment 27

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1179015 - WIP on the GCM registration.

Bug 1179015 - WIP on the GCM registration.

Comment 28

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 8655576 [details]
MozReview Request: Bug 1179015 - Part 1: Add MOZ_ANDROID_GCM{_SENDERID} build flags. r=nalexander

https://reviewboard.mozilla.org/r/17931/#review16057

::: mobile/android/base/AndroidManifest.xml.in:513
(Diff revision 1)
> +#ifdef MOZ_ANDROID_GCM

Sadly, with the newest iteration of GCM and the inclusion of the InstanceID API, this may need to change.
https://developers.google.com/instance-id/

We briefly discussed a few additional changes that are outside of this patch, however this should be good for now. 

r+

Comment 29

[Richard Newman [:rnewman]]

JR, Nick: could you tidy up the flags and attachments to this? Not sure what's stale.

Comment 30

[JR Conlin [:jrconlin,:jconlin]]

Sorry for the late reply on this, I've been dealing with a few fires around Push and FindMyDevice. 

I've been refactoring the code to use the latest recommendations around GCM (switching to InstanceID and GCM registration listeners). Currently, i'm hitting a problem with the emulator (running 5.1.1 Google APIs) reporting that the Google Play Services are out of date (Requires 7895000 found 6774430). I'm trying to figure out what's causing the discrepancy. Once I have that resolve and can verify that the refactoring properly activates Google Pay and GCM, then I will merge the latest code base.

Comment 31

[Nick Alexander :nalexander [he/him]]

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #30)
> Sorry for the late reply on this, I've been dealing with a few fires around
> Push and FindMyDevice. 
> 
> I've been refactoring the code to use the latest recommendations around GCM
> (switching to InstanceID and GCM registration listeners). Currently, i'm
> hitting a problem with the emulator (running 5.1.1 Google APIs) reporting
> that the Google Play Services are out of date (Requires 7895000 found
> 6774430).

Can I see the complete errors, and how they arose?  If the errors are from Fennec, it's likely that the current build configuration is picking up the library-style Google Play dependencies, which might be version 6.7.  The work under review in Bug 1108782 will make updating this dependency to 7.8 easy.

 I'm trying to figure out what's causing the discrepancy. Once I
> have that resolve and can verify that the refactoring properly activates
> Google Pay and GCM, then I will merge the latest code base.

If you push your WIP, I might rebase it onto Bug 1108782 and we could see if it's our build or an emulator issue.

Comment 32

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 8650774 [details]
MozReview Request: WIP: Bug 1179015 - Add Push GCM support to Android sync client

WIP: Bug 1179015 - Add Push GCM support to Android sync client

* Added Device Manager hooks to sync handler
** Account is registered with device manager at end of first sync.
* Fixed methods to reduce required arguments where possible.
* Removed some unused functions
* Fixed GCM update command to use auth signatures.
* Added push and device manager server urls as config options.
TODO:
* Testing. Lots of testing.

NOTE: due to my screw up, this patch is a refresh from
https://reviewboard.mozilla.org/r/15907/

Comment 33

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 8653736 [details]
MozReview Request: Bug 1179015 - part 1a r?nalexander

Bug 1179015 - part 1a r?nalexander

* Moved GCM to GCMBridge
* Broke out Push Delegate to own class
* First go at moving SenderID to imported key
  (NOTE: If template files are not showing new values, remove
   $TARGET/config.statusc and re-run mach configure)
* More general Exception trapping for GCMBridge failures
* Added svUID to BridgeException
* Converted HttpResponse handlers to MozResponse/ removed json/string
  conv
* Moved Push flag to confvars.sh
* added null checks
* reworking to use latest GCM initialization method
* removed unused vars and libararies
* added transition functions for SearchFragments (to resolve compiles)

Comment 34

[JR Conlin [:jrconlin,:jconlin]]

The lines from my log are: 

>==
09-14 09:38:23.988: I/GeckoLogger(1466): fennec_firefox_dev :: HealthReportStorage :: Initializing measurement org.mozilla.appSessions to 4 (current 4)
09-14 09:38:23.988: I/GeckoLogger(1466): fennec_firefox_dev :: HealthReportStorage :: Measurement org.mozilla.appSessions already at v4
09-14 09:38:23.989: I/GeckoLogger(1466): fennec_firefox_dev :: HealthReportStorage :: Initializing measurement org.mozilla.searches.counts to 6 (current 6)
09-14 09:38:23.989: I/GeckoLogger(1466): fennec_firefox_dev :: HealthReportStorage :: Measurement org.mozilla.searches.counts already at v6
09-14 09:38:23.990: D/GeckoHealthRec(1466): Ensuring environment.
09-14 09:38:24.011: D/GeckoSysInfo(1466): System memory: 731MB.
09-14 09:38:24.072: W/GooglePlayServicesUtil(1466): Google Play services out of date.  Requires 7895000 but found 6774430
09-14 09:38:24.072: D/GeckoBrowserApp(1466): init Server Urls
09-14 09:38:24.113: E/SQLiteLog(1466): (2067) abort at 45 in [INSERT INTO environments(sysVersion,architecture,hash,updateChannel,cpuCount,acceptLangSet,addonsID,pluginCount,screenYInMM,uiMode,appVersion,xpcomabi,version,extensionCount,isBlocklistEnabled,
09-14 09:38:24.119: D/GeckoHealthRec(1466): Finishing init.
>==

This happens very early on in the log, and prevents any additional GCM initialization to happen. 

I've also updated the review to show the latest WIP changes.

Comment 35

[Nick Alexander :nalexander [he/him]]

jrconlin: I took your work, rebased it on top of some not-yet-landed build changes, and pushed to review.  (But didn't open the review.)  You can get the changes by doing:

hg pull -r 2a241f3a42f440664b90599f6ce1e68088c7b554 https://reviewboard-hg.mozilla.org/gecko/

Then, |hg up 2a241f3a42f440664b90599f6ce1e68088c7b554|.  Start modifying that top commit.  A few notes:

* I removed the AndroidManifest.xml.in registration.  Work on that next, since Google changed things.

* The Context vs. Activity stuff should not happen.  These build patches fix versions; you may need to install different versions.  You're my build incompatibilities guinea pig.

* You had the conditional reversed in your GPS exists.

* You're swallowing useful information (the error code) in order to return a useless BridgeException.

Comment 36

[Nick Alexander :nalexander [he/him]]

There's a lot of discussion on this ticket, but I'm reducing the scope.

This ticket tracks interfacing with whatever GCM / Mozilla Push bridge the Services team builds.  We'll do the Fennec architecture work elsewhere.

It's my intention that non-Fennec experts can develop this piece, building on top of the Fennec architecture work I will do.

Comment 37

[Nick Alexander :nalexander [he/him]]

It would be best if this bridge happened behind a feature flag independent of the GCM feature flag(s) added by Bug 1207708.

Comment 38

[JR Conlin [:jrconlin,:jconlin]]

GCM bridge work added previously.