convert mailnews/base/src/*.js to Services.jsm and MailServices.js

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Backend
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

8.95 KB, patch
mconley
: review+
Ian Neal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
msgAsyncPrompter.js:  this._threadManager = Cc["@mozilla.org/thread-manager;1"]
nsMailNewsCommandLineHandler.js:        let ioService = Cc["@mozilla.org/network/io-service;1"]
nsMailNewsCommandLineHandler.js:      let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
nsMailNewsCommandLineHandler.js:        let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
virtualFolderWrapper.js:    Cc["@mozilla.org/messenger/account-manager;1"]
virtualFolderWrapper.js:      Cc["@mozilla.org/messenger/services/filters;1"]

I wonder why these .js files are even here under /src. They should probably be in /content. Would it be OK to move them?
(In reply to :aceman from comment #0)
> I wonder why these .js files are even here under /src. They should probably
> be in /content. Would it be OK to move them?

No, they are JS modules (loaded by Components.utils.import) or JS components (loaded by XPCOM infrastructure per their manifest definitions).  If you look in Makefile.in, you will see they are listed under EXTRA_COMPONENTS or EXTRA_JS_MODULES (or PP variants).

Files in /content go there because they are scripts loaded into the global scope by 'script' tags.
(Assignee)

Comment 2

5 years ago
Thanks. So why not mailnews/base/util?
(Assignee)

Comment 3

5 years ago
Created attachment 627580 [details] [diff] [review]
patch
Attachment #627580 - Flags: review?(mbanner)
(In reply to :aceman from comment #2)
> Thanks. So why not mailnews/base/util?

Attempts at consistency, even if we lack the dependency reasons that might have motivated the C++ directory structure.  For example virtualFolderWrapper.js deals with virtual folders, and nsMsgSearchDBView.cpp and nsMsgXFVirtualFolderDBView.cpp which implement the actual virtual folder logic live in the src/ directory, so it's more appropriate than util/ which is more a home for data-types/catch-all fallback.
Comment on attachment 627580 [details] [diff] [review]
patch

I'm busy this week, so passing review on.
Attachment #627580 - Flags: review?(mbanner) → review?(mconley)
Comment on attachment 627580 [details] [diff] [review]
patch

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

::: mailnews/base/src/msgAsyncPrompter.js
@@ +84,5 @@
>  };
>  
>  function msgAsyncPrompter() {
>    this._pendingPrompts = {};
> +  this._threadManager = Services.tm;

Is there even a point of having this be aliased? We might as well just use Services.tm everywhere, instead of _threadManager.
(Assignee)

Comment 7

5 years ago
Maybe for external callers, but I couldn't find any via http://mxr.mozilla.org/comm-central/search?string=_threadManager&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central .
The underscore indicates that this should only be used internally. Anybody reaching in and attempting to manipulate something starting with an underscore are playing a dangerous game.

So I think you're safe to just replace it with Services.tm.
(Assignee)

Comment 9

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

OK.
Attachment #627580 - Attachment is obsolete: true
Attachment #627580 - Flags: review?(mconley)
Attachment #631510 - Flags: review?(mconley)
Comment on attachment 631510 [details] [diff] [review]
patch v2

Looks good to me - thanks for your work,

-Mike
Attachment #631510 - Flags: review?(mconley) → review+
(Assignee)

Updated

5 years ago
Attachment #631510 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #631510 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/4ef0d79ae8b4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
You need to log in before you can comment on or make changes to this bug.