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)
Thunderbird
Search
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 18.0
People
(Reporter: aceman, Assigned: aceman)
References
Details
Attachments
(2 files, 2 obsolete files)
11.87 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
1.77 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #662685 -
Flags: review?(mkmelin+mozilla)
Comment 2•12 years ago
|
||
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.
Updated•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Keywords: checkin-needed
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 6•12 years ago
|
||
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 8•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #662685 -
Attachment is obsolete: true
Attachment #662685 -
Flags: feedback?(neil)
Attachment #668031 -
Flags: review+
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #668032 -
Flags: review?(neil)
Keywords: checkin-needed
Whiteboard: [please leave open after checkin]
Comment 12•12 years ago
|
||
(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?
Assignee | ||
Comment 13•12 years ago
|
||
Sorry, no.
Attachment #668032 -
Attachment is obsolete: true
Attachment #668032 -
Flags: review?(neil)
Attachment #668193 -
Flags: review?(neil)
Updated•12 years ago
|
Flags: in-testsuite-
Keywords: checkin-needed
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
Comment on attachment 668193 [details] [diff] [review] patch for mailnews v2 I can't actually get the patch to apply...
Comment 16•12 years ago
|
||
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+
Assignee | ||
Comment 17•12 years ago
|
||
Thanks.
Keywords: checkin-needed
Whiteboard: [please leave open after checkin] → [checkin the mailnews patch, then close the bug]
Comment 18•12 years ago
|
||
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.
Description
•