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

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Filters
--
trivial
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

13.78 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

6 years ago
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?
Attachment #610654 - Flags: review?(mbanner)
Attachment #610654 - Flags: feedback?(philip.chee)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
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.
Attachment #610654 - Flags: review?(mbanner) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 616691 [details] [diff] [review]
patch v2

Thanks, fixed. Carrying over r=standard8.
Attachment #610654 - Attachment is obsolete: true
Attachment #610654 - Flags: feedback?(philip.chee)
Attachment #616691 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/770e16975c84
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0

Comment 5

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

Comment 6

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

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

Updated

5 years ago
Blocks: 748965
(Assignee)

Comment 8

5 years ago
OK, I'll try to refine this in bug 748965.
You need to log in before you can comment on or make changes to this bug.