Closed Bug 777787 Opened 7 years ago Closed 7 years ago

Fix issues discovered in msgDBCacheManager.js while reviewing Bug 770559

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)

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;
>     }
>   },
Attachment #646811 - Flags: review?(iann_bugzilla)
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?
Attachment #646811 - Attachment is obsolete: true
Attachment #646811 - Flags: review?(iann_bugzilla)
Attachment #646954 - Flags: review?(iann_bugzilla)
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
Attachment #646954 - Flags: review?(iann_bugzilla) → review+
> 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.
(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 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.
Attachment #646954 - Flags: feedback?(mozilla)
(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.
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");
Attachment #646954 - Attachment is obsolete: true
Attachment #646954 - Flags: feedback?(mozilla)
Attachment #648177 - Flags: review+
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
Attachment #648177 - Flags: review?
Attachment #648177 - Flags: review? → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/7d005aa5bd49
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.