1234054 - Enable Push on Desktop release builds

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

Push is currently pref'd off on release builds. This ticket tracks turning Push on by default for Desktop (Android is targeting 46, and service workers are currently disabled on B2G). We'll need to uplift to beta.

Intent to ship thread: https://groups.google.com/d/topic/mozilla.dev.platform/vU4NsuKhTOY/discussion

Comment 1

[Lina Butler [:lina]]

We should also enable `dom.webnotifications.serviceworker.enabled`.

Comment 2

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

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

Comment 3

[(no longer active)]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

https://reviewboard.mozilla.org/r/29477/#review26241

Comment 4

[(no longer active)]

Hmm, that was supposed to be r+, not sure what happened!

Comment 5

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8a8cfe20da543764a5d8a3d97af098e675c5d3
Bug 1234054 - Enable Push and service worker notifications on Desktop release builds. r=ehsan

Comment 6

[Lina Butler [:lina]]

Thanks, Ehsan!

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb8a8cfe20da

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

Approval Request Comment
[Feature/regressing bug #]: Push notifications.
[User impact if declined]: Push and service worker notifications will be disabled in 44.
[Describe test coverage new/current, TreeHerder]: Updated DOM interface mochitests. This patch just exposes features already available in Nightly and Aurora.
[Risks and why]: QA and product have signed off on Push shipping in 44 Desktop. It will remain disabled on B2G and Android.
[String/UUID change made/needed]: None.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

beta44+ for obvious reasons.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Kit, do we need to uplift to Aurora45 as well? Or will 45 have this pref enabled once Aurora45 -> goes to Beta45?

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

I think we do want to uplift to Aurora, too, so that this makes it to Beta 45. Thanks, Ritu! And thank you for all the other uplifts. It's exciting to see this ship. :-)

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/aa03498e8961

Comment 13

[(no longer active)]

I missed that this patch turns on Notifications everywhere in my review.  It needs to only do that on desktop.

Comment 14

[Lina Butler [:lina]]

Attachment: 1234054.patch

This patch exposes service worker notifications only on Desktop. Pushed to beta; requesting Aurora uplift per comment 11.

Comment 15

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c634b57f2b0f

Comment 16

[Lina Butler [:lina]]

Comment on attachment 8704378 [details] [diff] [review]
1234054.patch

Looks like `{ b2g: false, android: false }` fixed the bustage on beta, but fails for Android and Mulet on that try push. Guessing it'll fail when uplifted to Aurora, too.

I don't see a way to say "enabled on all desktop, but only non-release Android/B2G." Should we just disable them on the latter? Or extend the syntax in `createInterfaceMap` to support something like `{ b2g: { release: false, nightly: true } }`?

Comment 17

[(no longer active)]

I think first you need to decide exactly where you want this enabled, and then add the appropriate conditions to test_interfaces.html and friends as needed.  For example in bug 1204397 I added a nightlyAndroid attribute to denote that my feature is enabled on Android nightly, but disabled on Android Aurora onwards (by saying |{nightlyAndroid: true, android: false}|.  We can add similar attributes like nonReleaseAndroid and nonReleaseB2G which sound like what you'd need.

The situation on Mulet is... sad.  I never really understood what it is supposed to be.  Please ask someone on #b2g to help tell you what Mulet is.

But please make sure to post the patch based off all of those branches to try since I found it tricky to maintain the matrix of where we expect what correctly in my head and went through several iterations.  :-)

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kit Cambridge [:kitcambridge] from comment #14)
> Created attachment 8704378 [details] [diff] [review]
> 1234054.patch
> 
> This patch exposes service worker notifications only on Desktop. Pushed to
> beta; requesting Aurora uplift per comment 11.

Kit, it is indeed nice to see this ship! Could you please add a link to the beta push that changed push notifications to be pref'd on by default on desktop only in this bug? This will be useful for historical record keeping and a way to reconfirm :)

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/aa03498e8961
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/557c92e190be

Comment 20

[Lina Butler [:lina]]

https://hg.mozilla.org/releases/mozilla-beta/rev/557c92e190be

This is OK on Beta, but needs a follow-up for Aurora and Nightly. Apologies for the delay; I got sidetracked by other bugs.

Comment 21

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9f47ce14259

Comment 22

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f9c605c45bc

Comment 23

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6b49ec1cd97

Comment 24

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb059c088941

Comment 25

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b78330c300fb

Comment 26

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2370a716fa9e

Comment 27

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49989708eebd

Comment 28

[Lina Butler [:lina]]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

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

Comment 29

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/29477/#review27345

::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js:160
(Diff revision 2)
> -    { name: "Notification", release: false },
> +    { name: "Notification", nightlyB2G: true, auroraB2G: true,

I decided to keep Nightly and Aurora separate because `{ nonReleaseB2G: true, b2g: false }` made me do a double-take every time I saw it. Then again, I don't think we'll ever expose something to Aurora and/or Release, but not Nightly, so maybe it's OK. Your call.

Comment 30

[(no longer active)]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

https://reviewboard.mozilla.org/r/29477/#review27343

r- becuase of the issues below, except that MozReview doesn't know how to r-. :-)

::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js:219
(Diff revision 2)
> -  var isRelease = !version.includes("a");
> +  var isAurora = version.includes("a") && !isNightly;

This doesn't account for "a2" which can happen on Beta, so isAurora doesn't actually mean "Aurora" here.

::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js:237
(Diff revision 2)
> +           true;

Can you please use if/else instead?  It's very difficult to follow the logic in this function.

::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js:251
(Diff revision 2)
> +            (isAurora && isB2G && entry.auroraB2G === false) ||

I'm not sure if I understand the goal here.  The rest of the patch enables this unconditionally on desktop, and on Android and b2g it enables it only for non-release builds.  Based on that, I would expect to see a nonReleaseAndroid and nonReleaseB2G property here, similar to <https://hg.mozilla.org/mozilla-central/rev/65cec42b60fa#l1.39>.  I'm not sure why you have separate flags for Nightly and Aurora...

Comment 31

[Lina Butler [:lina]]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

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

Comment 32

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/29477/#review27343

> I'm not sure if I understand the goal here.  The rest of the patch enables this unconditionally on desktop, and on Android and b2g it enables it only for non-release builds.  Based on that, I would expect to see a nonReleaseAndroid and nonReleaseB2G property here, similar to <https://hg.mozilla.org/mozilla-central/rev/65cec42b60fa#l1.39>.  I'm not sure why you have separate flags for Nightly and Aurora...

You're right. I think the double negative in `!(isB2G && !isRelease)` thoroughly confused me, so I wanted to make it explicit that the interfaces were exposed to Nightly and Aurora. But the check is more complicated and wrong. :-)

Try runs look good for Beta [https://treeherder.mozilla.org/#/jobs?repo=try&revision=da7b0a6e4d9e], Aurora [https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7a4080e1d16], and Nightly [https://treeherder.mozilla.org/#/jobs?repo=try&revision=477e0177dd91].

Comment 33

[(no longer active)]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

https://reviewboard.mozilla.org/r/29477/#review27523

Looks good!

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bc0334f3ea8d

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc0334f3ea8d

Comment 36

[Lina Butler [:lina]]

Comment on attachment 8703818 [details]
MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan

Re-requesting approval for Aurora 45 per comment 11. The test should pass this time [https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7a4080e1d16].

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Kit, could you fill the uplift form? Thanks

Comment 38

[Lina Butler [:lina]]

Sure!

Approval Request Comment
[Feature/regressing bug #]: Push notifications.
[User impact if declined]: Push is already enabled on non-release builds and Beta 44. Requesting uplift to Aurora so that it remains enabled when Aurora 45 -> Beta 45.
[Describe test coverage new/current, TreeHerder]: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js
[Risks and why]: Low risk; already enabled on Beta 44 and Aurora/Nightly 44+.
[String/UUID change made/needed]: None.

Comment 39

[Lina Butler [:lina]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/737f3b6e93c6

Comment 40

[Lina Butler [:lina]]

Attachment: beta.patch

So...I uplifted the patch to enable service worker notifications on 45, but not push. The current state of things is:

* On 44 release builds, push and worker notifications are enabled.
* On 45 release, worker notifications are enabled, but push is disabled.
* On 46, both push and notifications are enabled again.

The attached patch enables push on 45, and should only be checked in to mozilla-beta. I thought 44 would be merged back in to 45 on release day, so both would be enabled, but I was wrong. I'm very sorry for muddling this. :-(

Comment 41

[(no longer active)]

Why is it that we want to keep push enabled in 44 and 46 but not 45?  I'm not sure I'm following what we're trying to achieve here.

Comment 42

[Lina Butler [:lina]]

(In reply to :Ehsan Akhgari from comment #41)
> Why is it that we want to keep push enabled in 44 and 46 but not 45?

We don't. Right now, push is disabled in 45 release, because I didn't realize I had to uplift both (I thought the 44 branch would be merged back in to 45 on release day, but that's wrong). The patch enables push in 45.

Comment 43

[(no longer active)]

So are you asking me to backport the patch for you?  It looks I've reviewed this before.  If the patch hasn't changed, and all you need is the backport, you need to ask for approval, not review.

Comment 44

[(no longer active)]

(And granting approval is owned by the release managers, not me.  :-)

Comment 45

[Lina Butler [:lina]]

I had to change the patch to get it to apply cleanly, so that's why I asked you for another review. :-) Sorry for the noise.

Comment 46

[Lina Butler [:lina]]

Comment on attachment 8713364 [details] [diff] [review]
beta.patch

Approval Request Comment
[Feature/regressing bug #]: This bug.
[User impact if declined]: Push is currently disabled on Beta 45 due to an incomplete uplift.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9e4271e1d71
[Risks and why]: Low risk, since this is already enabled on 44 and 46.
[String/UUID change made/needed]: None.

Comment 47

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8713364 [details] [diff] [review]
beta.patch

Taking it as consistency with other releases.
Should be in 45 beta 2.

Comment 48

[Sylvestre Ledru [:Sylvestre]]

Next time, please create a new bug to address the issue itself.

Comment 49

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-beta/rev/486d7f18d276

Comment 50

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: Firefox Can Now Get Push Notifications From Your Favorite Sites
[Links (documentation, blog post, etc)]: https://blog.mozilla.org/blog/2016/01/25/firefox-can-now-get-push-notifications-from-your-favorite-sites/

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx44 release notes. Not sure how this got missed out from all the reviews!!!