Closed Bug 770559 Opened 8 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philip.chee, Assigned: ewong)

References

Details

Attachments

(1 file, 2 obsolete files)

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>
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #640503 - Flags: review?(iann_bugzilla)
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.
Attachment #640503 - Flags: review?(iann_bugzilla) → review+
Attachment #640503 - Flags: review?(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…
Attachment #640503 - Flags: review?(bwinton) → review?(mbanner)
Attachment #640503 - Flags: review?(mbanner) → review+
Attachment #640503 - Flags: superreview?(neil)
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.
> 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;
>   },
Attachment #640503 - Attachment is obsolete: true
Attachment #640503 - Flags: superreview?(neil)
Attachment #644154 - Flags: review?(iann_bugzilla)
> +    if (!this._initialized) {
won't it be easier and less messy to just do:

if (this._initialized)
  return;
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.
Attachment #644154 - Flags: review?(iann_bugzilla) → review+
> 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.
Attachment #644154 - Attachment is obsolete: true
Attachment #645296 - Flags: superreview?(neil)
Attachment #645296 - Flags: review+
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...]
Attachment #645296 - Flags: superreview?(neil) → superreview+
Blocks: 777787
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a3b46de2b086
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You might also need to do a seamonkey port of some of bug 1135310 which fixes flaws of bug 723248
Flags: needinfo?(philip.chee)
Summary: Add support for closing inactive databases (folders) [SeaMonkey part] → Add support for closing inactive databases (folders) [SeaMonkey part for Thunderbird bug 723248]
(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.
(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.
Flags: needinfo?(philip.chee)
You need to log in before you can comment on or make changes to this bug.