1788159 - Simplify nsMessenger attachment methods

Status: RESOLVED FIXED

(MailNews Core :: Backend, task)

Description

[Ben Campbell]

The nsIMessenger attachment functions seem a little convoluted.

Here are my proposals on what should change:

saveAllAttachments()

• Don't prompt for filenames. Make the caller pass in destination path(s) up front
• Single messageUri rather than array? (all attachments must be from same message, right?)
• Rename to saveAttachments()
• Make sure it works with multiple attachments
• Add unit tests (there's a mochitest, but no xpcshell test)

detachAllAttachments()

• Rename to detachAttachments().
• Add a listener to trigger when complete.
• Ditch saveFirst option (make the caller do it - easy if there's listener support!)
• Single messageUri rather than array? (all attachments must be from same message, right?)
• No UI prompting (not sure if it does or not right now).
• Caller to pass in list of file urls for previously-detached attachments (to fill out X-Mozilla-External-Attachment-URL headers in the deleted attachements).
• Make sure it works with multiple attachments
• Add unit tests (there's a mochitest, but no xpcshell test)

detachAttachmentsWOPrompt()

• Ditch entirely, use detachAttachments() instead.

detachAttachment()

• Ditch entirely, use detachAttachments() instead.

saveAttachment()

• Ditch entirely, use saveAttachments() instead.

Comment 1

[Ben Campbell]

Implementation notes:

So many moving parts, so much undefined ownership, so much scope for leaks and bad error handling!

Current system involves:

• nsSaveAllAttachmentsState - a data-only (mostly) class which holds a list of items to save, and a flag to say if they should also be detached afterwards.
• nsMessenger::SaveAttachment() (the internal one, not the NS_IMETHODIMP one) - handles a single item, by creating a nsSaveMsgListener. But takes an nsSaveAllAttachmentsState object which holds all the items to be processed.
• nsSaveMsgListener - handles just a single item. But has an nsSaveAllAttachments, which it uses to kick off the next save (by calling nsMessenger::SaveAttachment() again!), and/or to kick off a Detachment phase.

nsSaveMsgListener handles destroying the nsSaveAllAttachmentsState once all the items are saved... but it's the ownership is pretty odd and there's a lot of scope for leaks and oddness and non-invoked listeners when errors occur!

Suggestion:

• merge nsSaveMsgListener and nsSaveAllAttachmentsState and nsMessenger::SaveAttachment() into one class.
• use modern C++ lambdas to replace the current callback soup (nsSaveMsgListener currently derives from nsIUrlListener, nsIMsgCopyServiceListener, nsIStreamListener, nsICancelable and it's really not clear which callbacks are used by which calls! The inheritance is all public, and exposed to the caller, where it's really just implementation detail).
• make it really bulletproof! Clean ownership of everything to avoid leaks, and clear start/stop/error handling to ensure callers don't hang upon listener notifications which never come...
• look at nsAttachmentState and see where it fits in to all this.

Comment 2

[Ben Campbell]

As per Bug 1135434, the detach-attachment-from-a-message on IMAP could also use some attention.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: Bug 1788159 - rework nsIMessenger attachment handling. r=babolivier,john.bieling

This removes the messenger.detachAttachment() method, and implements it delete/detach all as well, but it only hooks up one case of detachAttachmentsWOPrompts() yet.
Further patches will handle replacing the rest of the attachments handling in messenger.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: Bug 1788159 - Remove nsIMessenger.detachAttachmentsWOPrompts. r=babolivier

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: Bug 1788159 - Remove nsIMessenger.detachAllAttachments. r=babolivier

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: Bug 1788159 - Remove nsIMessenger.saveAllAttachments(). r=babolivier

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/11b1e55cae56
rework nsIMessenger attachment handling. r=babolivier,john.bieling
https://hg.mozilla.org/comm-central/rev/013ed3b72d72
Remove nsIMessenger.detachAttachmentsWOPrompts. r=babolivier

Comment 8

[Vineet Deo]

Because I couldn't land the whole stack due to linting errors, I have to reopen this bug.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/96b621ca6787
Remove nsIMessenger.detachAllAttachments. r=babolivier
https://hg.mozilla.org/comm-central/rev/9bf83d1c1116
Remove nsIMessenger.saveAllAttachments(). r=babolivier