Closed Bug 748965 Opened 12 years ago Closed 12 years ago

convert mail/base/content/FilterListDialog.js and SearchDialog.js to Services.jsm and MailServices.js

Categories

(Thunderbird :: Search, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(2 files, 2 obsolete files)

SearchDialog.js:      !Components.classes["@mozilla.org/network/io-service;1"]
FilterListDialog.js:      var promptSvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
FilterListDialog.js:  let prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
FilterListDialog.js:  let promptSvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
FilterListDialog.js:    var promptSvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
FilterListDialog.js:  var filterService = Components.classes["@mozilla.org/messenger/services/filters;1"].getService(Components.interfaces.nsIMsgFilterService);
FilterListDialog.js:        = Components.classes["@mozilla.org/messenger/account-manager;1"].

And also fix https://bugzilla.mozilla.org/show_bug.cgi?id=736870#c5

But wait for bug 450302 to not collide.
Attached patch patch (obsolete) — Splinter Review
Attachment #662685 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 662685 [details] [diff] [review]
patch

Review of attachment 662685 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. r=mkmelin

::: mailnews/base/search/content/searchWidgets.xml
@@ -356,5 @@
>        <method name="initWithAction">
>          <parameter name="aFilterAction"/>
>          <body>
>            <![CDATA[
> -            Components.utils.import("resource://gre/modules/Services.jsm", this);

This probably shouldn't be removed, unless seamonkey is fixed first also, and even with that, the code is more self-contained having it there.

@@ -591,5 @@
>  
>      <implementation>
>        <constructor>
>          <![CDATA[
> -            Components.utils.import("resource:///modules/mailServices.js", this);

Same thing
Attachment #662685 - Flags: review?(mkmelin+mozilla) → review+
I think Seamonkey is not affected by this because in this case it uses the same files for the filter editor (mailnews/base/search/content). But we can ask IanN.
Comment on attachment 662685 [details] [diff] [review]
patch

IanN, please see the mailnews/base/search/content/searchWidgets.xml part, if it still works in Seamonkey.
Attachment #662685 - Flags: review?(iann_bugzilla)
Comment on attachment 662685 [details] [diff] [review]
patch

But I remove the imports because I think nothing was using them, because they import into the context of 'this' and there were no references to this.Services / this.MailServices. Philip is an expert on this ;)
Attachment #662685 - Flags: feedback?(philip.chee)
Comment on attachment 662685 [details] [diff] [review]
patch

> But I remove the imports because I think nothing was using them, because they import
> into the context of 'this' and there were no references to this.Services /
> this.MailServices. Philip is an expert on this ;)

I did point this out to you in:
https://bugzilla.mozilla.org/show_bug.cgi?id=736870#c6
Where I said:
> So I think this only works because the global window scope (that contains the bound
> element) already imports Services.

Then you said:
> But if I went according to bug 722187 then I see I forgot referring to Services as
> this.Services. Maybe that is part of the problem.

So your options:
1. Nothing is broken so you could remove the imports as they aren't doing anything and depend on the global scope including Services and MailServices.
or
2. Fix the searchWidgets.xml by adding |this.| to the places you've missed.

You'll need to ask a mailnews peer which they prefer. Either way f=me for the searchWidgets.xml changes.
Attachment #662685 - Flags: feedback?(philip.chee) → feedback+
Comment on attachment 662685 [details] [diff] [review]
patch

I'd like to use the global import where there already is one.

But let's ask Neil what is the preferred way.
Attachment #662685 - Flags: review?(iann_bugzilla) → feedback?(neil)
Comment on attachment 662685 [details] [diff] [review]
patch

Eek! I think some of SeaMonkey's dialogs use Services without importing it :-(
Well, I added these useless imports in https://bug736870.bugzilla.mozilla.org/attachment.cgi?id=616691 myself. So if using Services without importing it properly still worked for Seamonkey in bug 736870 and it did not fail (only Ratty complained for semantical cleanliness), I don't think this removing if broken imports will break SM now (or TB). But when that is a problem I can just drop that part from the patch.
Attachment #662685 - Attachment is obsolete: true
Attachment #662685 - Flags: feedback?(neil)
Attachment #668031 - Flags: review+
Attached patch patch for mailnews (obsolete) — Splinter Review
Attachment #668032 - Flags: review?(neil)
Keywords: checkin-needed
Whiteboard: [please leave open after checkin]
(In reply to :aceman from comment #11)
> Created attachment 668032 [details] [diff] [review]
> patch for mailnews

This patch has both mail/ and mailnews/ code in, did you really mean to do that?
Sorry, no.
Attachment #668032 - Attachment is obsolete: true
Attachment #668032 - Flags: review?(neil)
Attachment #668193 - Flags: review?(neil)
Flags: in-testsuite-
Keywords: checkin-needed
Comment on attachment 668031 [details] [diff] [review]
[checked in] patch for TB

https://hg.mozilla.org/comm-central/rev/b2b2112e8473
Attachment #668031 - Attachment description: patch for TB [ready for checkin] → [checked in] patch for TB
Comment on attachment 668193 [details] [diff] [review]
patch for mailnews v2

I can't actually get the patch to apply...
Comment on attachment 668193 [details] [diff] [review]
patch for mailnews v2

Sorry, wrong tree.

It looks like the widgets you're changing are only used in the filter editor, so in that case that's OK.
Attachment #668193 - Flags: review?(neil) → review+
Thanks.
Keywords: checkin-needed
Whiteboard: [please leave open after checkin] → [checkin the mailnews patch, then close the bug]
Checked in: https://hg.mozilla.org/comm-central/rev/4d54f0776411
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin the mailnews patch, then close the bug]
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.