Remove mail's dependency on nsTryToClose.js

RESOLVED FIXED in Thunderbird 10.0

Status

Thunderbird
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: standard8, Assigned: mconley)

Tracking

Trunk
Thunderbird 10.0
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird10 fixed)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

Firefox is removing nsTryToClose.js from Firefox because it registers as app-startup and hence slows down starting up.

We should just switch these to observer registrations in appropriate places.

Someone fancy taking this on? I've said we'll do this before the next merge point, so they can land their patch before then.
Created attachment 557297 [details] [diff] [review]
Patch v1

First run at this bug.  Try results showing up here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=2edbe8f29dc6
Assignee: nobody → mconley
Status: NEW → ASSIGNED
egrep --exclude-dir=.hg -R "[t|T]ryToClose" *

from within mail and mailnews is only returning the instances in installer/removed-files.in, so I'm pretty sure this cleans out Thunderbird's usage of tryToClose.
Created attachment 557926 [details] [diff] [review]
Patch v2

My last patch was breaking some tests, since the Mock Prompt Service wasn't putting the original Prompt Service back after unregistering itself.

Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=98f8cbaeccd4
Attachment #557297 - Attachment is obsolete: true
Comment on attachment 557926 [details] [diff] [review]
Patch v2

Try builds seem to have passed muster.  Submitting for review.
Attachment #557926 - Flags: review?(mbanner)
Comment on attachment 557926 [details] [diff] [review]
Patch v2

Review of attachment 557926 [details] [diff] [review]:
-----------------------------------------------------------------

r+ as long as we don't break lightning when we land this (see comment below).

::: mail/base/content/FilterListDialog.js
@@ +124,4 @@
>          selectFolder(firstItem);
>      }
>  
> +    Components.utils.import("resource://gre/modules/Services.jsm");

Given you're using Services.jsm multiple places in this file, I think you could just import it once.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1123,4 @@
>      gSetupLdapAutocomplete = true;
>  }
>  
> +var messageComposeQuitObserver = {

I think you could rename messageComposeOfflineObserver and work this into that observer. I don't see much point in having multiple observer definitions here.

@@ +1138,5 @@
> +function AddMessageComposeQuitObserver()
> +{
> +  var observerService = Components.classes["@mozilla.org/observer-service;1"]
> +                                  .getService(Components.interfaces.nsIObserverService);
> +  observerService.addObserver(messageComposeQuitObserver, "quit-application-requested", false);

Might as well pull in Services.jsm and use Services.obs

::: mail/installer/package-manifest.in
@@ -598,5 @@
>  @BINPATH@/components/lwbrk.xpt
>  @BINPATH@/components/nsINIProcessor.js
>  @BINPATH@/components/nsINIProcessor.manifest
> -@BINPATH@/components/nsTryToClose.js
> -@BINPATH@/components/nsTryToClose.manifest

This is fine, as long as we fix bug 682583 first so as not to break calendar.

::: mail/test/mozmill/composition/test-save-changes-on-quit.js
@@ +13,5 @@
> + *
> + * The Original Code is Thunderbird Mail Client code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mike Conley <mconley@mozilla.com>

Initial developer should be the Mozilla Foundation, and add yourself to the Contributors.
Attachment #557926 - Flags: review?(mbanner) → review+
Created attachment 558600 [details] [diff] [review]
Patch v3

Thanks for the review!  I've made the corrections you've requested.
Attachment #557926 - Attachment is obsolete: true
Attachment #558600 - Flags: review?(mbanner)
tracking-thunderbird9: --- → ?
Comment on attachment 558600 [details] [diff] [review]
Patch v3

Review of attachment 558600 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8, but you'll need to fix the bitrot in test-address-book.js before landing.

::: mail/installer/removed-files.in
@@ +68,5 @@
>  #ifdef XP_MACOSX
>    components/nsSpotlightIntegration.js
>  #endif
> +components/nsTryToClose.js
> +components/nsTryToClose.manifest

These are included in omnijar, so we don't actually need to list them here (they are referenced elsewhere in this file already). Hence just drop the removed-files.in part of the diff.
Attachment #558600 - Flags: review?(mbanner) → review+
Created attachment 568054 [details] [diff] [review]
Patch v4 (based on v3, r+'d by Standard8)

Fixed bitrot, and removed lines from removes-files.in.
Attachment #558600 - Attachment is obsolete: true
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/d18a04c66ee8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status-thunderbird10: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
tracking-thunderbird9: ? → ---
While porting this I noticed that clicking "Don't Save" permanently changes the compose window's unsaved state, although I would expect that if you cancel the quit in another window then the compose window remains unsaved and prompts you again if you try to close or quit again. We filed bug 698011 on this.
You need to log in before you can comment on or make changes to this bug.