Last Comment Bug 539997 - remove nsTryToClose.js from Firefox
: remove nsTryToClose.js from Firefox
Status: RESOLVED FIXED
[ts]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 10
Assigned To: Marco Castelluccio [:marco]
:
Mentors:
Depends on: 543312 682580 682581 682583
Blocks: 447581 386002
  Show dependency treegraph
 
Reported: 2010-01-15 11:21 PST by Dietrich Ayala (:dietrich)
Modified: 2011-11-06 05:39 PST (History)
13 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
the manifest bits (870 bytes, patch)
2010-01-15 13:20 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
v1 (2.39 KB, patch)
2010-01-15 14:42 PST, Dietrich Ayala (:dietrich)
robert.strong.bugs: review-
Details | Diff | Splinter Review
v2 (3.20 KB, patch)
2010-01-15 15:40 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
perhaps something like this - landed in bug 543312 (5.58 KB, patch)
2010-01-20 13:30 PST, Robert Strong [:rstrong] (use needinfo to contact me)
dietrich: review+
Details | Diff | Splinter Review
Remove nsTryToClose (7.11 KB, patch)
2011-08-15 17:16 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Remove nsTryToClose v2 (7.46 KB, patch)
2011-08-27 09:08 PDT, Marco Castelluccio [:marco]
mark.finkle: review+
Details | Diff | Splinter Review
Remove nsTryToClose v3 (6.79 KB, patch)
2011-11-02 10:52 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review

Description Dietrich Ayala (:dietrich) 2010-01-15 11:21:07 PST
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 Michael Wu [:mwu] 2010-01-15 11:36:17 PST
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.
Comment 2 Dietrich Ayala (:dietrich) 2010-01-15 13:19:06 PST
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?
Comment 3 Dietrich Ayala (:dietrich) 2010-01-15 13:20:04 PST
Created attachment 421909 [details] [diff] [review]
the manifest bits
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-15 13:54:54 PST
The gUpdates onLoad should be sufficient
Comment 5 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-15 14:29:03 PST
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.
Comment 6 Dietrich Ayala (:dietrich) 2010-01-15 14:42:26 PST
Created attachment 421933 [details] [diff] [review]
v1
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-15 14:45:34 PST
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.
Comment 8 Dietrich Ayala (:dietrich) 2010-01-15 15:40:14 PST
Created attachment 421942 [details] [diff] [review]
v2

comments addressed
Comment 9 Nickolay_Ponomarev 2010-01-16 06:44:50 PST
Was this morphed into "remove nsTryToClose" or am I missing something? Any reason you're not removing the code itself from the tree?
Comment 10 Dietrich Ayala (:dietrich) 2010-01-16 13:27:23 PST
(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 Nickolay_Ponomarev 2010-01-16 16:53:12 PST
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.
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-16 18:06:16 PST
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 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-20 13:30:58 PST
Created attachment 422611 [details] [diff] [review]
perhaps something like this - landed in bug 543312

Dietrich, I think something along these lines would be cleaner
Comment 14 Dietrich Ayala (:dietrich) 2010-01-21 13:39:59 PST
Comment on attachment 422611 [details] [diff] [review]
perhaps something like this - landed in bug 543312

looks good to me, thanks!
Comment 15 Dietrich Ayala (:dietrich) 2010-01-26 17:23:41 PST
Comment on attachment 422611 [details] [diff] [review]
perhaps something like this - landed in bug 543312

r=me
Comment 16 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-27 16:21:42 PST
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?
Comment 17 Michael Wu [:mwu] 2010-01-27 16:28:08 PST
See (and possibly reopen) bug 338040 for the last attempt at killing tryToClose.
Comment 18 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-31 03:22:18 PST
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.
Comment 19 Robert Strong [:rstrong] (use needinfo to contact me) 2010-01-31 03:23:08 PST
I filed bug 543312 for removing the dependency in app update and I'll likely get that landed this weekend.
Comment 20 Marco Castelluccio [:marco] 2011-08-15 17:16:47 PDT
Created attachment 553311 [details] [diff] [review]
Remove nsTryToClose

As bug 543312 is fixed, I think we can now get rid of nsTryToClose.js.
Comment 21 Dietrich Ayala (:dietrich) 2011-08-15 21:13:29 PDT
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?
Comment 22 Marco Castelluccio [:marco] 2011-08-25 15:49:04 PDT
(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?
Comment 23 Marco Castelluccio [:marco] 2011-08-25 15:53:08 PDT
It seems to be used there: https://mxr.mozilla.org/comm-central/source/suite/common/src/nsSuiteGlue.js#215
Comment 24 Robert Kaiser 2011-08-26 04:48:33 PDT
(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.
Comment 25 Dietrich Ayala (:dietrich) 2011-08-27 08:42:47 PDT
Oops. Marco can you also update /mobile/installer/removed-files.in, then ask for review from :mfinkle for the Fennec changes?
Comment 26 Marco Castelluccio [:marco] 2011-08-27 09:08:44 PDT
Created attachment 556267 [details] [diff] [review]
Remove nsTryToClose v2
Comment 27 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-27 09:48:12 PDT
Comment on attachment 556267 [details] [diff] [review]
Remove nsTryToClose v2

mobile parts look fine
Comment 28 Mark Banner (:standard8) 2011-08-27 10:49:29 PDT
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 neil@parkwaycc.co.uk 2011-08-27 13:39:55 PDT
So, in the absence of nsTryToClose.js, who's going to call tryToClose()?
Comment 30 neil@parkwaycc.co.uk 2011-10-19 07:38:11 PDT
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.
Comment 31 Marco Castelluccio [:marco] 2011-10-19 07:54:11 PDT
(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 Mark Banner (:standard8) 2011-10-19 08:14:27 PDT
Thunderbird & Lightning are now no longer using nsTryToClose.js, sorry for the delay in getting that to happen.
Comment 33 Marco Castelluccio [:marco] 2011-10-28 09:57:29 PDT
I think this can now be checked-in ;)
Comment 34 Mark Banner (:standard8) 2011-10-28 10:55:47 PDT
(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.
Comment 35 Dão Gottwald [:dao] 2011-10-30 07:43:27 PDT
The patch fails to apply.
Comment 36 Marco Castelluccio [:marco] 2011-11-02 10:52:02 PDT
Created attachment 571375 [details] [diff] [review]
Remove nsTryToClose v3

Unbitrotted patch.
Comment 37 Marco Castelluccio [:marco] 2011-11-02 10:56:56 PDT
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 Ed Morley [:emorley] 2011-11-03 18:32:46 PDT
Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=37490de18b94
Comment 39 Ed Morley [:emorley] 2011-11-04 14:59:30 PDT
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
Comment 40 Marco Castelluccio [:marco] 2011-11-05 01:56:34 PDT
Thank you Ed. The failure seem to be due to bug 513558.
Comment 41 Ed Morley [:emorley] 2011-11-05 03:14:21 PDT
(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 Ed Morley [:emorley] 2011-11-05 03:16:01 PDT
Ah you meant the comment 39 try run, not the original one, sorry.

Yeah looks fine now apart from the random orange :-)
Comment 44 Ed Morley [:emorley] 2011-11-06 05:39:54 PST
https://hg.mozilla.org/mozilla-central/rev/3690cdb0bcb6

Note You need to log in before you can comment on or make changes to this bug.