682581 - Remove mail's dependency on nsTryToClose.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

First run at this bug.  Try results showing up here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=2edbe8f29dc6

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 557926 [details] [diff] [review]
Patch v2

Try builds seem to have passed muster.  Submitting for review.

Comment 5

[Mark Banner (:standard8)]

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 <[e-mail]>

Initial developer should be the Mozilla Foundation, and add yourself to the Contributors.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

Thanks for the review!  I've made the corrections you've requested.

Comment 7

[Mark Banner (:standard8)]

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.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4 (based on v3, r+'d by Standard8)

Fixed bitrot, and removed lines from removes-files.in.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/d18a04c66ee8

Comment 10

[[e-mail]]

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.