Closed Bug 539997 Opened 10 years ago Closed 8 years ago

remove nsTryToClose.js from Firefox

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: dietrich, Assigned: marco)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ts])

Attachments

(1 file, 6 obsolete files)

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.
Blocks: 386002
Whiteboard: [ts]
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.
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?
Attached patch the manifest bits (obsolete) — Splinter Review
Assignee: nobody → dietrich
The gUpdates onLoad should be sufficient
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.
Attached patch v1 (obsolete) — Splinter Review
Attachment #421909 - Attachment is obsolete: true
Attachment #421933 - Flags: review?(robert.bugzilla)
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-
Attached patch v2 (obsolete) — Splinter Review
comments addressed
Attachment #421933 - Attachment is obsolete: true
Attachment #421942 - Flags: review?(robert.bugzilla)
Was this morphed into "remove nsTryToClose" or am I missing something? Any reason you're not removing the code itself from the tree?
(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.
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
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.
Dietrich, I think something along these lines would be cleaner
Comment on attachment 422611 [details] [diff] [review]
perhaps something like this - landed in bug 543312

looks good to me, thanks!
Attachment #421942 - Attachment is obsolete: true
Attachment #421942 - Flags: review?(robert.bugzilla)
Comment on attachment 422611 [details] [diff] [review]
perhaps something like this - landed in bug 543312

r=me
Attachment #422611 - Flags: review+
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+
See (and possibly reopen) bug 338040 for the last attempt at killing tryToClose.
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+
I filed bug 543312 for removing the dependency in app update and I'll likely get that landed this weekend.
Attachment #422611 - Attachment description: perhaps something like this → perhaps something like this - landed in bug 543312
Attachment #422611 - Attachment is obsolete: true
Blocks: 447581
Assignee: dietrich → nobody
Attached patch Remove nsTryToClose (obsolete) — Splinter Review
As bug 543312 is fixed, I think we can now get rid of nsTryToClose.js.
Attachment #553311 - Flags: review?(dietrich)
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?
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
(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?
(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.
Oops. Marco can you also update /mobile/installer/removed-files.in, then ask for review from :mfinkle for the Fennec changes?
Attached patch Remove nsTryToClose v2 (obsolete) — Splinter Review
Attachment #553311 - Attachment is obsolete: true
Attachment #556267 - Flags: review?(mark.finkle)
Attachment #553311 - Flags: review?(dietrich)
Comment on attachment 556267 [details] [diff] [review]
Remove nsTryToClose v2

mobile parts look fine
Attachment #556267 - Flags: review?(mark.finkle) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Depends on: 682580
Depends on: 682581
Depends on: 682583
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.
So, in the absence of nsTryToClose.js, who's going to call tryToClose()?
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.
(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
Thunderbird & Lightning are now no longer using nsTryToClose.js, sorry for the delay in getting that to happen.
I think this can now be checked-in ;)
(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
The patch fails to apply.
Keywords: checkin-needed
Unbitrotted patch.
Attachment #556267 - Attachment is obsolete: true
Keywords: checkin-needed
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.
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
Thank you Ed. The failure seem to be due to bug 513558.
(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.
Ah you meant the comment 39 try run, not the original one, sorry.

Yeah looks fine now apart from the random orange :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3690cdb0bcb6
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/3690cdb0bcb6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.