Last Comment Bug 736870 - convert mailnews/base/search/content/ to Services.jsm and MailServices.js
: convert mailnews/base/search/content/ to Services.jsm and MailServices.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: 720356 720358 748965
  Show dependency treegraph
 
Reported: 2012-03-18 11:01 PDT by :aceman
Modified: 2012-04-25 14:20 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (13.66 KB, patch)
2012-03-29 12:58 PDT, :aceman
standard8: review+
Details | Diff | Splinter Review
patch v2 (13.78 KB, patch)
2012-04-19 12:14 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-03-18 11:01:01 PDT
CustomHeaders.js:    gPrefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
CustomHeaders.js:    var prefService = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService);
FilterEditor.js:  gPrefBranch = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService).getBranch(null);
FilterEditor.js:      let filterService = Components.classes["@mozilla.org/messenger/services/filters;1"]
FilterEditor.js:      gMailSession = Components.classes["@mozilla.org/messenger/services/session;1"]
FilterEditor.js:                        "@mozilla.org/messenger/services/filters;1"]
searchWidgets.xml:                  var consoleService = Components.classes["@mozilla.org/consoleservice;1"]
searchWidgets.xml:            var tagService = Components.classes["@mozilla.org/messenger/tagservice;1"]
searchWidgets.xml:            var accountManager = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager);
Comment 1 :aceman 2012-03-29 12:58:11 PDT
Created attachment 610654 [details] [diff] [review]
patch

I am not sure about two things:
1. there is a gMailSession variable in FilterEditor.js. I did not remove it because there is .RemoveFolderListener run when this variable is set (in SearchNewFolderOkCallback). I am not sure just replacing it with MailServices.mailSession would be right, as that may be always set (not only after SearchNewFolderOkCallback). So I at least changed it to bool indicating if that function was run. What do you think?
2. I imported the services modules into searchWidgets.xml. I am not sure I did it right. It works, but maybe some of them are superfluous. It seemed it worked also without the import of mailServices, but not without the Services one. How does this work?
Comment 2 Mark Banner (:standard8, limited time in Dec) 2012-04-19 04:29:11 PDT
Comment on attachment 610654 [details] [diff] [review]
patch

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

Sorry for the delay, r=me with the comments addressed

::: mailnews/base/search/content/FilterEditor.js
@@ +54,5 @@
>  var gFilterContext;
>  var gFilterBundle;
>  var gPreFillName;
>  var nsMsgSearchScope = Components.interfaces.nsMsgSearchScope;
> +var gMailSession = false;

Good idea changing this to a bool, but lets rename it to something like gSessionFolderListenerAdded.

@@ +539,5 @@
> +        gMailSession = true;
> +      }
> +      catch (ex)
> +      {
> +        dump("Error adding to session: " +ex + "\n");

Please change this to Components.utils.reportError, a dump isn't very useful to most people.
Comment 3 :aceman 2012-04-19 12:14:01 PDT
Created attachment 616691 [details] [diff] [review]
patch v2

Thanks, fixed. Carrying over r=standard8.
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-04-19 16:02:18 PDT
http://hg.mozilla.org/comm-central/rev/770e16975c84
Comment 5 Philip Chee 2012-04-25 07:25:55 PDT
Comment on attachment 610654 [details] [diff] [review]
patch

>        <method name="initWithAction">
[...]
> +            Components.utils.import("resource://gre/modules/Services.jsm", this);
I think this imports services into the scope of the element the XBL is bound to.
[...]
> -                  consoleService.logMessage(scriptError);
> +                  Services.console.logMessage(scriptError);
So I think this only works because the global window scope (that contains the bound element) already imports Services.

Looking at similar code the pattern seems to be:

<method name="foo">
  var tmp = {};
  Components.utils.import("resource://gre/modules/Services.jsm", tmp);
  ....
  tmp.Services.console.logMessage(...);
</method>

See for example:
http://hg.mozilla.org/mozilla-central/annotate/75c7378c87b6/browser/base/content/sync/notification.xml#l59
http://hg.mozilla.org/mozilla-central/annotate/75c7378c87b6/browser/base/content/sync/notification.xml#l69
Comment 6 :aceman 2012-04-25 08:00:22 PDT
Thanks, but I can't say I understand it any more now.
If it is needed to import it in the constructor and destructor too (in the linked example code), there is not much gain in converting the functions.
And it is also a bit different to what you wrote in bug 722187.

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.
Comment 7 Philip Chee 2012-04-25 10:32:14 PDT
> Thanks, but I can't say I understand it any more now.
> If it is needed to import it in the constructor and destructor too (in the linked
> example code), there is not much gain in converting the functions.
> And it is also a bit different to what you wrote in bug 722187.

It depends. Are you going to make extensive reference to the modules in several methods? Then import into the scope of the bound element.
> there is not much gain in converting the functions.
Modules are only compiled once. So you can import them multiple times with very little overhead.
Comment 8 :aceman 2012-04-25 14:20:15 PDT
OK, I'll try to refine this in bug 748965.

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