Last Comment Bug 748965 - convert mail/base/content/FilterListDialog.js and SearchDialog.js to Services.jsm and MailServices.js
: convert mail/base/content/FilterListDialog.js and SearchDialog.js to Services...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 450302 736870
Blocks: 720356 720358
  Show dependency treegraph
 
Reported: 2012-04-25 14:18 PDT by :aceman
Modified: 2012-10-08 11:04 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (13.44 KB, patch)
2012-09-19 14:04 PDT, :aceman
mkmelin+mozilla: review+
philip.chee: feedback+
Details | Diff | Splinter Review
[checked in] patch for TB (11.87 KB, patch)
2012-10-04 09:47 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review
patch for mailnews (13.44 KB, patch)
2012-10-04 09:47 PDT, :aceman
no flags Details | Diff | Splinter Review
patch for mailnews v2 (1.77 KB, patch)
2012-10-04 14:47 PDT, :aceman
neil: review+
Details | Diff | Splinter Review

Description :aceman 2012-04-25 14:18:22 PDT
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.
Comment 1 :aceman 2012-09-19 14:04:13 PDT
Created attachment 662685 [details] [diff] [review]
patch
Comment 2 Magnus Melin 2012-09-28 11:20:58 PDT
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
Comment 3 :aceman 2012-09-28 11:37:20 PDT
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 4 :aceman 2012-10-01 04:46:19 PDT
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.
Comment 5 :aceman 2012-10-01 05:40:45 PDT
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 ;)
Comment 6 Philip Chee 2012-10-01 09:13:03 PDT
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.
Comment 7 :aceman 2012-10-01 10:50:07 PDT
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.
Comment 8 neil@parkwaycc.co.uk 2012-10-03 17:17:56 PDT
Comment on attachment 662685 [details] [diff] [review]
patch

Eek! I think some of SeaMonkey's dialogs use Services without importing it :-(
Comment 9 :aceman 2012-10-04 00:08:35 PDT
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.
Comment 10 :aceman 2012-10-04 09:47:11 PDT
Created attachment 668031 [details] [diff] [review]
[checked in] patch for TB
Comment 11 :aceman 2012-10-04 09:47:46 PDT
Created attachment 668032 [details] [diff] [review]
patch for mailnews
Comment 12 Ian Neal 2012-10-04 14:28:08 PDT
(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?
Comment 13 :aceman 2012-10-04 14:47:47 PDT
Created attachment 668193 [details] [diff] [review]
patch for mailnews v2

Sorry, no.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-10-04 14:51:32 PDT
Comment on attachment 668031 [details] [diff] [review]
[checked in] patch for TB

https://hg.mozilla.org/comm-central/rev/b2b2112e8473
Comment 15 neil@parkwaycc.co.uk 2012-10-07 13:55:15 PDT
Comment on attachment 668193 [details] [diff] [review]
patch for mailnews v2

I can't actually get the patch to apply...
Comment 16 neil@parkwaycc.co.uk 2012-10-07 13:57:03 PDT
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.
Comment 17 :aceman 2012-10-07 14:04:54 PDT
Thanks.
Comment 18 Mark Banner (:standard8, limited time in Dec) 2012-10-08 11:04:29 PDT
Checked in: https://hg.mozilla.org/comm-central/rev/4d54f0776411

Note You need to log in before you can comment on or make changes to this bug.