Closed
Bug 740639
Opened 12 years ago
Closed 12 years ago
Outlook import module should be rewritten with nsIWindowsRegKey
Categories
(MailNews Core :: Import, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 16.0
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 7 obsolete files)
33.66 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
4.34 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
9.42 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
872 bytes,
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
This is a sibling of bug 731877
Attachment #610733 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
The result on try server: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=bf5330c256fb
Comment 3•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
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•12 years ago
|
||
mconley, Thank you for your review!
Attachment #611725 -
Attachment is obsolete: true
Attachment #612793 -
Flags: review+
Comment 8•12 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•12 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•12 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•12 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•12 years ago
|
||
(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•12 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+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 14•12 years ago
|
||
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•12 years ago
|
||
Attachment #618092 -
Attachment is obsolete: true
Attachment #630087 -
Flags: review+
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #610734 -
Attachment is obsolete: true
Attachment #630088 -
Flags: review+
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #612793 -
Attachment is obsolete: true
Attachment #630089 -
Flags: review+
Assignee | ||
Comment 18•12 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
Comment 19•12 years ago
|
||
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
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Comment 20•12 years ago
|
||
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•12 years ago
|
||
Attachment #633618 -
Flags: review?(dbienvenu)
Comment 22•12 years ago
|
||
Note that review requests for closed bugs don't show up in my dashboard...
Updated•12 years ago
|
Attachment #633618 -
Flags: review?(dbienvenu) → review+
Comment 23•12 years ago
|
||
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.
Description
•