Last Comment Bug 770559 - Add support for closing inactive databases (folders) [SeaMonkey part for Thunderbird bug 723248]
: Add support for closing inactive databases (folders) [SeaMonkey part for Thun...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Edmund Wong (:ewong)
:
Mentors:
Depends on: 723248
Blocks: 767210 777787
  Show dependency treegraph
 
Reported: 2012-07-03 09:08 PDT by Philip Chee
Modified: 2015-08-03 07:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add support for closing inactive databases (folders) (SeaMonkey part) (2.96 KB, patch)
2012-07-09 22:02 PDT, Edmund Wong (:ewong)
iann_bugzilla: review+
standard8: review+
Details | Diff | Splinter Review
Add support for closing inactive databases (folders) [SeaMonkey part] (v2) (2.92 KB, patch)
2012-07-19 20:25 PDT, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Splinter Review
Add support for closing inactive databases (folders) [SeaMonkey part] (v3) (3.85 KB, patch)
2012-07-24 07:31 PDT, Edmund Wong (:ewong)
ewong: review+
neil: superreview+
Details | Diff | Splinter Review

Description Philip Chee 2012-07-03 09:08:07 PDT
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) 2012-07-09 22:02:15 PDT
Created attachment 640503 [details] [diff] [review]
Add support for closing inactive databases (folders) (SeaMonkey part)
Comment 2 Ian Neal 2012-07-15 15:39:19 PDT
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) (:☕️) 2012-07-16 11:44:19 PDT
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 neil@parkwaycc.co.uk 2012-07-18 02:34:57 PDT
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 2012-07-18 05:28:40 PDT
> 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) 2012-07-19 20:25:20 PDT
Created attachment 644154 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v2)
Comment 7 Philip Chee 2012-07-20 03:23:28 PDT
> +    if (!this._initialized) {
won't it be easier and less messy to just do:

if (this._initialized)
  return;
Comment 8 Ian Neal 2012-07-22 12:31:59 PDT
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 2012-07-22 13:57:17 PDT
> 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) 2012-07-24 07:31:42 PDT
Created attachment 645296 [details] [diff] [review]
Add support for closing inactive databases (folders) [SeaMonkey part] (v3)
Comment 11 neil@parkwaycc.co.uk 2012-07-24 12:12:31 PDT
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) 2012-07-27 21:16:13 PDT
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a3b46de2b086
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2015-08-02 20:15:44 PDT
You might also need to do a seamonkey port of some of bug 1135310 which fixes flaws of bug 723248
Comment 14 Ian Neal 2015-08-03 04:37:19 PDT
(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 2015-08-03 07:11:53 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.