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

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla48
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Component: General → DOM: Push Notifications
Product: Firefox for Android → Core
(Assignee)

Comment 1

2 years ago
overholt: can you redirect this to somebody who can guide me through?  Thanks!
Flags: needinfo?(overholt)
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
(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.
Flags: needinfo?(overholt) → needinfo?(ehsan)
Keywords: dev-doc-needed

Comment 5

2 years ago
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.
Flags: needinfo?(ehsan) → needinfo?(kcambridge)
Nick, I set you to the assignee but feel free to change that if it's incorrect.
Assignee: nobody → nalexander
Whiteboard: btpp-active
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.
Flags: needinfo?(kcambridge)
(Assignee)

Updated

2 years ago
Blocks: 1252650
(Assignee)

Comment 8

2 years ago
Created attachment 8727646 [details]
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/
Attachment #8727646 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 9

2 years ago
Created attachment 8727647 [details]
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/
Attachment #8727647 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 10

2 years ago
Created attachment 8727648 [details]
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/
Attachment #8727648 - Flags: review?(kcambridge)
Attachment #8727648 - Flags: review?(ehsan)
(Assignee)

Comment 11

2 years ago
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/
Attachment #8727648 - Attachment description: MozReview Request: Bug 1252666 - Part 3: Mark Push{Manager,Subscription} exposed in Fennec Nightly. r?ehsan,kitcambridge → MozReview Request: Bug 1252666 - Part 1: Mark Push{Manager,Subscription} exposed in Fennec Nightly. r?ehsan,kitcambridge
(Assignee)

Comment 12

2 years ago
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/
Attachment #8727646 - Attachment description: MozReview Request: Bug 1252666 - Part 1: Use reflection to start PushService. r?margaret → MozReview Request: Bug 1252666 - Part 2: Use reflection to start PushService. r?margaret
(Assignee)

Comment 13

2 years ago
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/
Attachment #8727647 - Attachment description: MozReview Request: Bug 1252666 - Part 2: Enable DOM Push API in Fennec Nightly. r?margaret → MozReview Request: Bug 1252666 - Part 3: Enable DOM Push API in Fennec Nightly. r?margaret
(Assignee)

Updated

2 years ago
Attachment #8727648 - Attachment description: MozReview Request: Bug 1252666 - Part 1: Mark Push{Manager,Subscription} exposed in Fennec Nightly. r?ehsan,kitcambridge → MozReview Request: Bug 1252666 - Part 1: Mark Push* exposed in Fennec Nightly. r?ehsan,kitcambridge
(Assignee)

Comment 16

2 years ago
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/
(Assignee)

Comment 17

2 years ago
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/
(Assignee)

Comment 18

2 years ago
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

2 years ago
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.
Attachment #8727648 - Flags: review?(ehsan)
(Assignee)

Comment 20

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

Comment 21

2 years ago
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/
Attachment #8727648 - Flags: review?(ehsan)
(Assignee)

Comment 22

2 years ago
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/
(Assignee)

Comment 23

2 years ago
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

2 years ago
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?
Attachment #8727648 - Flags: review?(ehsan) → review+

Comment 25

2 years ago
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.
Attachment #8727646 - Flags: review?(margaret.leibovic) → review+

Updated

2 years ago
Attachment #8727647 - Flags: review?(margaret.leibovic) → review+

Comment 26

2 years ago
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.
(Assignee)

Comment 28

2 years ago
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.
Attachment #8727648 - Flags: review?(kcambridge)

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16e447df3972
https://hg.mozilla.org/mozilla-central/rev/4e99d0086c3b
https://hg.mozilla.org/mozilla-central/rev/82efa3bc8731
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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!
Flags: needinfo?(nalexander)
(Assignee)

Comment 31

2 years ago
(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.
Flags: needinfo?(nalexander)
(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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.