Closed
Bug 539997
Opened 14 years ago
Closed 13 years ago
remove nsTryToClose.js from Firefox
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 10
People
(Reporter: dietrich, Assigned: marco)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ts])
Attachments
(1 file, 6 obsolete files)
6.79 KB,
patch
|
Details | Diff | Splinter Review |
it's a toolkit js xpcom service that registers w/ app startup category *only* to register itself as an observer of quit-application-requested. it's also a tiny amount of code. maybe for Firefox we could fold it into nsBrowserGlue or something like that.
Comment 1•14 years ago
|
||
For Firefox, it should not be needed at all. It's mostly a backwards compatibility thing for apps (and maybe extensions) not listening to the right shutdown events, IIRC. For some reason http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/content/updates.js#1641 is using it but it shouldn't.
Reporter | ||
Comment 2•14 years ago
|
||
Rob, where'd be the best place in the update service to listen for the shutdown notification in order to do the wizard cancel that way? Can I just make gUpdates observe it?
Reporter | ||
Comment 3•14 years ago
|
||
Assignee: nobody → dietrich
Comment 4•14 years ago
|
||
The gUpdates onLoad should be sufficient
Comment 5•14 years ago
|
||
btw: removing nsTryToClose.js should save around 23 ms on Firefox WinCE Tegra plus additional native code time that isn't accounted for when profiling with the patch from Bug 507012. btw: if Fuel isn't removed from the startup path then it might be a good thing to just move this code over to Fuel where the additional time during startup would be to say the least minimized.
Reporter | ||
Comment 6•14 years ago
|
||
Attachment #421909 -
Attachment is obsolete: true
Attachment #421933 -
Flags: review?(robert.bugzilla)
Comment 7•14 years ago
|
||
Comment on attachment 421933 [details] [diff] [review] v1 >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js >--- a/toolkit/mozapps/update/content/updates.js >+++ b/toolkit/mozapps/update/content/updates.js >@@ -337,6 +337,27 @@ > var startPage = this.startPage; > LOG("gUpdates", "onLoad - setting current page to startpage " + startPage.id); > gUpdates.wiz.currentPage = startPage; >+ >+ // Listen for shutdown. >+ var os = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); You need to use CoC and CiC due to the mac overlay already defining Cc and Ci Please also remove the observer when the wizard closes.
Attachment #421933 -
Flags: review?(robert.bugzilla) → review-
Reporter | ||
Comment 8•14 years ago
|
||
comments addressed
Attachment #421933 -
Attachment is obsolete: true
Attachment #421942 -
Flags: review?(robert.bugzilla)
Comment 9•14 years ago
|
||
Was this morphed into "remove nsTryToClose" or am I missing something? Any reason you're not removing the code itself from the tree?
Reporter | ||
Comment 10•14 years ago
|
||
(In reply to comment #9) > Was this morphed into "remove nsTryToClose" or am I missing something? Any > reason you're not removing the code itself from the tree? Yes, according to :mwu there's no longer a need for it in Firefox. I'm not removing the code from the tree because non-Firefox apps currently include it - Thunderbird for sure.
Comment 11•14 years ago
|
||
Thanks. Adding dev-doc-needed, since we probably should add a note about window.tryToClose not having any special meaning to the "Updating extensions for Firefox 3.x" page when it's created.
Summary: move nsTryToClose.js out of the startup path → remove nsTryToClose.js from Firefox
Comment 12•14 years ago
|
||
Dietrich, it seems to me that this can be reworked so observers aren't necessary. I'll take a look at this around Tuesday or Wednesday to see if it can be simplified a bit.
Comment 13•14 years ago
|
||
Dietrich, I think something along these lines would be cleaner
Reporter | ||
Comment 14•14 years ago
|
||
Comment on attachment 422611 [details] [diff] [review] perhaps something like this - landed in bug 543312 looks good to me, thanks!
Reporter | ||
Updated•14 years ago
|
Attachment #421942 -
Attachment is obsolete: true
Attachment #421942 -
Flags: review?(robert.bugzilla)
Reporter | ||
Comment 15•14 years ago
|
||
Comment on attachment 422611 [details] [diff] [review] perhaps something like this - landed in bug 543312 r=me
Attachment #422611 -
Flags: review+
Comment 16•14 years ago
|
||
Comment on attachment 421942 [details] [diff] [review] v2 r=me for the code changes to package-manifest.in and removed-files.in. I personally think this should just be removed from all toolkit apps if possible... can you file followup bugs for this?
Attachment #421942 -
Flags: review+
Comment 17•14 years ago
|
||
See (and possibly reopen) bug 338040 for the last attempt at killing tryToClose.
Comment 18•14 years ago
|
||
Comment on attachment 421942 [details] [diff] [review] v2 The compilation of nsTryToClose.js should probably be ifndef'd out so it isn't in dist/bin/components for Firefox. It would also be a good thing to file bugs for SeaMonkey, Thunderbird, and Sunbird to remove any dependencies on nsTryToClose.js. It would be ideal to get this file removed from toolkit so extensions, etc. don't expect it to be available in all toolkit apps as well as xulrunner.
Attachment #421942 -
Flags: review+
Comment 19•14 years ago
|
||
I filed bug 543312 for removing the dependency in app update and I'll likely get that landed this weekend.
Updated•14 years ago
|
Attachment #422611 -
Attachment description: perhaps something like this → perhaps something like this - landed in bug 543312
Attachment #422611 -
Attachment is obsolete: true
Reporter | ||
Updated•14 years ago
|
Assignee: dietrich → nobody
Assignee | ||
Comment 20•13 years ago
|
||
As bug 543312 is fixed, I think we can now get rid of nsTryToClose.js.
Attachment #553311 -
Flags: review?(dietrich)
Reporter | ||
Comment 21•13 years ago
|
||
Comment on attachment 553311 [details] [diff] [review] Remove nsTryToClose Review of attachment 553311 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Marco! The patch itself looks fine. However, before actually removing the files we need to confirm that Fennec, Thunderbird, Seamonkey and Sunbird do not have dependencies on this component. Could you either do a search through their source code, or CC the right people from those projects on this bug?
Updated•13 years ago
|
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Assignee | ||
Comment 22•13 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #21) > Thanks Marco! The patch itself looks fine. However, before actually removing > the files we need to confirm that Fennec, Thunderbird, Seamonkey and Sunbird > do not have dependencies on this component. Could you either do a search > through their source code, or CC the right people from those projects on > this bug? They are unused just as in Firefox. Do I need to send a patch similar to this to remove nsTryToClose from manifests?
Assignee | ||
Comment 23•13 years ago
|
||
It seems to be used there: https://mxr.mozilla.org/comm-central/source/suite/common/src/nsSuiteGlue.js#215
Comment 24•13 years ago
|
||
(In reply to Marco Castelluccio from comment #23) > It seems to be used there: > https://mxr.mozilla.org/comm-central/source/suite/common/src/nsSuiteGlue. > js#215 AFAIK, just copied from Firefox, but I don't have time any more to run and do SeaMonkey fixes. CCing some SeaMonkey folks who could find the time to look into this instead.
Reporter | ||
Comment 25•13 years ago
|
||
Oops. Marco can you also update /mobile/installer/removed-files.in, then ask for review from :mfinkle for the Fennec changes?
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #553311 -
Attachment is obsolete: true
Attachment #556267 -
Flags: review?(mark.finkle)
Attachment #553311 -
Flags: review?(dietrich)
Comment 27•13 years ago
|
||
Comment on attachment 556267 [details] [diff] [review] Remove nsTryToClose v2 mobile parts look fine
Attachment #556267 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 28•13 years ago
|
||
I've just filed these bugs on removing the dependencies in comm-central apps: bug 682580, bug 682581, bug 682583. I think we should be able to get these done before the next merge point. Feel free to ping me if you need a status update.
Comment 29•13 years ago
|
||
So, in the absence of nsTryToClose.js, who's going to call tryToClose()?
Comment 30•13 years ago
|
||
OK, so due to the unnatural silence around here, I'm going to have to assume that everyone's expected to write their own quit request observer.
Assignee | ||
Comment 31•13 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #30) > OK, so due to the unnatural silence around here, I'm going to have to assume > that everyone's expected to write their own quit request observer. Yes, instead of using tryToClose you have to add an observer on quit request :) The change should be trivial
Comment 32•13 years ago
|
||
Thunderbird & Lightning are now no longer using nsTryToClose.js, sorry for the delay in getting that to happen.
Assignee | ||
Comment 33•13 years ago
|
||
I think this can now be checked-in ;)
Comment 34•13 years ago
|
||
(In reply to Marco Castelluccio from comment #33) > I think this can now be checked-in ;) If you can't do that yourself, then add checkin-needed to the keywords.
Keywords: checkin-needed
Assignee | ||
Comment 36•13 years ago
|
||
Unbitrotted patch.
Attachment #556267 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 37•13 years ago
|
||
P.S.: Differently from the last patch, I've leaved components/nsTryToClose.js in browser/installer/removed-files.in, as it has to be removed during updates.
Comment 38•13 years ago
|
||
Sent to try: https://tbpl.mozilla.org/?tree=Try&rev=37490de18b94
Comment 39•13 years ago
|
||
Either this or the other changeset in the comment 38 try push turned linux M1 permaorange, so pushing each separately, to work out which: https://tbpl.mozilla.org/?tree=Try&rev=f203272d2a9d
Assignee | ||
Comment 40•13 years ago
|
||
Thank you Ed. The failure seem to be due to bug 513558.
Comment 41•13 years ago
|
||
(In reply to Marco Castelluccio from comment #40) > Thank you Ed. The failure seem to be due to bug 513558. There were other failures in there + 4 in a row was suspicious.
Comment 42•13 years ago
|
||
Ah you meant the comment 39 try run, not the original one, sorry. Yeah looks fine now apart from the random orange :-)
Comment 44•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3690cdb0bcb6
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•