Closed Bug 1234054 Opened 4 years ago Closed 4 years ago

Enable Push on Desktop release builds

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed
b2g-v2.5 --- fixed
relnote-firefox --- 44+

People

(Reporter: Lina, Assigned: Lina)

References

Details

Attachments

(1 file, 2 obsolete files)

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
We should also enable `dom.webnotifications.serviceworker.enabled`.
Blocks: WADI
Attachment #8703818 - Flags: review?(ehsan)
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
Hmm, that was supposed to be r+, not sure what happened!
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8a8cfe20da543764a5d8a3d97af098e675c5d3
Bug 1234054 - Enable Push and service worker notifications on Desktop release builds. r=ehsan
Thanks, Ehsan!
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/cb8a8cfe20da
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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.
Attachment #8703818 - Flags: approval-mozilla-beta?
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.
Attachment #8703818 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Kit, do we need to uplift to Aurora45 as well? Or will 45 have this pref enabled once Aurora45 -> goes to Beta45?
Flags: needinfo?(kcambridge)
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. :-)
Flags: needinfo?(kcambridge)
Attachment #8703818 - Flags: approval-mozilla-aurora?
I missed that this patch turns on Notifications everywhere in my review.  It needs to only do that on desktop.
Attached patch 1234054.patch (obsolete) — Splinter Review
This patch exposes service worker notifications only on Desktop. Pushed to beta; requesting Aurora uplift per comment 11.
Attachment #8703818 - Attachment is obsolete: true
Attachment #8703818 - Flags: approval-mozilla-aurora?
Attachment #8704378 - Flags: approval-mozilla-aurora?
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 } }`?
Flags: needinfo?(ehsan)
Attachment #8704378 - Flags: approval-mozilla-aurora?
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.  :-)
Flags: needinfo?(ehsan)
(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 :)
Flags: needinfo?(kcambridge)
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.
Flags: needinfo?(kcambridge)
Attachment #8703818 - Attachment description: MozReview Request: Bug 1234054 - Enable Push and service worker notifications on Desktop release builds. r?ehsan → MozReview Request: Bug 1234054 - Only enable service worker notifications on Desktop and non-release B2G/Android. r?ehsan
Attachment #8703818 - Attachment is obsolete: false
Attachment #8703818 - Flags: review?(ehsan)
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/
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 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...
Attachment #8703818 - Flags: review?(ehsan)
Attachment #8703818 - Attachment description: MozReview Request: Bug 1234054 - Only enable service worker notifications on Desktop and non-release B2G/Android. r?ehsan → MozReview Request: Bug 1234054 - Expose notification interfaces to service workers; disable on B2G and Android. r?ehsan
Attachment #8703818 - Flags: review?(ehsan)
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/
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 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!
Attachment #8703818 - Flags: review?(ehsan) → review+
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].
Attachment #8703818 - Flags: approval-mozilla-aurora?
Kit, could you fill the uplift form? Thanks
Flags: needinfo?(kcambridge)
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.
Flags: needinfo?(kcambridge)
Attachment #8703818 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached patch beta.patchSplinter Review
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. :-(
Attachment #8703818 - Attachment is obsolete: true
Attachment #8704378 - Attachment is obsolete: true
Attachment #8713364 - Flags: review?(ehsan)
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.
Flags: needinfo?(kcambridge)
(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.
Flags: needinfo?(kcambridge)
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.
Flags: needinfo?(kcambridge)
(And granting approval is owned by the release managers, not me.  :-)
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.
Flags: needinfo?(kcambridge)
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.
Attachment #8713364 - Flags: review?(ehsan) → approval-mozilla-beta?
Comment on attachment 8713364 [details] [diff] [review]
beta.patch

Taking it as consistency with other releases.
Should be in 45 beta 2.
Attachment #8713364 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Next time, please create a new bug to address the issue itself.
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/
relnote-firefox: --- → ?
Added to Fx44 release notes. Not sure how this got missed out from all the reviews!!!
You need to log in before you can comment on or make changes to this bug.