777787 - Fix issues discovered in msgDBCacheManager.js while reviewing Bug 770559

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Philip Chee]

Fron Bug #770559 Comment 19:

>> 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.

Fron Bug #770559 Comment 11:

> 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...]

>   observe: function dbCache_observe(aSubject, aTopic, aData) {
>     switch (aTopic) {
>     // This is observed before any windows start unloading if something other
>     // than the last 3pane window closing requested the application be
>     // shutdown. For example, when the user quits via the file menu.
>     case "quit-application-granted":

<< probably we can remove the observer here >>

>       this.stopPeriodicCheck();
>       break;
>     }
>   },

Comment 1

[Edmund Wong (:ewong)]

Attachment: Fix issues discovered in msgDBCacheManager.js (v1)

Comment 2

[Ian Neal]

Comment on attachment 646811 [details] [diff] [review]
Fix issues discovered in msgDBCacheManager.js (v1)

>   observe: function dbCache_observe(aSubject, aTopic, aData) {
>     switch (aTopic) {
>     // This is observed before any windows start unloading if something other
>     // than the last 3pane window closing requested the application be
>     // shutdown. For example, when the user quits via the file menu.
>     case "quit-application-granted":
>+      Services.obs.removeObserver(this, "quite-application-granted");
quite?

Comment 3

[Edmund Wong (:ewong)]

Attachment: Fix issues discovered in msgDBCacheManager.js while reviewing bug 770559 (v2)

Comment 4

[Ian Neal]

Comment on attachment 646954 [details] [diff] [review]
Fix issues discovered in msgDBCacheManager.js while reviewing bug 770559 (v2)

>-Cu.import("resource:///modules/IOUtils.js");
I cannot see anywhere that this is used in the file, so probably correct to remove. Bienvenu is probably the best person to check though.
> Cu.import("resource:///modules/iteratorUtils.jsm");
>-Cu.import("resource://gre/modules/XPCOMUtils.jsm");
As Services.jsm loads this, correct to remove. There seems to be a good number of other places in suite/ and mail/ that could have the same done.
> Cu.import("resource://gre/modules/Services.jsm");

r=me

Comment 5

[Philip Chee]

> As Services.jsm loads this, correct to remove. There seems to be a good number of
> other places in suite/ and mail/ that could have the same done.

Services.jsm does *not* re-export XPCOMUtils, so if you want to use XPCOMUtils you would need a separate C.u.import() statement. However grepping through msgDBCacheManager.js I see no usage of XPCOMUtils there.

Comment 6

[Ian Neal]

(In reply to Philip Chee from comment #5)
> > As Services.jsm loads this, correct to remove. There seems to be a good number of
> > other places in suite/ and mail/ that could have the same done.
> 
> Services.jsm does *not* re-export XPCOMUtils, so if you want to use
> XPCOMUtils you would need a separate C.u.import() statement. However
> grepping through msgDBCacheManager.js I see no usage of XPCOMUtils there.

Not sure why I said that following my comment about IOUtils.js, need to eat less cheese for supper perhaps.
My comment for both should have been:
"I cannot see anywhere that this is used in the file, so probably correct to remove. Bienvenu is probably the best person to check with though."

Comment 7

[Philip Chee]

Comment on attachment 646954 [details] [diff] [review]
Fix issues discovered in msgDBCacheManager.js while reviewing bug 770559 (v2)

> Bienvenu is probably the best person to check though.

Comment 8

[David :Bienvenu]

(In reply to Ian Neal from comment #4)
> Comment on attachment 646954 [details] [diff] [review]
> Fix issues discovered in msgDBCacheManager.js while reviewing bug 770559 (v2)
> 
> >-Cu.import("resource:///modules/IOUtils.js");
> I cannot see anywhere that this is used in the file, so probably correct to
> remove. Bienvenu is probably the best person to check though.

I don't see any use of IOUtils.js. I'm not sure we need iteratorUtils either, since that might have been leftover from when we returned an enumerator instead of an array from the db service.

Comment 9

[Philip Chee]

I confirm that iteratorUtils.jsm isn't used either.
So going forward we can remove these:
> Cu.import("resource:///modules/IOUtils.js");
> Cu.import("resource:///modules/iteratorUtils.jsm");
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Comment 10

[Edmund Wong (:ewong)]

Attachment: Fix issues discovered in msgDBCacheManager.js while reviewing bug 770559 (v3)

Comment 11

[Philip Chee]

Comment on attachment 648177 [details] [diff] [review]
Fix issues discovered in msgDBCacheManager.js while reviewing bug 770559 (v3)

Yes you can check this in r=me

Comment 12

[Edmund Wong (:ewong)]

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