remove nsTryToClose.js from Firefox

RESOLVED FIXED in Firefox 10

Status

()

Firefox
General
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: dietrich, Assigned: marco)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Blocks: 386002
Whiteboard: [ts]

Comment 1

8 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

8 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

8 years ago
Created attachment 421909 [details] [diff] [review]
the manifest bits
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.
(Reporter)

Comment 6

8 years ago
Created attachment 421933 [details] [diff] [review]
v1
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-
(Reporter)

Comment 8

8 years ago
Created attachment 421942 [details] [diff] [review]
v2

comments addressed
Attachment #421933 - Attachment is obsolete: true
Attachment #421942 - Flags: review?(robert.bugzilla)

Comment 9

8 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

8 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

8 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
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.
Created attachment 422611 [details] [diff] [review]
perhaps something like this - landed in bug 543312

Dietrich, I think something along these lines would be cleaner
(Reporter)

Comment 14

8 years ago
Comment on attachment 422611 [details] [diff] [review]
perhaps something like this - landed in bug 543312

looks good to me, thanks!
(Reporter)

Updated

8 years ago
Attachment #421942 - Attachment is obsolete: true
Attachment #421942 - Flags: review?(robert.bugzilla)
(Reporter)

Comment 15

8 years ago
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+

Comment 17

8 years ago
See (and possibly reopen) bug 338040 for the last attempt at killing tryToClose.
Depends on: 543312
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
(Reporter)

Updated

7 years ago
Blocks: 447581
(Reporter)

Updated

7 years ago
Assignee: dietrich → nobody
(Assignee)

Comment 20

6 years ago
Created attachment 553311 [details] [diff] [review]
Remove nsTryToClose

As bug 543312 is fixed, I think we can now get rid of nsTryToClose.js.
Attachment #553311 - Flags: review?(dietrich)
(Reporter)

Comment 21

6 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

6 years ago
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
(Assignee)

Comment 22

6 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

6 years ago
It seems to be used there: https://mxr.mozilla.org/comm-central/source/suite/common/src/nsSuiteGlue.js#215

Comment 24

6 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

6 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

6 years ago
Created attachment 556267 [details] [diff] [review]
Remove nsTryToClose v2
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+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 31

6 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
Thunderbird & Lightning are now no longer using nsTryToClose.js, sorry for the delay in getting that to happen.
(Assignee)

Comment 33

6 years ago
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
(Assignee)

Comment 36

6 years ago
Created attachment 571375 [details] [diff] [review]
Remove nsTryToClose v3

Unbitrotted patch.
Attachment #556267 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 37

6 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.
Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=37490de18b94
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

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.