1131813 - replacing Loop Push with standard Simple Push

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect, P2)

Description

[:shell escalante]

discussion in https://etherpad.mozilla.org/hello-push-jan-2015

move from maintaining our own to using the standard simple push client. benefit is don't have to maintain ourselves and less likely to break during updates. 

Loop timing for ~2 weeks of tech-debt work (and then regressions). 

target for early fx40 to coordinate with Push team work.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: 0001-WIP-Add-notification-connection-state-observers.patch

WIP, part one: adding observer notifications for when the client connects and disconnects. This is only currently implemented for the WebSocket client; I haven't added it to the H/2 client yet.

In this patch, `nsIPushNotificationService` grows `{add, remove}Observer` methods that take an `nsIPushNotificationObserver` as the first argument. Observers can receive Simple Push (`onPushVersion`) or Web Push (`onPushData`) messages, and are restricted to a `(scope, originAttributes)` pair. There's no more global broadcasting of messages, and you no longer need to check if a push message is intended for you or not. This is one of the sticking points of the current API.

Observers can also define `onOpen` and `onClose` handlers to listen for when the connection is established or torn down. `onClose` receives the estimated reconnect time as its only argument (or -1 if we don't know).

This still needs work to get it over the finish line, but I think I'm finally happy with the new API.

Comment 2

[Lina Butler (ex-Mozilla)]

Attachment: 0002-Initial-Loop-Push-client-migration.patch

WIP, part two: the actual client migration. Testing this is going to be interesting; I'll probably just mock `nsIPushNotificationService` directly, instead of trying to use a mock WebSocket with it.

Ben, if you have some cycles next week, this might be a good starting point to see (and critique!) how chrome code could use the Push client. If not, no worries; just wanted to post the patches so they don't get lost in my local tree...again. :-/

Comment 3

[u549168]

We're deprioritizing this right now, to focus on getting the Push DOM API generally available (for example some of those changes will invalidate this patch).  We're planning to revisit when metrics shows a significant number of duplicate connections due to the two parallel sets of client code, or when someone on the Hello side raises the question of the priority of this issue.

Comment 4

[Mark Banner (:standard8)]

(In reply to ibicking from comment #3)
> We're deprioritizing this right now, to focus on getting the Push DOM API
> generally available (for example some of those changes will invalidate this
> patch).  We're planning to revisit when metrics shows a significant number
> of duplicate connections due to the two parallel sets of client code, or
> when someone on the Hello side raises the question of the priority of this
> issue.

Do you have a rough timescale on the Push DOM API?

There's some things I was thinking that we should be handling that we're not currently (like displaying connection failures to the user), and I was partially waiting on getting this fixed up before doing that. If its a shorter timescale, then maybe it can wait a bit longer.

Comment 5

[Ian Bicking (:ianbicking)]

We're targeting 43 for the DOM API.  I would expect we'd continue to refine things in at least 44, though that's in the realm of unknown unknowns, i.e., we don't know what issues will emerge that will have to be addressed to make the DOM API fully successful.  Maybe we'd be able to revisit this for 45 (though I feel like I'm guessing here).

Comment 6

[Andrew Overholt [:overholt]]

(Updating to reflect dependency on standard Push API and not Simple Push)

Comment 7

[Ian Bicking (:ianbicking)]

As part of this we may want to

Comment 8

[Ian Bicking (:ianbicking)]

Kit: what's the status on this patch?  I think the Push Service code has changed significantly since the patch.  

We don't need Bug 1206190 to land this, that's only a nice-to-have.

Comment 9

[Lina Butler (ex-Mozilla)]

(In reply to Ian Bicking (:ianb) from comment #8)
> Kit: what's the status on this patch?  I think the Push Service code has
> changed significantly since the patch.

It doesn't seem too bad. At first glance, `register` and `unregister` have new names (and use callbacks instead of promises), and notification versions aren't used anymore. We'll also need to disable the quota for Hello subscriptions. The FxA folks are integrating Push, too, so now might be a good time to pick this up again.

I also haven't looked at the tests, besides removing `test_looppush_initialize.js`. If the other tests mock out `MozLoopPushHandler`, or if it's not needed, we should be OK.

Going to keep ni? until this is rebased.

> We don't need Bug 1206190 to land this, that's only a nice-to-have.

Cool, good to know.

Comment 10

[Chris Karlof [:ckarlof]]

Ian what's the priority of this? What's the impact of not doing it?

Comment 11

[Mark Banner (:standard8)]

(In reply to Chris Karlof [:ckarlof] from comment #10)
> Ian what's the priority of this? What's the impact of not doing it?

If we don't do this, then we have "duplicated" code for connecting to push servers - the core mechanism and Loop's specific one. Hence causing a slightly bigger download size, and extra maintenance.

There's also likely to be some more experience of working with the push servers in the core code, so it may be more efficient/stable than using Hello's home grown version.

Comment 12

[Ian Bicking (:ianbicking)]

The priority is not particularly high.  It causes some inefficiency for the push service; clients that use Hello and Webpush will have two connections open instead of one.  Also I'm concerned about the patch bitrotting further.  It will become more critical if the push service wants to change the protocol or deprecate the protocol that Hello is using.  Ben might be able to comment on it from that perspective.  

For Kit, note that Loop is now developed in https://github.com/mozilla/loop

Comment 13

[Lina Butler (ex-Mozilla)]

Placing this on the backburner for now.

Comment 14

[Ben Bangert [:benbangert]]

Loop has consistently used a few hundred thousand connections, and we haven't seen any growth in that number for the past 6 months or so. Out of millions of connections we anticipate with WebPush, leaving Loop on their own client for awhile will not be a big deal from an infrastructure perspective.

There is possibly a QA issue here though, since having a custom client like Hello connect to Push means Push QA has to test Hello as well as WebPush every time we do a Push deploy.

Comment 15

[Ian Bicking (:ianbicking)]

Another option to consider here is to switch to the DOM WebPush API.  If we did that then one of our about: pages would register with Push and have an associated Service Worker to handle the push message.  We'd have to confirm that this works reasonably from about about: page.

Comment 16

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.