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

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
Search
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 18.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 662685 [details] [diff] [review]
patch
Attachment #662685 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 2

5 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+
(Assignee)

Comment 3

5 years ago
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.
Keywords: checkin-needed
Keywords: checkin-needed
(Assignee)

Comment 4

5 years ago
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)
(Assignee)

Comment 5

5 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 ;)
Attachment #662685 - Flags: feedback?(philip.chee)

Comment 6

5 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+
(Assignee)

Comment 7

5 years ago
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

5 years ago
Comment on attachment 662685 [details] [diff] [review]
patch

Eek! I think some of SeaMonkey's dialogs use Services without importing it :-(
(Assignee)

Comment 9

5 years ago
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

5 years ago
Created attachment 668031 [details] [diff] [review]
[checked in] patch for TB
Attachment #662685 - Attachment is obsolete: true
Attachment #662685 - Flags: feedback?(neil)
Attachment #668031 - Flags: review+
(Assignee)

Comment 11

5 years ago
Created attachment 668032 [details] [diff] [review]
patch for mailnews
Attachment #668032 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [please leave open after checkin]

Comment 12

5 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

5 years ago
Created attachment 668193 [details] [diff] [review]
patch for mailnews v2

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+
(Assignee)

Comment 17

5 years ago
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
Last Resolved: 5 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.