454103 - Make nsIMsgCompFields's attachments an nsISimpleEnumerator

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Joey Minta]

Attachment: patch v1

This is me trying to force myself to pick up a bit more c++. This switches nsIMsgCompField's attachmentsArray property to be an nsISimpleEnumerator, instead of an nsISupportsArray. It also switches the internal m_attachments to be a nsCOMArray.

Comment 1

[[e-mail]]

Comment on attachment 337329 [details] [diff] [review]
patch v1

>+      var attachments = msgCompFields.attachments;
>+      var hasAttachments = false;
You could write var hasAttachments = attachments.hasMoreElements();
Or you could write
if (attachments.hasMoreElements()){ChangeAttachmentBucketVisibility(false);}

>-  return m_attachments->InsertElementAt(attachment, attachmentCount);
>+  return m_attachments.AppendObject(attachment);
InsertElementAt returns an nsresult. AppendObject returns a PRBool.

>+  for (PRInt32 i = 0; i < attachmentCount; i ++)
>+    m_attachments.RemoveObjectAt(0);
Strangely nobody bothered to use the Clear() method. Now's your big chance!

>-  nsCOMPtr<nsISupportsArray>  m_attachments;
>+  nsCOMArray<nsIMsgAttachment>  m_attachments;
Nit: don't need two spaces here.

>+    attachmentCount++;
We don't actually care.

Comment 2

[Joey Minta]

Attachment: patch v2

Updated to review comments.

Comment 3

[[e-mail]]

Comment on attachment 337341 [details] [diff] [review]
patch v2

>+      if(attachments.hasMoreElements())
Nit.

Comment 4

[Joey Minta]

Nit picked, and "patch v2" push as 362:8240c87b0dab