Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Outlook import module should be rewritten with nsIWindowsRegKey

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Import
RESOLVED FIXED
5 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

5 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

5 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

5 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

5 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

5 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

5 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.