740639 - Outlook import module should be rewritten with nsIWindowsRegKey

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed fix

This is a sibling of bug 731877

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Set smtp server key

In current implementation, SMTP server key is not set to imported account, so the imported smtp server settings can not be checked in test.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Tests

The result on try server:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=bf5330c256fb

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 610736 [details] [diff] [review]
Tests

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

This looks really good!  Just a few minor nits.  With those fixed, r=me.

::: mailnews/import/test/unit/resources/mock_windows_reg_factory.js
@@ +33,5 @@
> +  },
> +
> +  getChildName: function(aIndex) {
> +    let count = 0;
> +    for (let name in this._registryData[this._keyPath]) {

I think you could simplify this function with something like:

let keys = Object.keys(this._registryData[this._keyPath]);
let keyAtIndex = keys[aIndex];
if (!keyAtIndex)
  throw Cr.NS_ERROR_FAILURE;

return this._registryData[this._keyPath][keyAtIndex];

@@ +42,5 @@
> +    throw Cr.NS_ERROR_FAILURE;
> +  },
> +
> +  _readValue: function(aName) {
> +    if (!this._registryData[this._keyPath][aName])

Can we ever be in a state where this._keyPath is not in this._registryData?  If so, this will error out.

@@ +70,5 @@
> +    return key.QueryInterface(aIid);
> +  },
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory])
> +};
> +

Remove the extra newline here.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised test

(In reply to Mike Conley (:mconley) from comment #3)
> Comment on attachment 610736 [details] [diff] [review]
> ::: mailnews/import/test/unit/resources/mock_windows_reg_factory.js
> @@ +33,5 @@
> > +  },
> > +
> > +  getChildName: function(aIndex) {
> > +    let count = 0;
> > +    for (let name in this._registryData[this._keyPath]) {
> 
> I think you could simplify this function with something like:
> 
> let keys = Object.keys(this._registryData[this._keyPath]);
> let keyAtIndex = keys[aIndex];
> if (!keyAtIndex)
>   throw Cr.NS_ERROR_FAILURE;
> 
> return this._registryData[this._keyPath][keyAtIndex];

Wow impressive! I did not know Object.keys method. 

> @@ +42,5 @@
> > +    throw Cr.NS_ERROR_FAILURE;
> > +  },
> > +
> > +  _readValue: function(aName) {
> > +    if (!this._registryData[this._keyPath][aName])
> 
> Can we ever be in a state where this._keyPath is not in this._registryData? 
> If so, this will error out.

Right. Checked this.registryData[this._keyPath] too.

> @@ +70,5 @@
> > +    return key.QueryInterface(aIid);
> > +  },
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory])
> > +};
> > +
> 
> Remove the extra newline here.

Done.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix return variable of getChileName

Oops! The return variable of getChildName should be keyAtIndex.

mconley, could you please re-review this?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 611725 [details] [diff] [review]
Fix return variable of getChileName

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

Hey Hiro,

This looks great, and passed for me locally.  Just two nits - with those fixed, r=me.

-Mike

