1252666 - Flip dom.push.enabled to true in Firefox for Android

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

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

Comment 2

[Nick Alexander :nalexander [he/him]]

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.

Comment 3

[Nick Alexander :nalexander [he/him]]

(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+.

Comment 4

[Andrew Overholt [:overholt]]

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

Comment 5

[(no longer active)]

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.

Comment 6

[Andrew Overholt [:overholt]]

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

Comment 7

[Lina Butler [:lina]]

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.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1252666 - Part 2: Use reflection to start PushService. r?margaret

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/

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1252666 - Part 3: Enable DOM Push API in Fennec Nightly. r?margaret

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

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge

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

Comment 11

[Nick Alexander :nalexander [he/him]]

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 12

[Nick Alexander :nalexander [he/him]]

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 13

[Nick Alexander :nalexander [he/him]]

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/

Comment 14

[Nick Alexander :nalexander [he/him]]

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

Comment 15

[Lina Butler [:lina]]

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 16

[Nick Alexander :nalexander [he/him]]

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 17

[Nick Alexander :nalexander [he/him]]

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 18

[Nick Alexander :nalexander [he/him]]

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 19

[(no longer active)]

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.

Comment 20

[Nick Alexander :nalexander [he/him]]

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 21

[Nick Alexander :nalexander [he/him]]

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 22

[Nick Alexander :nalexander [he/him]]

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 23

[Nick Alexander :nalexander [he/him]]

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 24

[(no longer active)]

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 25

[:Margaret Leibovic]

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 26

[:Margaret Leibovic]

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.

Comment 27

[Pulsebot]

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 28

[Nick Alexander :nalexander [he/him]]

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.

Comment 29

[Carsten Book [:Tomcat]]

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

Comment 30

[Chris Mills [:cmills]]

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!

Comment 31

[Nick Alexander :nalexander [he/him]]

(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.

Comment 32

[Chris Mills [:cmills]]

(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.