In a recent try job, I enabled DOM Push in Fennec: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69e3ab293c88&selectedJob=17241858 I ran afoul of tests designed to catch exactly this -- exposing new symobls to web content. This ticket tracks doing whatever is needed to expose these symbols safely to the world.

overholt: can you redirect this to somebody who can guide me through? Thanks!

Looks like the code part isn't so hard: https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html#971. However, we want Android to be Nightly-only on day one. Not clear how to arrange that given that this is no longer Nightly-only outside of Android.

(In reply to Nick Alexander :nalexander from comment #2) > Looks like the code part isn't so hard: > https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/ > test_interfaces.html#971. > > However, we want Android to be Nightly-only on day one. Not clear how to > arrange that given that this is no longer Nightly-only outside of Android. Oh yeah -- this will depend on the version of Android, too. It's off in API 9-10, on in API 15+. If we slip to 48, there's only API 15+.

Ehsan can help. If there hasn't been an intent to ship for fennec sent yet, that'd be good to do soon.

Hmm, I'm not 100% sure what the question here is... If it's how to change test_interfaces.html and friends correctly, it depends on the exact environment in which push is enabled. For example, if you want it enabled on Nightly Fennec but not Aurora+, you'd need to reintroduce the nightlyAndroid flag that I added in bug 1204397 (and subsequently removed in bug 1215601.) If you need to have it on only in API 15+, you'd need to add a similar flag which will be true on API 15+ but false otherwise. I have no idea how we could do that... Maybe by looking at the UA string? If the question is what we need to do to actually ensure that push works the way we want it to, I'm not familiar with the details. Perhaps Kit can help you with that. AFAIK there was going to be some Android specific work to be done as well (bug 1179015). Not sure what all of such dependencies are.

Nick, I set you to the assignee but feel free to change that if it's incorrect.

Nick, if you haven't found these already, I think we'll need `nightlyAndroid` here, too: https://dxr.mozilla.org/mozilla-central/rev/eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/dom/workers/test/serviceworkers/test_serviceworker_interfaces.js#175-182 I guess we'll end up with `{ b2g: false, nightlyAndroid: true, android: false }` for Push.

The alternative is to define an interface and two conditional implementations, and then build only one depending on MOZ_ANDROID_GCM. That's what we did for Adjust, and it works; but it's awkward here because the the PushService code is truly part of the browser, and the conditional code is compiled very early (much earlier than the browser). (The Adjust library was built even earlier than the existing conditional code, so this wasn't an issue.) To work around this, we'd want to add conditional code to the main browser, which complicates the build. This is expedient until we get to a proper dependency injection scheme (for example, Dagger). Review commit: https://reviewboard.mozilla.org/r/38573/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38573/

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

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

Comment on attachment 8727648 [details] MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38577/diff/1-2/

Comment on attachment 8727646 [details] MozReview Request: Bug 1252666 - Part 2: Use reflection to start PushService. r?margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38573/diff/1-2/

Comment on attachment 8727647 [details] MozReview Request: Bug 1252666 - Part 3: Enable DOM Push API in Fennec Nightly. r?margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38575/diff/1-2/

I think https://dxr.mozilla.org/mozilla-central/rev/be593a64d7c6a826260514fe758ef32a6ee580f7/dom/workers/test/serviceworkers/test_serviceworker_interfaces.js#175-182 needs an update, too.

Comment on attachment 8727648 [details] MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38577/diff/2-3/

Comment on attachment 8727646 [details] MozReview Request: Bug 1252666 - Part 2: Use reflection to start PushService. r?margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38573/diff/2-3/

Comment on attachment 8727647 [details] MozReview Request: Bug 1252666 - Part 3: Enable DOM Push API in Fennec Nightly. r?margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38575/diff/2-3/

Comment on attachment 8727648 [details] MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge https://reviewboard.mozilla.org/r/38577/#review35333 r- for the below. ::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js:176 (Diff revision 3) > - { name: "PushEvent", b2g: false, android: false }, > + { name: "PushEvent", b2g: false, android: false, nonReleaseAndroid: true }, nonReleaseAndroid also covers Aurora in addition to Nightly, so you don't want to use that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33951b429b7b shows that the approach is reasonable. I'm bumping the "nonReleaseAndroid" bits now. Thanks, ehsan.

Comment on attachment 8727648 [details] MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38577/diff/3-4/

Comment on attachment 8727646 [details] MozReview Request: Bug 1252666 - Part 2: Use reflection to start PushService. r?margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38573/diff/3-4/

Comment on attachment 8727647 [details] MozReview Request: Bug 1252666 - Part 3: Enable DOM Push API in Fennec Nightly. r?margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38575/diff/3-4/

Comment on attachment 8727648 [details] MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge https://reviewboard.mozilla.org/r/38577/#review35387 r+ with the comment below addressed. ::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js:235 (Diff revision 4) > (entry.android === !isAndroid && !entry.nonReleaseAndroid) || Don't you also need a |&& !entry.nightlyAndroid| here too?

Comment on attachment 8727646 [details] MozReview Request: Bug 1252666 - Part 2: Use reflection to start PushService. r?margaret https://reviewboard.mozilla.org/r/38573/#review35591 Approach seems fine to me. Disclaimer: not familiar with the PushService implementation details.

Comment on attachment 8727647 [details] MozReview Request: Bug 1252666 - Part 3: Enable DOM Push API in Fennec Nightly. r?margaret https://reviewboard.mozilla.org/r/38575/#review35593 ::: mobile/android/app/mobile.js:969 (Diff revision 4) > +#endif It's a bit confusing that this feature depends on both a build flag and a gecko pref, but I don't imagine anyone flipping this pref in about:config to expect it to work.

https://hg.mozilla.org/integration/fx-team/rev/16e447df3972 https://hg.mozilla.org/integration/fx-team/rev/4e99d0086c3b https://hg.mozilla.org/integration/fx-team/rev/82efa3bc8731

Comment on attachment 8727648 [details] MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge Landed with ehsan's comment addressed, so no need for kit's feedback.

https://hg.mozilla.org/mozilla-central/rev/16e447df3972 https://hg.mozilla.org/mozilla-central/rev/4e99d0086c3b https://hg.mozilla.org/mozilla-central/rev/82efa3bc8731

I've just gone and updated the browser compat info here for now: https://developer.mozilla.org/en-US/docs/Web/API/Push_API#Browser_compatibility Let me know if you think this reads ok, before I go and update all the other relevant pages ;-) One query — since before this point we had FxA support recorded as "No", do I even need to bother with the footnote? Shall I just get rid of footnote 4? Thanks!

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #30) > I've just gone and updated the browser compat info here for now: > > https://developer.mozilla.org/en-US/docs/Web/API/ > Push_API#Browser_compatibility > > Let me know if you think this reads ok, before I go and update all the other > relevant pages ;-) Fine by me! > One query — since before this point we had FxA support recorded as "No", do > I even need to bother with the footnote? Shall I just get rid of footnote 4? I'm sorry but I don't know house style. It doesn't hurt, in my opinion.

(In reply to Nick Alexander :nalexander from comment #31) > (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #30) > > I've just gone and updated the browser compat info here for now: > > > > https://developer.mozilla.org/en-US/docs/Web/API/ > > Push_API#Browser_compatibility > > > > Let me know if you think this reads ok, before I go and update all the other > > relevant pages ;-) > > Fine by me! Cool, I've updated all relevant pages with the new support info, including the footnote. > > One query — since before this point we had FxA support recorded as "No", do > > I even need to bother with the footnote? Shall I just get rid of footnote 4? > > I'm sorry but I don't know house style. It doesn't hurt, in my opinion. We don't really have a house style for this as such; some engineers prefer the data to be really exact, and some prefer brevity. Either is ok by me, as long as it makes sense to readers.