Closed Bug 731877 Opened 8 years ago Closed 8 years ago

Import module of Outlook Express should be rewritten with nsIWindowsRegKey

Categories

(MailNews Core :: Import, defect)

All
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 16.0

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files, 5 obsolete files)

We can test the module of settings import with the mock of nsIWindowsRegKey
Attached patch Proposed fix (obsolete) — Splinter Review
Attachment #601830 - Flags: review?(dbienvenu)
Attached patch Tests (obsolete) — Splinter Review
These test covers only normal case, but I suppose it is sufficient for now.
Attachment #601831 - Flags: review?(mconley)
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
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.
Attachment #601830 - Flags: review?(dbienvenu) → review+
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.
Attachment #601831 - Flags: review?(mconley) → review-
Attached patch Revised test (obsolete) — Splinter Review
Addressed mconley's comments.
Attachment #601831 - Attachment is obsolete: true
Attachment #603067 - Flags: review?(mconley)
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.
Attachment #603067 - Flags: review?(mconley) → review+
(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?
Attached patch Updated fix (obsolete) — Splinter Review
Adapted to the latest trunk.
Attachment #601830 - Attachment is obsolete: true
Attachment #609609 - Flags: review+
Attached patch Revised testSplinter Review
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
Attachment #603067 - Attachment is obsolete: true
Attachment #609609 - Attachment is obsolete: true
Attachment #609612 - Flags: review+
Comment on attachment 609609 [details] [diff] [review]
Updated fix

Oops! I did make this obsolete..
Attachment #609609 - Attachment is obsolete: false
Attached patch Adapt to the latest trunk (obsolete) — Splinter Review
Attachment #609609 - Attachment is obsolete: true
Attachment #630419 - Flags: review+
Keywords: checkin-needed
This has bitrotted again. Please rebase and I'll get it checked in.
Keywords: checkin-needed
Hiro ?
Attached patch de-bitrottedSplinter Review
Attachment #630419 - Attachment is obsolete: true
Attachment #635511 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/73bc66a4817e
https://hg.mozilla.org/comm-central/rev/9f8c975b8d30
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Depends on: 770330
You need to log in before you can comment on or make changes to this bug.