Closed Bug 1278435 Opened 4 years ago Closed 4 years ago

Enable Push interfaces on Android

Categories

(Core :: DOM: Push Notifications, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 2 obsolete files)

DOM interface tests are failing on Aurora because Push is now enabled on all Android channels: https://treeherder.mozilla.org/logviewer.html#?job_id=2763633&repo=mozilla-aurora
And that's a blocker since we aren't going to open mozilla-aurora with "DANGER..." test failures.
Severity: normal → blocker
i guess this depend on bug 127509, Ningjie, snorp can you take a look at this so that we are able to reopen mozilla-aurora again ?
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
Kit, are you going to take this?
Flags: needinfo?(kcambridge)
Sure, I'll take care of it now. Was away from my work machine when I filed the bug.
Assignee: nobody → kcambridge
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
Flags: needinfo?(kcambridge)
Whiteboard: btpp-active
I think we should also enable service worker notifications (get rid of the #if here: http://searchfox.org/mozilla-central/rev/3f096f780166bc4772199d79a79c8e3541ff99be/modules/libpref/init/all.js#4628-4630), since Push is riding the trains. Persistent notifications are waiting on bug 1264815, but at least `serviceWorkerRegistration.showNotification()` will be available.

William, what do you think?
Flags: needinfo?(wchen)
(In reply to Kit Cambridge [:kitcambridge] from comment #5)
> I think we should also enable service worker notifications (get rid of the
> #if here:
> http://searchfox.org/mozilla-central/rev/
> 3f096f780166bc4772199d79a79c8e3541ff99be/modules/libpref/init/all.js#4628-
> 4630), since Push is riding the trains. Persistent notifications are waiting
> on bug 1264815, but at least `serviceWorkerRegistration.showNotification()`
> will be available.
> 
> William, what do you think?

It was a product decision to disable the feature [1], I think mainly because of bug 1130687. I wouldn't mind having it turned on because I think there is still value in the feature even if bug 1130687 isn't implemented.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1203324#c9
Flags: needinfo?(wchen)
Missed one.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7984f6b3d13e
Attachment #8760915 - Attachment is obsolete: true
Attachment #8760915 - Flags: review?(amarchesini)
Attachment #8761019 - Flags: review?(amarchesini)
Attached patch notifsEverywhere.patch (obsolete) — Splinter Review
Enabling service worker notifications everywhere except non-release B2G, because I'm not sure what the experience is like there. This patch can be even simpler if you think it's ready to enable there, too.
Attachment #8761021 - Flags: review?(wchen)
Attachment #8761021 - Flags: review?(amarchesini)
Attachment #8761019 - Flags: review?(amarchesini) → review+
Comment on attachment 8761021 [details] [diff] [review]
notifsEverywhere.patch

Review of attachment 8761021 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +1061,5 @@
>  pref("dom.serviceWorkers.enabled", false);
>  pref("dom.push.enabled", false);
>  
> +#if defined(RELEASE_BUILD)
> +pref("dom.webnotifications.serviceworker.enabled", false);

else
pref("dom.webnotifications.serviceworker.enabled", true);

@@ +1062,5 @@
>  pref("dom.push.enabled", false);
>  
> +#if defined(RELEASE_BUILD)
> +pref("dom.webnotifications.serviceworker.enabled", false);
> +#endif

Do we have a bug about enabling this for b2g? We should. File it in case.
Attachment #8761021 - Flags: review?(amarchesini) → review+
Keywords: leave-open
This will need to go to beta once bug 1275095 is uplifted. Requesting tracking because it'll cause m-b to permafail if it's not.
Status: NEW → ASSIGNED
Keywords: leave-open
Also, I'm going to move part 2 to another bug, because it needs to land on m-c, m-a, and m-b.
Blocks: 1278851
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d48d9a24ce8b
Update DOM interface tests to reflect shipping Push on Android. r=baku
(In reply to Andrea Marchesini (:baku) from comment #10)
> else
> pref("dom.webnotifications.serviceworker.enabled", true);

Fixed in attachment 8761213 [details] [diff] [review].

> Do we have a bug about enabling this for b2g? We should. File it in case.

Filed bug 1278848.
Attachment #8761021 - Attachment is obsolete: true
Attachment #8761021 - Flags: review?(wchen)
https://hg.mozilla.org/mozilla-central/rev/d48d9a24ce8b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.