1612246 - Replace idl nsIArray usage with Array<T> in nsIMsgSend

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

mailnews/compose/public/nsIMsgSend.idl

Comment 1

[Magnus Melin [:mkmelin]]

Could be worth doing this soon to make it easier for bug 1211292.

Comment 2

[Ben Campbell]

I'm doing this now, but I'm a little confused about the aAttachments param of nsIMsgSend.createRFC822Message(). The comments say:

@param aAttachments Array of nsIMsgAttachmentData

But it seems to be used in code as already-loaded attachments: an array of nsIMsgAttachedFile objects.
Know which one it should be?

There's also the aEmbeddedObjects param which can contain (I think) either mozilla::dom::Element or nsIMsgEmbeddedImageData objects (I think the latter is for Outlook import support...).
I was going to go with:

in Array<nsISupports> aEmbeddedObjects

And keep the existing runtime QIs to distinguish each object. But I'm open to better suggestions :-)

Comment 3

[Magnus Melin [:mkmelin]]

(In reply to Ben Campbell from comment #2)

But it seems to be used in code as already-loaded attachments: an array of nsIMsgAttachedFile objects.
Know which one it should be?

I'd trust the code and not the comment.

There's also the aEmbeddedObjects param which can contain (I think) either mozilla::dom::Element or nsIMsgEmbeddedImageData objects (I think the latter is for Outlook import support...).

Maybe it's always Array<nsIMsgEmbeddedImageData>?
If not, it would be nicer if we can figure out things or split it of so it's not Array<nsISupports>. It's rather an ugly API so maybe we want to change it not to force that strange parameter on everyone...

Comment 4

[Ben Campbell]

Attachment: 1612246-nsIArray-removal-in-nsIMsgSend-and-nsIImportService-1.patch

A little more intrusive than I'd like, but here it is.

(In reply to Magnus Melin [:mkmelin] from comment #3)

There's also the aEmbeddedObjects param which can contain (I think) either mozilla::dom::Element or nsIMsgEmbeddedImageData objects (I think the latter is for Outlook import support...).

Maybe it's always Array<nsIMsgEmbeddedImageData>?

No, there are definitely code paths which shove Elements in there. I've left it as Array<nsISupports> for now - so should behave just like the previous version. I Don't know enough about what it's actually trying to do to have a good take on improvements.
But yeah, some API refinement is probably in order.

Try run here (have to head out this afternoon, so won't be around to see it finish - keep an eye on the windows build, as there's some win32-only MAPI code I haven't yet compiled locally):

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=88aae627daab3142778b5c8fba19b35319c1166c

Comment 5

[Ben Campbell]

Attachment: 1612246-nsIArray-removal-in-nsIMsgSend-and-nsIImportService-2.patch

Doh. Minor windows-specific fix
and try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=82fd8eecbd79716336bf34abbcdcea34b332eac7

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9176457 [details] [diff] [review]
1612246-nsIArray-removal-in-nsIMsgSend-and-nsIImportService-2.patch

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +1186,4 @@
>        rv = mMsgSend->CreateAndSendMessage(
>            m_composeHTML ? m_editor.get() : nullptr, identity, accountKey,
>            m_compFields, false, false, (nsMsgDeliverMode)deliverMode, nullptr,
> +          m_composeHTML ? TEXT_HTML : TEXT_PLAIN, bodyString, {}, {}, m_window,

can't these continue to be nullptr instead of empty array?

::: mailnews/compose/src/nsMsgSend.cpp
@@ +1262,5 @@
> +    // remove the others from the list
> +    RefPtr<nsMsgAttachmentData> attachment(new nsMsgAttachmentData);
> +
> +    int32_t i;
> +    for (i = count - 1, count = 0; i >= 0; i--) {

would put the i declaration inside the for

@@ +1265,5 @@
> +    int32_t i;
> +    for (i = count - 1, count = 0; i >= 0; i--) {
> +      // Reset this structure to null!
> +
> +      // now we need to get the element in the array and do the magic

Nit: Now

@@ +1267,5 @@
> +      // Reset this structure to null!
> +
> +      // now we need to get the element in the array and do the magic
> +      // to process this element.
> +      //

Nit: remove this empty commented out line

@@ +1978,3 @@
>      m_pre_snarfed_attachments_p = true;
>  
>      for (i = mCompFieldLocalAttachments; i < mPreloadedAttachmentCount; i++) {

Please declare uint32_t i inside the loops when not used for external purposes

Comment 7

[Ben Campbell]

Attachment: 1612246-nsIArray-removal-in-nsIMsgSend-and-nsIImportService-3.patch

nit fixes and get-it-building-on-windows fix.

And a not-quite-finished-but-looking-much-better try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4c9c69efe0c18ad3722a3a7b3dbcc2f6026bb0e9

Comment 8

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #6)

Comment on attachment 9176457 [details] [diff] [review]
::: mailnews/compose/src/nsMsgCompose.cpp
@@ +1186,4 @@

   rv = mMsgSend->CreateAndSendMessage(
       m_composeHTML ? m_editor.get() : nullptr, identity, accountKey,
       m_compFields, false, false, (nsMsgDeliverMode)deliverMode, nullptr,

•      m_composeHTML ? TEXT_HTML : TEXT_PLAIN, bodyString, {}, {}, m_window,
  

can't these continue to be nullptr instead of empty array?

No - those function params are references now rather than pointers (ie nsTArray<>&). I think that's good and reduces the ambiguity I've seen in a few places (ie "is a null array treated the same as an empty one, or does it signify something special?")

Comment 9

[Ben Campbell]

Holding off on landing this - it seems to break one of the mochitests. Something to do with pasting html with embedded images...

./mach mochitest --headless --log-mach - comm/mail/test/browser/composition/browser_blockedContent.js

Comment 10

[Ben Campbell]

(In reply to Ben Campbell from comment #9)

Holding off on landing this - it seems to break one of the mochitests. Something to do with pasting html with embedded images...

Haven't forgotten this - just having lots of trouble running the mochitests locally at the moment... (think it might be debug build running too slow and causing timeouts, so will try on a release build. byebye debugger :- )

Comment 11

[Ben Campbell]

[edit - remove doublepost]

Comment 12

[Ben Campbell]

Attachment: 1612246-part-one-remove-dead-code-1.patch

Just removing a little leftover dead code. Simplify the nsIArray-removal slightly.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9186103 [details] [diff] [review]
1612246-part-one-remove-dead-code-1.patch

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

LGTM, r=mkmelin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4829ad0bcb0b
Remove dead code in nsMsgComposeAndSend (left over from changes in Bug 1211292). r=mkmelin

Comment 15

[Ben Campbell]

Attachment: 1612246-part-two-nsIArray-removal-nsIMsgSend.createRFC822Message-attachments-1.patch

Taking very small steps with this one - I had a lot of trouble minor mochitest breakages last time I tried.

Try run for this one - there's one mochitest orange, but I'm not sure if it's the fault of this patch or not (it's marked as intermittent fail). I could do with a second opinion:

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=b23a6526144bbff29b3dc050e2534d80b6ad5b9f

Disregard the r? if you think the fail is due to this patch!

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9186541 [details] [diff] [review]
1612246-part-two-nsIArray-removal-nsIMsgSend.createRFC822Message-attachments-1.patch

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

::: mailnews/compose/src/nsMsgSend.cpp
@@ +1972,5 @@
>      // We merely need to point the internal attachment data at those tmp
>      // files.
>      m_pre_snarfed_attachments_p = true;
>  
>      for (i = mCompFieldLocalAttachments; i < mPreloadedAttachmentCount; i++) {

Would be good to move the i declaration into the for for cases like this. But we can do it in later patches.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ad4a22798876
Partial nsIArray removal in nsIMsgSend/nsIImportService createRFC822Message().

Comment 18

[Ben Campbell]

Attachment: 1612246-part-three-nsIMsgSend.createRFC822Message-embedded-1.patch

This one wraps up the remaining nsIArray use in createRFC822Message (both in nsIMsgSend and nsIImportService). There's still some internal nsIArray use which could be cleaned up (the nsMsgComposeAndSend initialisation is a bit tangled), but this fixes up the interfaces at least.

try run here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=814aafed6ad863ea9863db41948d4f0fe08380a4

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9190448 [details] [diff] [review]
1612246-part-three-nsIMsgSend.createRFC822Message-embedded-1.patch

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

LGTM, I wouldn't worry about nsMsgComposeAndSend. We'll just remove the nsMsgSend.cpp sooner or later anyway.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/520c2bc1321e
Remove nsIArray use from nsIMsgSend and nsIImportService. r=mkmelin

Comment 21

[Magnus Melin [:mkmelin]]

Not strictly this bug, but can we now just move createRFC822Message to nsIImportService, since it's only import that's using it?

Comment 22

[Magnus Melin [:mkmelin]]

Oh, see bug 1680189.