1732801 - Don't pass a nsIMsgCopyServiceListener into nsIMsgFolder.copyMessages()

Status: ASSIGNED

(MailNews Core :: Backend, task)

Description

[Ben Campbell]

This from looking at nsLocalMailFolder::CopyMessages(), but I think applies to the other folder CopyMessages() too:

nsIMsgCopyService.copyMessages() takes a nsIMsgCopyServiceListener param, and passes it on to the folder's CopyMessages() fn. This is used for updating progress (via OnProgress()), and for some message key magic during file copies (SetMessageKey(), and possibly some Message ID magic).

But the nsIMsgCopyServiceListener is for outside observers of the copyservice, not for internal synchronisation. The listener passed in to the folder is not used by the folder to tell the copyservice that the copy has completed, even though that's what you'd expect from a casual reading.

Instead, when the folder has completed it's copy operation, it calls nsIMsgCopyService.NotifyCompletion() to tell the copyservice it's done.
The copyservice is responsible for sending out the OnStartCopy()/OnStopCopy() notifications to any copy listeners.

Also, it's really important to pass in an nsISupports to NotifyCompletion() rather than nsIMsgFolder, as the copyservice might not be able to match up with the original copy request (see bug 1728495 for rectifying this).

This is all quite confusing.

Recommendation: The folder-specific CopyMessages() functions should never be exposed to nsIMsgCopyServiceListener.

Simple(ish) fix:

• Dont pass the nsIMsgCopyServiceListener into the folder CopyMessages() fns.
• Instead add a nsIMsgCopyService.NotifyProgress() for the folders to handle progress updates.
• Figure out exactly what nsIMsgCopyServiceListener.SetMessageKey() and nsIMsgCopyServiceListener.GetMessageID() are for, and replace them with another mechanism.

Better fix:
use a more specific async update mechanism (a custom listener maybe), shared between copyservice and folders. It needs to handle passing back:

• progress updates ("x out of y completed")
• Copy completed (or failed)
• whatever is required to handle what SetMessageKey()/GetMessageID() currently provide (but hopefully we can do it more cleanly elsewhere).

bonus points: the copyservice could use a local listener object, and use closures to handle the callbacks. This'd keep all the code in one place and much more readable.
There are soooo many places where classes derive from listener classes to handle async things which should be hidden internally. I want to avoid adding any more of this.
(eg folders deriving from nsIUrlListener)