::: mailnews/import/test/unit/resources/mock_windows_reg_factory.js
@@ +14,5 @@
> +  close: function() {
> +  },
> +
> +  openChild: function(aRelPath, aMode) {
> +    if (!this._registryData[this._keyPath][aRelPath])

You'll need the same sort of checks you used in _readValue in here, because _registryData[this._keyPath] may not exist.

@@ +26,5 @@
> +  },
> +
> +  get childCount() {
> +    let count = 0;
> +    for (let i in this._registryData[this._keyPath])

We can do that same trick we did before:

return Object.keys(this._registryData).length;

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Address comment #6

mconley, Thank you for your review!

Comment 8

[David :Bienvenu]

Comment on attachment 610733 [details] [diff] [review]
Proposed fix

this doesn't seem to apply at all...am I missing something?

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to David :Bienvenu from comment #8)
> Comment on attachment 610733 [details] [diff] [review]
> Proposed fix
> 
> this doesn't seem to apply at all...am I missing something?

The patch can be applied cleanly on my local now...

Comment 10

[David :Bienvenu]

Comment on attachment 610733 [details] [diff] [review]
Proposed fix

the for the patch, sorry it took me so long to look at this...I know the things I'm complaining about existing before your patch, but I think we should clean them up while we're changing this code.

+  if (NS_SUCCEEDED(rv)) {
+    NS_ADDREF(*aKey = key);
+    return rv;
+  }

You can use key.forget(aKey) instead for this pattern (occurs a few places in the code)

pretty sure NS_RELEASE nulls out ppAccount here, so you don't need the second line:
+          NS_RELEASE(*ppAccount);
+          *ppAccount = nsnull;

here, you can do NS_ADDREF(*ppAccount = anAccount)
+        } else {
+          *ppAccount = anAccount;
+          NS_ADDREF(anAccount);

or, make anAccount a comptr and do forget/swap, because  it looks to me like DoIMAPServer does addref ppAccount, which makes me think you might want to use a comptr for

+    nsIMsgAccount  *anAccount = nsnull;

to make the ref counting a little more robust.

I suspect the same thing is true for the pop3 server code...

clearing review request so you can clean up the pre-existing ref-counting issues.

Comment 11

[David :Bienvenu]

Comment on attachment 610734 [details] [diff] [review]
Set smtp server key

looks reasonable, thanks!

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

(In reply to David :Bienvenu from comment #10)
> 
> +  if (NS_SUCCEEDED(rv)) {
> +    NS_ADDREF(*aKey = key);
> +    return rv;
> +  }
> 
> You can use key.forget(aKey) instead for this pattern (occurs a few places
> in the code)

Done.

> pretty sure NS_RELEASE nulls out ppAccount here, so you don't need the
> second line:
> +          NS_RELEASE(*ppAccount);
> +          *ppAccount = nsnull;

Done.

> here, you can do NS_ADDREF(*ppAccount = anAccount)
> +        } else {
> +          *ppAccount = anAccount;
> +          NS_ADDREF(anAccount);

Used nsCOMPtr for nsIMsgAccount, but forget() and swap() can not be used here since anAccount is used just after this line.
So I just used NS_ADDREF(*ppAccount = ..); here.

Comment 13

[David :Bienvenu]

Comment on attachment 618092 [details] [diff] [review]
Revised patch

looks good, thx, sorry for the delay looking at this.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

These patches are definitely bitrotted. I can't apply them cleanly, and so can't check them in.

Hiro - would you be able to de-bitrot these patches?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 1/3] Rewrite Outlook importer with nsIWindowsRegKey.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 2/3] Set smtp server key

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 3/3] Test for Outlook settings importer

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mike Conley (:mconley) from comment #14)
> These patches are definitely bitrotted. I can't apply them cleanly, and so
> can't check them in.
> 
> Hiro - would you be able to de-bitrot these patches?

I've done. Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c2d74922a95a
https://hg.mozilla.org/comm-central/rev/ecb2970c809a
https://hg.mozilla.org/comm-central/rev/605de2e920d5

Comment 20

[[e-mail]]

Comment on attachment 630087 [details] [diff] [review]
[PATCH 1/3] Rewrite Outlook importer with nsIWindowsRegKey.

>+  nsCOMPtr<nsIWindowsRegKey> key =
>+    do_CreateInstance("@mozilla.org/windows-registry-key;1", &rv);
[nsComponentManagerUtils, which provides do_CreateInstance, is not automatically included in external linkage builds.]

Comment 21

[[e-mail]]

Attachment: Fix external linkage builds

Comment 22

[David :Bienvenu]

Note that review requests for closed bugs don't show up in my dashboard...

Comment 23

[[e-mail]]

Comment on attachment 633618 [details] [diff] [review]
Fix external linkage builds

Pushed comm-central changeset c0edd4f5d0e7.