1247786 - Add push subscription to device registration

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Vlad Filippov (:vladikoff)]

We need to register the device's Push callback and then save it with the rest of the device details when `_registerOrUpdateDevice` is called.

Comment 1

[Vlad Filippov (:vladikoff)]

Attachment: MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review commit: https://reviewboard.mozilla.org/r/35223/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35223/

Comment 2

[Vlad Filippov (:vladikoff)]

The goal of this patch is to start populating `pushCallback` values in the auth server for new devices.

Ref: https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountdevice

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review31867

Looks great!

::: services/fxaccounts/FxAccounts.jsm:67
(Diff revision 1)
> +const FXA_PUSH_SCOPE_ACCOUNT_UPDATE = "app://fxa-device-update";

Nit: I know I suggested `app://` before, but maybe chrome://fxa-device-update is a better fit, to match `FXA_PWDMGR_HOST`. Looks like `app://` technically belongs to B2G apps.

The Push code doesn't validate the scope for privileged subscriptions, so it's OK to leave as-is, or set it to a non-URL. Your call.

::: services/fxaccounts/FxAccounts.jsm:1408
(Diff revision 1)
> +  _registerPushEndpoint: function () {

I think this will reject on Android, since we're not shipping the component yet. Is that a problem?

Comment 4

[Ryan Kelly [:rfkelly]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review31977

Looks like a good initial step, I like the idea of figuring out all the moving pieces here as separate pieces.  I wonder if we should add handling of subscription-url changes to this patch as well (ref https://bugzilla.mozilla.org/show_bug.cgi?id=1235607#c10) so that the feature encompases everything about maintaining the push subscription on the device record, but separate from listening to and handling the messages.

::: services/fxaccounts/FxAccounts.jsm:66
(Diff revision 1)
> +// it is up to the client to figure out what exactly changed and take action

Style consistency nit: the other comments in this file seem to prefer full sentences with capitalization and punctuation.

::: services/fxaccounts/FxAccounts.jsm:1420
(Diff revision 1)
> +  },

Out of curiosity, will this generate a new pushCallback URL each time the browser starts up, or will it re-use an existing subscription from previous session?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review31989

Like Ryan, I like having this work split into nice small chunks. However, I'm not convinced it should actually land yet - we will just be asking Firefox to perform additional work for no reason - ie, I think we should get r+ on another patch that actually uses push and land them together (but I can be convinced otherwise).

My only real concern is the lack of unsubscription - given we don't have a service worker, I can't see how gecko would know this can ever be automagically cleaned up - but I know nothing about push, so if Kit is sure it's OK, please consider this r+ with the other issues addressed.

::: services/fxaccounts/FxAccounts.jsm:67
(Diff revision 1)
> +const FXA_PUSH_SCOPE_ACCOUNT_UPDATE = "app://fxa-device-update";

this should probably go into FxAccountsCommon.jsm (and I agree with Kit's comment re the name)

::: services/fxaccounts/FxAccounts.jsm:1410
(Diff revision 1)
> +      PushService.subscribe(FXA_PUSH_SCOPE_ACCOUNT_UPDATE,

I can't see a matching unsubscribe - is that a problem?

::: services/fxaccounts/FxAccounts.jsm:1413
(Diff revision 1)
> +          if (Components.isSuccessCode(result)) {

I think we should do some logging here to at least indicate if we successfully got a subscription or not. Also, is it possible .subscribe will throw instead of making a callback? If so, we should handle that. (And maybe we need to handle the push service not existing for b2g? Or maybe that's OK if b2g never actually gets here.)

::: services/fxaccounts/FxAccounts.jsm:1414
(Diff revision 1)
> +            return resolve(subscription);

There's no need to return here or below

::: services/fxaccounts/FxAccounts.jsm:1433
(Diff revision 1)
>      return Promise.resolve().then(() => {

With these changes we no longer need the dummy initial promise - ie, you should be able to just |return this.\_registerPushEndpoint().then(...)|

::: services/fxaccounts/FxAccounts.jsm:1437
(Diff revision 1)
> +    .then((subscription) => {

please move this to the previous line to be consistent with the other .then/.catch calls here.

::: services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js:151
(Diff revision 1)
> -  do_check_eq(spy.registerDevice.args[0].length, 3);
> +  do_check_eq(spy.registerDevice.args[0].length, 4);

Can we have _registerPushEndpoint here return an object with an |endpoint| attribute and ensure args[3] correctly gets it?

Comment 6

[Ryan Kelly [:rfkelly]]

> I can't see a matching unsubscribe - is that a problem?

I believe our intention is to keep the subscription alive for as long as the device is connected to FxA.  Maybe we need an explicit "unsubscribe" as part of the device disconnection procedure.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ryan Kelly [:rfkelly] from comment #6)
> Maybe we need an explicit "unsubscribe" as part
> of the device disconnection procedure.

That's exactly what I meant. The observer "weave:service:start-over" is probably reasonable for that (and yeah, we should probably unregister the device at that point, which IIUC we don't - I didn't consider that when device registration landed)

Comment 8

[Vlad Filippov (:vladikoff)]

https://reviewboard.mozilla.org/r/35223/#review31867

> Nit: I know I suggested `app://` before, but maybe chrome://fxa-device-update is a better fit, to match `FXA_PWDMGR_HOST`. Looks like `app://` technically belongs to B2G apps.
> 
> The Push code doesn't validate the scope for privileged subscriptions, so it's OK to leave as-is, or set it to a non-URL. Your call.

Sounds good! will change to `chrome://`

> I think this will reject on Android, since we're not shipping the component yet. Is that a problem?

Thanks for the note, none of the device code is on Android yet, so no need to worry.

Comment 9

[Vlad Filippov (:vladikoff)]

https://reviewboard.mozilla.org/r/35223/#review31977

Good idea, re `add handling of subscription-url changes to this patch as well`

Comment 10

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35223/diff/1-2/

Comment 11

[Vlad Filippov (:vladikoff)]

Mark, Ryan, I updated the patch to support subscription change and unsubscribe. 

From my end I need to update / add tests.

I decided to introduce the FxAccountsPush.jsm because FxAccounts.jsm is already a huge file. 
FxAccountsPush.jsm should help us better organize our code and make it easier to test Push stuff. 

Let me know if I'm missing any major components from this.

Comment 12

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

(used the wrong handle for Ryan, will change that in the updated commit)

Comment 13

[Vlad Filippov (:vladikoff)]

https://reviewboard.mozilla.org/r/35223/#review33099

::: services/fxaccounts/FxAccountsPush.jsm:104
(Diff revision 2)
> +    this.registerPushEndpoint()

TODO: This should also use the device API to update the device callback.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review33095

As discussed in IRC I've got a few concerns about the fact this doesn't survive browser restarts, may miss subscription changes, and doesn't update the server for the ones we *do* get. We should probably chat about this in the eng standup or similar.

::: services/fxaccounts/FxAccounts.jsm:1408
(Diff revision 2)
> +    return this.pushManager.registerPushEndpoint().then((subscription) => {

I prefer avoiding parens when not necessary (ie, then(subscription => ...) - but if you prefer it the other way feel free to leave it as is.

More importantly though, it seems as though we will not get a subscription on subsequent startups, which worries me.

::: services/fxaccounts/FxAccountsPush.jsm:50
(Diff revision 2)
> +      if (! isDevicePushEnabled) {

nit: remove the space after the "!"

::: services/fxaccounts/FxAccountsPush.jsm:62
(Diff revision 2)
> +            log.debug("FxAccountsPushManager failed to subscribe");

I think this should probably be log.warn and include result:

log.warn("FxAccountsPushManager failed to subscribe", result);

::: services/fxaccounts/FxAccountsPush.jsm:88
(Diff revision 2)
> +            log.debug("FxAccountsPushManager failed to unsubscribe");

similarly here - warn and log the result

::: services/fxaccounts/FxAccountsPush.jsm:90
(Diff revision 2)
> +          return resolve();

IIUC resolve() always returns undefined, and we aren't using the return for control flow here, so just drop the return

::: services/fxaccounts/FxAccountsPush.jsm:120
(Diff revision 2)
> +        this._onPushSubscriptionChange();

It looks like |data| is the scope that changed, so we should check that.

It looks like we also need to inform the server of the new subscription, and there's also a risk that the subscription will change before we've set the observers up.

So it sounds like this needs a function that's called every startup that (a) add the observers then (b) obtains the subscription and (c) updates the end-point with the server if necessary. By adding the observer before we get the subscription we should be safe in never missing an update.

Comment 15

[Vlad Filippov (:vladikoff)]

Kit, what are you thoughts on listening to Push after browser restarts? (See concerns in the comment above)

Comment 16

[Vlad Filippov (:vladikoff)]

```
[12:52:49]  <vladikoff>	kitcambridge, does "push-subscription-change" happen often? 
[12:54:45]  <kitcambridge>	vladikoff: not very often. if we reset the device ID (we've been considering doing that if the user gets too many messages, to avoid notification spam), or if the user clears out "dom.push.userAgentID", then it'll fire.
```

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review33177

::: services/fxaccounts/FxAccounts.jsm:354
(Diff revision 2)
> +    this.pushManager.initialize();

Vlad explained to me on IRC that `FxAccounts.jsm` is lazily initialized when Sync starts. I think we'll need a global observer that's initialized on startup, and listens for `push-subscription-change` and `push-message` notifications.

::: services/fxaccounts/FxAccounts.jsm:1407
(Diff revision 2)
> -    return Promise.resolve().then(() => {
> +      // register this device with a push callback

I think I'm missing something. IIUC, we only call `subscribe()` when we first register the device. Push persists that subscription to IndexedDB, so we only need to listen for the observer notifications on subsequent restarts.

::: services/fxaccounts/FxAccountsPush.jsm:76
(Diff revision 2)
> +  unsubscribe: function() {

Do you need to inform the FxA server that the endpoint is no longer valid, or is that already handled by device manager when the user signs out?

::: services/fxaccounts/FxAccountsPush.jsm:120
(Diff revision 2)
> +        this._onPushSubscriptionChange();

Yes, please check that `data` matches the scope. Sorry this is such a clunky API.

::: services/fxaccounts/FxAccountsPush.jsm:124
(Diff revision 2)
> +        this.unsubscribe();

Please add a `.catch()` here and above, just in case `PushService` throws.

Comment 18

[Ryan Kelly [:rfkelly]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review33345

I don't have much to add on top of other reviews and out-of-band discussion, but I'm excited to see this continue moving forward.  Sounds like we're definitely learning a lot about how to do push in chrome code from this experience.

::: services/fxaccounts/FxAccountsCommon.js:69
(Diff revision 2)
> +// else sensitive, such as credentials) should be logged.

This comment doesn't seem to match with the intent of the pref...

Comment 19

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35223/diff/2-3/

Comment 20

[Vlad Filippov (:vladikoff)]

https://reviewboard.mozilla.org/r/35223/#review34061

::: services/fxaccounts/FxAccounts.jsm:370
(Diff revision 3)
> +      }, ON_FXA_PUSH_SUBSCRIPTION_CHANGE, false);

Mark, we can also add `observe` to the main `FxAccountsInternal` object if you want.

::: services/fxaccounts/tests/xpcshell/test_accounts.js:169
(Diff revision 3)
> +    _registerPushEndpoint() {

I have not updated the tests yet

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review34239

Looks good from the push implementation side of things. I'm sure :markh will have more substantial comments than my nitpicking. :-)

::: services/fxaccounts/FxAccounts.jsm:365
(Diff revision 3)
> +      this._pushManager = new FxAccountsPushManager();

If we're going to have `FxAccountsPushManager` export an XPCOM component (for bug 1252290), I wonder if it makes sense to pass the `FxAccounts` instance to it, and have it register the observer.

::: services/fxaccounts/FxAccounts.jsm:523
(Diff revision 3)
> +        this.updateDeviceRegistration();

Does this need a `return`, or does it happen in the background?

::: services/fxaccounts/FxAccounts.jsm:738
(Diff revision 3)
> +      function resolveObserver(reason) {

Do you think it's worthwhile using `clearTimeout` here instead of the `resolved` flag...or is that not worth the additional complexity?

::: services/fxaccounts/FxAccounts.jsm:748
(Diff revision 3)
> +      Services.obs.addObserver(observe, ON_FXA_PUSH_MESSAGE, false)

Style nit: missing semicolon here, and below.

::: services/fxaccounts/FxAccounts.jsm:1013
(Diff revision 3)
> -  startVerifiedCheck: function(data) {
> +  startVerifiedCheck: function(data, options={}) {

Nit: it seems the dominant style in this file is spaces around `=`.

::: services/fxaccounts/FxAccounts.jsm:1053
(Diff revision 3)
> +          this.pollEmailStatus(currentState, data.sessionToken, "start");

Oh, I see. If we did it right, polling will succeed the first time; otherwise, it'll retry as before?

::: services/fxaccounts/FxAccountsPush.jsm:135
(Diff revision 3)
> +          this._onPushSubscriptionChange();

Looks like `_onPushSubscriptionChange` expects `data` to be passed along...or you could use the constant, like in `_onPushMessage`.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review34065

I agree with Kit's comments - some of them are also reflected here. I skimmed the tests, and I think I saw you say additional tests are forthcoming.

But in general this is coming along great, thanks!

::: services/fxaccounts/FxAccounts.jsm:367
(Diff revision 3)
> +      // register subscription change observer to be able to update device information when Push details change.

As Kit mentioned, I think we should consider if we can assume bug 1252290 is going to land as specified. IIUC, this would mean the PushManager object will be instantiated before FxAccounts and the relevant observe methods would live on that, which then makes calls back into the FxAccounts object - possibly initializing it.

This would have some benefits even if that bug takes forever to move - we could arrange for FxAccountsPushManager() to be loaded early in the browser startup process without requiring us to early-initialize the entire FxAccounts world at that point. Once that bug did land we'd just convert the push manager module to be an xpcom service and remove the early import of the module, assuming the push manager will do the right thing.

::: services/fxaccounts/FxAccounts.jsm:732
(Diff revision 3)
> +      let observe = function observe(subject, topic, data) {

ditto here - the observer should be in the push module and callback into here on receipt. It seems fine that this observer is always alive - presumably this exact same message will be used in the future for other account-state notifications (eg, deleted, creds changed, etc?)

::: services/fxaccounts/FxAccounts.jsm:740
(Diff revision 3)
> +          log.debug("push waiting resolved with the following reason:", reason);

remove the trailing ":" on the string - the log module will add it when there's a second param.

::: services/fxaccounts/FxAccounts.jsm:1046
(Diff revision 3)
> +      if (options.context === 'setSignedInUser') {

It seems this complexity exists so we aren't polling *and* waiting for push. I don't really see a good reason we can't just do both (possibly decreasing the verification poll interval at the same time) - it seems like it would make things much simpler and remove this "change behaviour depending on who called me" smell.

::: services/fxaccounts/FxAccountsPush.jsm:20
(Diff revision 3)
> +const OBSERVER_TOPIC_PUSH_MESSAGE = "push-message";

seems a pity these aren't attributes on nsIPushService - Kit, what do you think about adding them there?

::: services/fxaccounts/FxAccountsPush.jsm:76
(Diff revision 3)
> +  unsubscribe: function() {

We should be consistent with how we declare functions here - the fancy new syntax is preferred, so this should just be |unsubscribe() {...}| - and most other functiosn below.

::: services/fxaccounts/FxAccountsPush.jsm:88
(Diff revision 3)
> +            log.warn("FxAccountsPushManager failed to unsubscribe");

let's add |result| to the log output here.

::: services/fxaccounts/FxAccountsPush.jsm:101
(Diff revision 3)
> +    this.notifyObservers(ON_FXA_PUSH_MESSAGE, FXA_PUSH_SCOPE_ACCOUNT_UPDATE);

We can probably just call directly back into FxA here (and below). Ideally, we'd also pass some account data in too, so the consumer can explicitly check the .verified flag. It would also mean less changes are needed once we also send message for "acct deleted" (unless this channel will *only* be used for acct verification, in which case the scope name should change (but I think it's better left as a generic "account updated" flag)

(This could also be useful for profile changes later too - we are fairly inefficient with that currently)

Comment 23

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #22)
> seems a pity these aren't attributes on nsIPushService - Kit, what do you
> think about adding them there?

Sure, sounds good to me. I see XPIDL doesn't support string constants, but we can expose them as attributes. Filed bug 1253438.

Comment 24

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35223/diff/3-4/

Comment 25

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35223/diff/4-5/

Comment 26

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35223/diff/5-6/

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review35211

This looks good - my only issue is that the interaction with the polling isn't clear and I think still needs work, and probably tests that calling checkVerificationStatus when already polling, and when polling has stopped, all do the right thing (ie, resolve the "singleton" promise correctly and don't send multiple ON_VERIFIED notifications.)

I think we should also open a followup bug to collect telemetry on the polling and push - we aren't going to have any insights into how verification was determined (ie, we will expect it to trypically happen via push, but for all we know that may fail to work as expected and the polling remains the dominant method)

::: services/fxaccounts/FxAccounts.jsm:319
(Diff revision 6)
> +  if (! internal.fxaPushService) {

please remove the space after the "!" (and I guess a comment that this is for tests can't hurt either)

::: services/fxaccounts/FxAccounts.jsm:351
(Diff revision 6)
> -  VERIFICATION_POLL_TIMEOUT_SUBSEQUENT: 15000, // 15 seconds.
> +  VERIFICATION_POLL_TIMEOUT_SUBSEQUENT: 20000, // 20 seconds.

I reckon we should make this 30 seconds (or maybe even 60?) - we expect push to work, right? :)

::: services/fxaccounts/FxAccounts.jsm:638
(Diff revision 6)
> +    });

ISTM we may already be polling when we get here, so just calling .pollEmailStatus again will do the wrong thing. Passing "start" as the 3rd param will be closer, but I think that will create a new .whenVerified promise, which will do the wrong thing (ie, when browserid_identity is already waiting for .whenVerified to resolve, it will hang as that promise never will). So sadly I think we need to little refactoring of the polling functions to handle this.

Also, I don't see how this.isUserEmailVerified() is expected to be true here - we haven't re-fetched the data from the server. So I think a comment saying something like "in the normal case, this is called after push told us the user is already verified, but this isn't going to be reflected in |data| yet - but just in-case we get a push after we know it has been verified, we check that here." or something, just so the casual reader doesn't expect data.verified to be true in the normal case.

::: services/fxaccounts/FxAccountsComponents.manifest:3
(Diff revision 6)
> +contract @mozilla.org/fxaccounts/push;1 {1b7db999-2ecd-4abf-bb95-a726896798ca}

This manifest needs an entry:

category push chrome://fxa-device-update @mozilla.org/fxaccounts/push;1

(and I hope it works with the scope formatted that way!) Do you think it would be possible to add a new test similar to dom/push/test/xpcshell/test_handler_service.js to ensure that the service is auto-loaded by the push service? It will probably need to be a stand-alone test so you can check the service was instantiated as expected.

::: services/fxaccounts/FxAccountsPush.js:11
(Diff revision 6)
> +Cu.import("resource://gre/modules/Promise.jsm");

you should be able to kill this Promise.jsm import so we are using builtin DOM promises

::: services/fxaccounts/FxAccountsPush.js:31
(Diff revision 6)
> +  // wrappedJSObject allows us to maintain a singleton service.

wrappedJSObject is actually so JS code that references the service can get to the internal service implementation (ie, to avoid needing to implement a custom xpcom interface for access to the service) - xpcom itself manages the singleton nature of the service.

ie, please remove or change this comment.

::: services/fxaccounts/FxAccountsPush.js:74
(Diff revision 6)
> +      XPCOMUtils.defineLazyServiceGetter(this, "pushService",

we don't need the lazy getter here as we reference the service just a few lines below (ie, just get the real service reference)

::: services/fxaccounts/FxAccountsPush.js:109
(Diff revision 6)
> +            this.log.warn("FxAccountsPush failed to subscribe");

let's write |result| to the log here.

::: services/fxaccounts/FxAccountsPush.js:187
(Diff revision 6)
> +            this.log.warn("FxAccountsPushService failed to unsubscribe");

write |result| here too.

::: services/fxaccounts/FxAccountsPush.js:202
(Diff revision 6)
> +this.EXPORTED_SYMBOLS = Object.keys(exports);

can't this just be written as |this.EXPORTED_SYMBOLS=["FxAccountsPushService"]|?

Comment 28

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35223/diff/6-7/

Comment 29

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35223/diff/7-8/

Comment 30

[Vlad Filippov (:vladikoff)]

https://reviewboard.mozilla.org/r/35223/#review35211

https://bugzilla.mozilla.org/show_bug.cgi?id=1249029

Comment 31

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review35265

great, thanks!

Comment 32

[Vlad Filippov (:vladikoff)]

Comment on attachment 8720129 [details]
MozReview Request: Bug 1247786 - Add push subscription to device registration r=markh

https://reviewboard.mozilla.org/r/35223/#review35267

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4221fa1f45c3

Comment 34

[Mark Hammond [:markh] [:mhammond]]

reopening until merged to -central

Comment 35

[Mark Hammond [:markh] [:mhammond]]

Vlad also raised the point that QA can't sanely verify this

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4221fa1f45c3