Closed
Bug 863599
Opened 10 years ago
Closed 9 years ago
SimplePush: Enable PushService on desktop/android
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
DUPLICATE
of bug 871372
mozilla23
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(1 file, 2 obsolete files)
2.34 KB,
patch
|
nsm
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #743572 -
Flags: review?(doug.turner)
Assignee | ||
Comment 2•9 years ago
|
||
This patch does not enable push because the preference is still disabled. Patch for bug 857464 does that.
Comment 3•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Whiteboard: [good-first-bug]
Assignee | ||
Comment 4•9 years ago
|
||
@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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
Will try complain if the code leaks? I can't find any push specific leaks on local builds.
Comment 7•9 years ago
|
||
yup, it should.
Assignee | ||
Comment 8•9 years ago
|
||
Moved some shutdown code into bug 863598 for more coherence. carrying forward r=dougt.
Attachment #744159 -
Attachment is obsolete: true
Attachment #744464 -
Flags: review+
Assignee | ||
Comment 9•9 years ago
|
||
land once bug 863598 makes its way to m-c.
Assignee | ||
Comment 10•9 years ago
|
||
Don't land until leak is fixed (see bug 863598 comment 9).
Updated•9 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 11•9 years ago
|
||
Try run with nothing going wrong - https://tbpl.mozilla.org/?tree=Try&rev=d1cb326a55a3
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ca39ff01cc
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e7ca39ff01cc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment 14•9 years ago
|
||
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.
Comment 15•9 years ago
|
||
I filed bug 871372.
Assignee | ||
Comment 16•9 years ago
|
||
Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/5f4da66d8655
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 17•9 years ago
|
||
The patch in bug 871372 implements this.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 871372
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•