Outlook import module should be rewritten with nsIWindowsRegKey

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Import
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 16.0
All
Windows XP
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 610733 [details] [diff] [review]
Proposed fix

This is a sibling of bug 731877
Attachment #610733 - Flags: review?(dbienvenu)
(Assignee)

Comment 1

6 years ago
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.
Attachment #610734 - Flags: review?(dbienvenu)
(Assignee)

Comment 2

6 years ago
Created attachment 610736 [details] [diff] [review]
Tests

The result on try server:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=bf5330c256fb
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attachment #610736 - Flags: review?(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.
Attachment #610736 - Flags: review?(mconley) → review+
(Assignee)

Comment 4

6 years ago
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.
Attachment #610736 - Attachment is obsolete: true
Attachment #611697 - Flags: review+
(Assignee)

Comment 5

6 years ago
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?
Attachment #611697 - Attachment is obsolete: true
Attachment #611725 - Flags: review?(mconley)
(Assignee)

Updated

6 years ago
Blocks: 742621
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;
Attachment #611725 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 612793 [details] [diff] [review]
Address comment #6

mconley, Thank you for your review!
Attachment #611725 - Attachment is obsolete: true
Attachment #612793 - Flags: review+

Comment 8

5 years ago
Comment on attachment 610733 [details] [diff] [review]
Proposed fix

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

Comment 9

5 years ago
(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

5 years ago
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.
Attachment #610733 - Flags: review?(dbienvenu) → review-

Comment 11

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

looks reasonable, thanks!
Attachment #610734 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 12

5 years ago
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.
Attachment #610733 - Attachment is obsolete: true
Attachment #618092 - Flags: review?(dbienvenu)

Comment 13

5 years ago
Comment on attachment 618092 [details] [diff] [review]
Revised patch

looks good, thx, sorry for the delay looking at this.
Attachment #618092 - Flags: review?(dbienvenu) → review+
Keywords: checkin-needed
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?
Keywords: checkin-needed
(Assignee)

Comment 15

5 years ago
Created attachment 630087 [details] [diff] [review]
[PATCH 1/3] Rewrite Outlook importer with nsIWindowsRegKey.
Attachment #618092 - Attachment is obsolete: true
Attachment #630087 - Flags: review+
(Assignee)

Comment 16

5 years ago
Created attachment 630088 [details] [diff] [review]
[PATCH 2/3] Set smtp server key
Attachment #610734 - Attachment is obsolete: true
Attachment #630088 - Flags: review+
(Assignee)

Comment 17

5 years ago
Created attachment 630089 [details] [diff] [review]
[PATCH 3/3] Test for Outlook settings importer
Attachment #612793 - Attachment is obsolete: true
Attachment #630089 - Flags: review+
(Assignee)

Comment 18

5 years ago
(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!
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c2d74922a95a
https://hg.mozilla.org/comm-central/rev/ecb2970c809a
https://hg.mozilla.org/comm-central/rev/605de2e920d5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
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.]
Created attachment 633618 [details] [diff] [review]
Fix external linkage builds
Attachment #633618 - Flags: review?(dbienvenu)

Comment 22

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

Updated

5 years ago
Attachment #633618 - Flags: review?(dbienvenu) → review+
Comment on attachment 633618 [details] [diff] [review]
Fix external linkage builds

Pushed comm-central changeset c0edd4f5d0e7.
You need to log in before you can comment on or make changes to this bug.