Last Comment Bug 731877 - Import module of Outlook Express should be rewritten with nsIWindowsRegKey
: Import module of Outlook Express should be rewritten with nsIWindowsRegKey
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: All Windows XP
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on: 770330
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-29 17:18 PST by Hiroyuki Ikezoe (:hiro)
Modified: 2012-07-02 14:03 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (61.56 KB, patch)
2012-02-29 17:21 PST, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Splinter Review
Tests (12.30 KB, patch)
2012-02-29 17:22 PST, Hiroyuki Ikezoe (:hiro)
mconley: review-
Details | Diff | Splinter Review
Revised test (12.34 KB, patch)
2012-03-05 15:09 PST, Hiroyuki Ikezoe (:hiro)
mconley: review+
Details | Diff | Splinter Review
Updated fix (62.89 KB, patch)
2012-03-26 21:52 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
Revised test (12.41 KB, patch)
2012-03-26 22:13 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
Adapt to the latest trunk (63.03 KB, patch)
2012-06-05 19:53 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
de-bitrotted (62.87 KB, patch)
2012-06-21 15:38 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review

Description Hiroyuki Ikezoe (:hiro) 2012-02-29 17:18:25 PST
We can test the module of settings import with the mock of nsIWindowsRegKey
Comment 1 Hiroyuki Ikezoe (:hiro) 2012-02-29 17:21:16 PST
Created attachment 601830 [details] [diff] [review]
Proposed fix
Comment 2 Hiroyuki Ikezoe (:hiro) 2012-02-29 17:22:13 PST
Created attachment 601831 [details] [diff] [review]
Tests

These test covers only normal case, but I suppose it is sufficient for now.
Comment 3 David :Bienvenu 2012-03-05 08:05:03 PST
Comment on attachment 601830 [details] [diff] [review]
Proposed fix

thx, this looks fine - I don't have an OE profile on my machine, but the unit test works fine.
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-03-05 08:15:40 PST
Comment on attachment 601831 [details] [diff] [review]
Tests

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

Wow, nice tests! :)

This looks really good - just a few nits - see below.

Thanks,

-Mike

::: mailnews/import/test/unit/test_oe_settings.js
@@ +1,3 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const am = Cc["@mozilla.org/messenger/account-manager;1"]

