770559 - Add support for closing inactive databases (folders) [SeaMonkey part for Thunderbird bug 723248]

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Philip Chee]

From Bug 767210 comment 5:
> part of the fix for bug 723248 is TB-specific, though it would be easy for
> SM to port the code.

1. Move this file
<http://hg.mozilla.org/comm-central/rev/83fd926bf1f8#l3.2>
From /mail/base/modules/ to /mailnews/base/util/
1a. Adjust makefiles to suit.
<http://hg.mozilla.org/comm-central/rev/83fd926bf1f8#l2.1>

2. Port this bit from Thunderbirds msgMail3PaneWindow.js
to SeaMonkeys msgMail3PaneWindow.js
<http://hg.mozilla.org/comm-central/rev/83fd926bf1f8#l1.1>

Comment 1

[Edmund Wong (:ewong)]

Attachment: Add support for closing inactive databases (folders) (SeaMonkey part)

Comment 2

[Ian Neal]

Comment on attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)

Looks good to me, you will need a TB review due to the moving of the file and may be worth an sr from Neil to make sure he is happy we're importing and init'ing from the correct place.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)

Redirecting the review to Standard8, since I'm am totally not a Makefile guy, and even the 3Pane change doesn't look like it would impact UI…

Comment 4

[[e-mail]]

Comment on attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)

It looks to me that if you open the mail window a second time (which is of course much easier to do in SeaMonkey because you can easily close the mail window and open it again without quitting) then the init will throw an exception because the module is already initialised. (But I haven't actually checked.)

One approach might be to move the init to somewhere that really only happens once, such as nsSuiteGlue.js (final-ui-startup, I guess).

Another approach might be to add an initialised check to the module so that you can't accidentally initialise it twice.

Comment 5

[Philip Chee]

> Another approach might be to add an initialised check to the module so that you can't
> accidentally initialise it twice.

I think bienvenu had the right idea but forgot to complete it:
http://hg.mozilla.org/comm-central/annotate/8e441f4abb45/mail/base/modules/msgDBCacheManager.js#l51

>   init: function dbcachemgr_init()
>   {

<<Insert check for this._initialized here>>

>     // we listen for "quit-application-granted" instead of
>     // "quit-application-requested" because other observers of the
>     // latter can cancel the shutdown.
>     var observerSvc = Cc["@mozilla.org/observer-service;1"]
>                       .getService(Ci.nsIObserverService);
>     observerSvc.addObserver(this, "quit-application-granted", false);
> 
>     this.startPeriodicCheck();
> 
>     this._initialized = true;
>   },

Comment 6

[Edmund Wong (:ewong)]

Attachment: Add support for closing inactive databases (folders) [SeaMonkey part] (v2)

Comment 7

[Philip Chee]

> +    if (!this._initialized) {
won't it be easier and less messy to just do:

if (this._initialized)
  return;

Comment 8

[Ian Neal]

Comment on attachment 644154 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v2)

>+++ b/mailnews/base/util/msgDBCacheManager.js
>@@ -38,17 +38,18 @@ var msgDBCacheManager =
>    */
>   init: function dbcachemgr_init()
>   {
>+    if (!this._initialized) {
As Philip said would be easier to do:
if (this._initialized)
  return;

> 
>-    // we listen for "quit-application-granted" instead of
>-    // "quit-application-requested" because other observers of the
>-    // latter can cancel the shutdown.
>-    var observerSvc = Cc["@mozilla.org/observer-service;1"]
>-                      .getService(Ci.nsIObserverService);
>-    observerSvc.addObserver(this, "quit-application-granted", false);
As we already have Services.jsm imported, is it worth changing this to:
Services.obs.addObserver(this, "quit-application-granted", false);

r=me with those fixed/addressed.

Comment 9

[Philip Chee]

> As we already have Services.jsm imported...

> Cu.import("resource:///modules/IOUtils.js");
> Cu.import("resource:///modules/iteratorUtils.jsm");
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
I suspect that we need neither IOUtils.js nor XPCOMUtils.js. But perhaps this should go into a followup bug.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Add support for closing inactive databases (folders) [SeaMonkey part] (v3)

Comment 11

[[e-mail]]

Comment on attachment 645296 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v3)

>+    Services.obs.addObserver(this, "quit-application-granted", false);
[On an unrelated note, this never gets properly removed...]

Comment 12

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a3b46de2b086

Comment 13

[Wayne Mery (:wsmwk)]

You might also need to do a seamonkey port of some of bug 1135310 which fixes flaws of bug 723248

Comment 14

[Ian Neal]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #13)
> You might also need to do a seamonkey port of some of bug 1135310 which
> fixes flaws of bug 723248

I think as this bug moved msgDBCacheManager.js from mail/ to mailnews/ we have automatically picked up the fixes from bug 1135310.

Comment 15

[Philip Chee]

(In reply to Ian Neal from comment #14)

> I think as this bug moved msgDBCacheManager.js from mail/ to mailnews/ we
> have automatically picked up the fixes from bug 1135310.
Agreed.