Last Comment Bug 740639 - Outlook import module should be rewritten with nsIWindowsRegKey
: Outlook import module 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:
Blocks: 742621
  Show dependency treegraph
 
Reported: 2012-03-29 15:54 PDT by Hiroyuki Ikezoe (:hiro)
Modified: 2012-06-15 16:05 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (36.69 KB, patch)
2012-03-29 15:54 PDT, Hiroyuki Ikezoe (:hiro)
mozilla: review-
Details | Diff | Splinter Review
Set smtp server key (4.34 KB, patch)
2012-03-29 15:56 PDT, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Splinter Review
Tests (9.37 KB, patch)
2012-03-29 15:58 PDT, Hiroyuki Ikezoe (:hiro)
mconley: review+
Details | Diff | Splinter Review
Revised test (9.45 KB, patch)
2012-04-02 19:39 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
Fix return variable of getChileName (9.42 KB, patch)
2012-04-02 23:05 PDT, Hiroyuki Ikezoe (:hiro)
mconley: review+
Details | Diff | Splinter Review
Address comment #6 (9.42 KB, patch)
2012-04-05 21:30 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
Revised patch (36.62 KB, patch)
2012-04-24 16:25 PDT, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Splinter Review
[PATCH 1/3] Rewrite Outlook importer with nsIWindowsRegKey. (33.66 KB, patch)
2012-06-04 23:46 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
[PATCH 2/3] Set smtp server key (4.34 KB, patch)
2012-06-04 23:47 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
[PATCH 3/3] Test for Outlook settings importer (9.42 KB, patch)
2012-06-04 23:48 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review
Fix external linkage builds (872 bytes, patch)
2012-06-15 12:02 PDT, neil@parkwaycc.co.uk
mozilla: review+
Details | Diff | Splinter Review

Description Hiroyuki Ikezoe (:hiro) 2012-03-29 15:54:38 PDT
Created attachment 610733 [details] [diff] [review]
Proposed fix

This is a sibling of bug 731877
Comment 1 Hiroyuki Ikezoe (:hiro) 2012-03-29 15:56:51 PDT
Created attachment 610734 [details] [diff] [review]
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) 2012-03-29 15:58:05 PDT
Created attachment 610736 [details] [diff] [review]
Tests

The result on try server:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=bf5330c256fb
Comment 3 Mike Conley (:mconley) - (needinfo me!) 2012-04-02 07:40:17 PDT
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) 2012-04-02 19:39:22 PDT
Created attachment 611697 [details] [diff] [review]
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) 2012-04-02 23:05:08 PDT
Created attachment 611725 [details] [diff] [review]
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) - (needinfo me!) 2012-04-05 13:38:22 PDT
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) 2012-04-05 21:30:51 PDT
Created attachment 612793 [details] [diff] [review]
Address comment #6

mconley, Thank you for your review!
Comment 8 David :Bienvenu 2012-04-09 09:17:10 PDT
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) 2012-04-09 16:08:22 PDT
(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 2012-04-19 12:31:38 PDT
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 2012-04-19 13:38:24 PDT
Comment on attachment 610734 [details] [diff] [review]
Set smtp server key

looks reasonable, thanks!
Comment 12 Hiroyuki Ikezoe (:hiro) 2012-04-24 16:25:05 PDT
Created attachment 618092 [details] [diff] [review]
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 2012-05-07 11:48:43 PDT
Comment on attachment 618092 [details] [diff] [review]
Revised patch

looks good, thx, sorry for the delay looking at this.
Comment 14 Mike Conley (:mconley) - (needinfo me!) 2012-06-04 11:04:40 PDT
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) 2012-06-04 23:46:51 PDT
Created attachment 630087 [details] [diff] [review]
[PATCH 1/3] Rewrite Outlook importer with nsIWindowsRegKey.
Comment 16 Hiroyuki Ikezoe (:hiro) 2012-06-04 23:47:27 PDT
Created attachment 630088 [details] [diff] [review]
[PATCH 2/3] Set smtp server key
Comment 17 Hiroyuki Ikezoe (:hiro) 2012-06-04 23:48:05 PDT
Created attachment 630089 [details] [diff] [review]
[PATCH 3/3] Test for Outlook settings importer
Comment 18 Hiroyuki Ikezoe (:hiro) 2012-06-04 23:48:41 PDT
(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 20 neil@parkwaycc.co.uk 2012-06-14 16:48:52 PDT
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 neil@parkwaycc.co.uk 2012-06-15 12:02:27 PDT
Created attachment 633618 [details] [diff] [review]
Fix external linkage builds
Comment 22 David :Bienvenu 2012-06-15 13:24:02 PDT
Note that review requests for closed bugs don't show up in my dashboard...
Comment 23 neil@parkwaycc.co.uk 2012-06-15 16:05:37 PDT
Comment on attachment 633618 [details] [diff] [review]
Fix external linkage builds

Pushed comm-central changeset c0edd4f5d0e7.

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