Last Comment Bug 758979 - convert mailnews/base/src/*.js to Services.jsm and MailServices.js
: convert mailnews/base/src/*.js to Services.jsm and MailServices.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 16.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 720356 720358
  Show dependency treegraph
 
Reported: 2012-05-27 11:38 PDT by :aceman
Modified: 2012-07-02 15:12 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.28 KB, patch)
2012-05-27 12:03 PDT, :aceman
no flags Details | Diff | Review
patch v2 (8.95 KB, patch)
2012-06-08 13:12 PDT, :aceman
mconley: review+
iann_bugzilla: review+
Details | Diff | Review

Description :aceman 2012-05-27 11:38:43 PDT
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?
Comment 1 Andrew Sutherland [:asuth] 2012-05-27 11:45:48 PDT
(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.
Comment 2 :aceman 2012-05-27 11:55:45 PDT
Thanks. So why not mailnews/base/util?
Comment 3 :aceman 2012-05-27 12:03:35 PDT
Created attachment 627580 [details] [diff] [review]
patch
Comment 4 Andrew Sutherland [:asuth] 2012-05-27 12:07:32 PDT
(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 5 Mark Banner (:standard8) 2012-06-05 03:46:33 PDT
Comment on attachment 627580 [details] [diff] [review]
patch

I'm busy this week, so passing review on.
Comment 6 Mike Conley (:mconley) - (Away until June 29th) 2012-06-06 10:31:49 PDT
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.
Comment 7 :aceman 2012-06-06 11:05:41 PDT
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 .
Comment 8 Mike Conley (:mconley) - (Away until June 29th) 2012-06-06 11:12:28 PDT
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.
Comment 9 :aceman 2012-06-08 13:12:12 PDT
Created attachment 631510 [details] [diff] [review]
patch v2

OK.
Comment 10 Mike Conley (:mconley) - (Away until June 29th) 2012-06-18 06:26:14 PDT
Comment on attachment 631510 [details] [diff] [review]
patch v2

Looks good to me - thanks for your work,

-Mike
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-07-02 15:12:44 PDT
https://hg.mozilla.org/comm-central/rev/4ef0d79ae8b4

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