SimplePush: Enable PushService on desktop/android

RESOLVED DUPLICATE of bug 871372

Status

()

defect
RESOLVED DUPLICATE of bug 871372
6 years ago
4 months ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

unspecified
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

PushService.jsm (once 863598 lands) should be included in browser/base/content/browser.js based on "services.push.enabled" being true.

Add a uninit function to PushService that will remove observers.

This is a simple first bug.
Posted patch Patch (obsolete) — Splinter Review
Attachment #743572 - Flags: review?(doug.turner)
This patch does not enable push because the preference is still disabled. Patch for bug 857464 does that.
Comment on attachment 743572 [details] [diff] [review]
Patch

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

::: browser/base/content/browser.js
@@ +1361,5 @@
>        OfflineApps.uninit();
>        IndexedDBPromptHelper.uninit();
>        AddonManager.removeAddonListener(AddonsMgrListener);
>        SocialUI.uninit();
> +      PushService.uninit();

Is this executed each time a browser window is closed, or only when the last browser window is closed?
Whiteboard: [good-first-bug]
Posted patch Patch (obsolete) — Splinter Review
@flo: thanks for that, that was indeed running on closing a window.

Turns out the code already watched profile-change-teardown, so I merged uninit() into _shutdown(), which now closes down the database and cancels timers and all sockets.
Attachment #743572 - Attachment is obsolete: true
Attachment #743572 - Flags: review?(doug.turner)
Attachment #744159 - Flags: review?(doug.turner)
Comment on attachment 744159 [details] [diff] [review]
Patch

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

::: browser/base/content/browser.js
@@ +7,5 @@
>  let Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource:///modules/RecentWindow.jsm");
> +Cu.import("resource://gre/modules/PushService.jsm");

make sure you don't leak anything.

::: dom/push/src/PushService.jsm
@@ +496,5 @@
> +    if (this._retryTimeoutTimer)
> +      this._retryTimeoutTimer.cancel();
> +
> +    if (this._requestTimeoutTimer)
> +      this._requestTimeoutTimer.cancel();

isn't the style :

if (t) {
  stmt;
}
Attachment #744159 - Flags: review?(doug.turner) → review+
Will try complain if the code leaks? I can't find any push specific leaks on local builds.
yup, it should.
Posted patch PatchSplinter Review
Moved some shutdown code into bug 863598 for more coherence.

carrying forward r=dougt.
Attachment #744159 - Attachment is obsolete: true
Attachment #744464 - Flags: review+
land once bug 863598 makes its way to m-c.
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/e7ca39ff01cc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
browser.js is part of the Firefox module, and so you need to get Firefox peer review before changing it.

I don't think this is the right way to initialize the service. There's no need to pollute the Firefox window scope with PushService.jsm's symbol, and having import() cause side-effects (triggering initialization of the service) is also generally frowned upon. It seems odd to want to have this be Firefox-desktop-specific to begin with, but if that's really what you want then having the initialization occur (explicitly) in nsBrowserGlue is probably a better idea.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The patch in bug 871372 implements this.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 871372
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.