am and smtpService can be pulled from MailServices (http://mxr.mozilla.org/comm-central/source/mailnews/base/util/mailServices.js).

@@ +10,5 @@
> +let gOriginalCID = Components.manager.contractIDToCID(CONTRACT_ID);
> +let factory;
> +let uuid;
> +
> +function POP3Account() {

This can be a one-liner, like:

function POP3Account() {}

@@ +12,5 @@
> +let uuid;
> +
> +function POP3Account() {
> +}
> +POP3Account.prototype = {

Newline before this.

@@ +35,5 @@
> +  'SMTP Use Sicily': 1,
> +  'SMTP User Name': 'smtpuser'
> +};
> +
> +function IMAPAccount() {

One-liner.

@@ +37,5 @@
> +};
> +
> +function IMAPAccount() {
> +}
> +IMAPAccount.prototype = {

Newline before this.

@@ +56,5 @@
> +  'SMTP Use Sicily': 2,
> +  'SMTP User Name': 'smtpuser'
> +};
> +
> +function NNTPAccount() {

One-liner.

@@ +58,5 @@
> +};
> +
> +function NNTPAccount() {
> +}
> +NNTPAccount.prototype = {

Newline before this.

@@ +73,5 @@
> +/* Outlook Express 4.0 */
> +function OE4Registry(defaultAccount) {
> +  this._defaultAccount = defaultAccount;
> +}
> +OE4Registry.prototype = {

Newline before this.

@@ +83,5 @@
> +    'Mail': {
> +      'Poll For Mail': 1234 * 60000
> +    },
> +  },
> +  get 'Identities\\{DEFAULT_ID}\\Software\\Microsoft\\Internet Account Manager\\Accounts'() {

Wow - today I learned you can define getters using a string like this.  Neat. :)

@@ +92,5 @@
> +/* Outlook Express 5.0 */
> +function OE5Registry(defaultAccount) {
> +  this._defaultAccount = defaultAccount;
> +}
> +OE5Registry.prototype = {

Newline before this.

@@ +190,5 @@
> +
> +function MockWindowsRegKey(registryData) {
> +  this._registryData = registryData;
> +}
> +MockWindowsRegKey.prototype = {

Newline before this.

@@ +198,5 @@
> +    if (!this._registryData[aRelPath])
> +      throw Cr.NS_ERROR_FAILURE;
> +    this._keyPath = aRelPath;
> +  },
> +  close: function() {

Let's separate all of these functions, for readabilities sake.

@@ +331,5 @@
> +    let account = am.accounts.QueryElementAt(i, Ci.nsIMsgAccount);
> +    am.removeAccount(account);
> +  }
> +  let smtpServers = smtpService.smtpServers;
> +  while (smtpServers.hasMoreElements()) {

Newline before this while block.
Comment 5 Hiroyuki Ikezoe (:hiro) 2012-03-05 15:09:39 PST
Created attachment 603067 [details] [diff] [review]
Revised test

Addressed mconley's comments.
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-03-19 08:37:25 PDT
Comment on attachment 603067 [details] [diff] [review]
Revised test

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

Hey hiro - sorry it took me so long to get to this.

Just a few more tiny nits. After those are fixed, and assuming these tests actually pass, r=me.

::: mailnews/import/test/unit/test_oe_settings.js
@@ +1,1 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

I think it'd be good to have a comment at the start of this file saying what exactly we're testing for.

@@ +1,4 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource:///modules/mailServices.js");
> +
> +const registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);

why is registrar lowercase and CONTRACT_ID in upper?  I think you should choose a consistent style.  Either all caps, or prefix with k, like kRegistrar, kContractID.

@@ +339,5 @@
> +  for (let i = 0; i < accounts.Count(); i++) {
> +    let account = accounts.QueryElementAt(i, Ci.nsIMsgAccount);
> +    MailServices.accounts.removeAccount(account);
> +  }
> +  let smtpServers = MailServices.smtp.smtpServers;

I'd put a newline after this for block finishes.
Comment 7 Hiroyuki Ikezoe (:hiro) 2012-03-22 02:34:09 PDT
(In reply to Mike Conley (:mconley) from comment #6)
> Comment on attachment 603067 [details] [diff] [review]
> @@ +1,4 @@
> > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Components.utils.import("resource:///modules/mailServices.js");
> > +
> > +const registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> 
> why is registrar lowercase and CONTRACT_ID in upper?

I actually prefer uppercase name for constant variable, but in this case its appearance seems weird for me.

 REGISTRAR.registerFactory(...);

So I'd propose that 'let' is used for the registrar like this:

let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);

What do you think?
Comment 8 Hiroyuki Ikezoe (:hiro) 2012-03-26 21:52:34 PDT
Created attachment 609609 [details] [diff] [review]
Updated fix

Adapted to the latest trunk.
Comment 9 Hiroyuki Ikezoe (:hiro) 2012-03-26 22:13:20 PDT
Created attachment 609612 [details] [diff] [review]
Revised test

This patch is addressed mconley's comment.

And fixed the failure of smtp server port check on try server because the try server is not applied KB933612. See http://mxr.mozilla.org/comm-central/source/mailnews/import/oexpress/nsOESettings.cpp#873
So I've changed the expected smtp server port 465 to avoid the failure.

And also fix the run-if directive in xpcshell.ini.

I pushed these fixes to try server. The fixes does not contain the correct run-if directive and registrar name is lower-case, but I suppose it is OK.
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=0a0cc63aeb50
Comment 10 Hiroyuki Ikezoe (:hiro) 2012-03-29 15:23:22 PDT
Comment on attachment 609609 [details] [diff] [review]
Updated fix

Oops! I did make this obsolete..
Comment 11 Hiroyuki Ikezoe (:hiro) 2012-06-05 19:53:03 PDT
Created attachment 630419 [details] [diff] [review]
Adapt to the latest trunk
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-06-09 07:09:15 PDT
This has bitrotted again. Please rebase and I'll get it checked in.
Comment 13 Ludovic Hirlimann [:Usul] 2012-06-18 05:31:51 PDT
Hiro ?
Comment 14 Hiroyuki Ikezoe (:hiro) 2012-06-21 15:38:34 PDT
Created attachment 635511 [details] [diff] [review]
de-bitrotted

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