Last Comment Bug 833988 - Convert nsMsgCompose::BuildMailListArray nsISupportsArray argument to something better
: Convert nsMsgCompose::BuildMailListArray nsISupportsArray argument to somethi...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 21.0
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 394167 834020 838805
  Show dependency treegraph
 
Reported: 2013-01-23 14:09 PST by :aceman
Modified: 2013-03-08 02:09 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.87 KB, patch)
2013-01-23 14:12 PST, :aceman
neil: review-
mkmelin+mozilla: feedback+
Details | Diff | Splinter Review
Proposed patch (8.38 KB, patch)
2013-01-24 04:49 PST, neil@parkwaycc.co.uk
standard8: review+
Details | Diff | Splinter Review

Description :aceman 2013-01-23 14:09:08 PST
nsresult nsMsgCompose::BuildMailListArray(nsIAbDirectory* parentDir,
                                          nsISupportsArray* array)

and

nsresult nsMsgCompose::GetMailListAddresses(nsString& name, nsISupportsArray* mailListArray, nsIMutableArray** addressesArray)
Comment 1 :aceman 2013-01-23 14:12:05 PST
Created attachment 705567 [details] [diff] [review]
patch
Comment 2 Magnus Melin 2013-01-24 03:28:29 PST
Comment on attachment 705567 [details] [diff] [review]
patch

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

Looks ok afaikt.
Comment 3 neil@parkwaycc.co.uk 2013-01-24 04:20:15 PST
Comment on attachment 705567 [details] [diff] [review]
patch

>       nsMsgMailList* mailList;
>-      rv = enumerator->CurrentItem((nsISupports**)&mailList);
>+      rv = enumerator->GetNext((nsISupports**)&mailList);
I'm sorry but I can't perpetuate this horror, the code needs a rewrite.
Comment 4 neil@parkwaycc.co.uk 2013-01-24 04:49:42 PST
Created attachment 705819 [details] [diff] [review]
Proposed patch

* Moved more of the fullname init code into the constructor
* Changed the array from an nsISupportsArray to an nsTArray
* Rewrote GetMailListAddresses to use an IndexOf comparator
Comment 5 :aceman 2013-01-24 05:20:54 PST
Quite involved rewrite, even the class changed to struct :)
Comment 6 Mark Banner (:standard8) 2013-02-07 06:31:23 PST
Comment on attachment 705819 [details] [diff] [review]
Proposed patch

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

Looks good, and the unit tests pass :-)
Comment 7 neil@parkwaycc.co.uk 2013-02-07 12:47:00 PST
Pushed comm-central changeset c52e29d0ff20.

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