736870 - convert mailnews/base/search/content/ to Services.jsm and MailServices.js

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[:aceman]

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]

Attachment: 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)]

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]

Attachment: patch v2

Thanks, fixed. Carrying over r=standard8.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/770e16975c84

Comment 5

[Philip Chee]

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]

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]

> 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]

OK, I'll try to refine this in bug 748965.