Not all the references to nsISupportsArray in the files touched were removed. The initial goal was to make nsMsgFolderNotificationService accept nsIArrays instead of nsISupportsArrays. Whatever changes were necessary to implement this have been made.

Comment on attachment 322171 [details] [diff] [review] first stab per comments on irc.

Comment on attachment 322171 [details] [diff] [review] first stab >+#include "nsIArray.idl" Nit: interface nsIArray; >+#include "MailNewsTypes2.idl" Nit: unnecessary? > // for the convenience of the callers, we allow this to be an nsISupportsArray of items, >- // or an individual nsISupports. >+ // an individual nsISupports, or an array of message keys. This change is wrong, because the previous line already mentions an array. Perhaps all this needs is s/nsISupportsArray/nsIArray/ ? > nsTArray<nsMsgKey> m_searchHits; >- nsCOMPtr <nsISupportsArray> m_searchHitHdrs; >+ nsCOMPtr <nsIMutableArray> m_searchHitHdrs; Nit: the lines used to line up. ^ here >+ nsCOMPtr<nsIMutableArray> messageArray; >+ nsresult rv; >+ messageArray = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv); Nit: swap these around: nsresult rv; nsCOMPtr<nsIMutableArray> messageArray(do_CreateInstance(NS_ARRAY_CONTRACTID, &rv)); NS_ENSURE_SUCCESS(rv, rv); >+ m_msgFolder = do_QueryInterface(srcFolder); Nit: actually a simple assignment suffices here. > nsCOMPtr<nsISupports> supports(do_QueryInterface(aMsg)); > if(supports) >- m_messageArray->AppendElement(supports); >+ m_messageArray->AppendElement(supports, PR_FALSE); Nit: is the QI needed here? >+ nsCOMPtr <nsIMutableArray> mutableArray = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv); Nit: IMHO this name sounds silly; use array or folderArray. >+ nsCOMPtr<nsIMsgDBHdr> currMsg = do_QueryElementAt(messages, i); >+ msgArray.AppendObject(currMsg); [Sadly msgArray.AppendElement(currMsg.forget()); only works with nsTArray<nsCOMPtr>] >- msgArray->Clear(); >+ msgArray.Clear(); Nit: probably unnecessary. >+ support = do_QueryElementAt(folders, 0); [Sadly nsIArray has no GetElementAt method.] > arguments->RemoveElementAt(0); > itemCount--; > >- nsCOMPtr<nsISupportsArray> messageArray; >- NS_NewISupportsArray(getter_AddRefs(messageArray)); Strange, why does this code mutate the array, then copy it? I think I would prefer it if it was passed in an nsIArray, and then you copied all but the first element to the new array. >+ nsCOMPtr <nsISupports> supports = do_QueryElementAt(itemArray, i, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); > if (supports) Nit: you don't need both tests. > nsCOMPtr<nsISupports> msgSupport = do_QueryInterface(mMessage); >- msgArray->InsertElementAt(msgSupport, 0); >+ msgArray->AppendElement(msgSupport, PR_FALSE); Nit: is the QI needed here? I'd just append mMessage. > nsCOMPtr<nsISupports> folderSupport = do_QueryInterface(which, &rv); > NS_ENSURE_SUCCESS(rv, rv); >- folders->AppendElement(folderSupport); >+ folders->AppendElement(folderSupport, PR_FALSE); Nit: and another! > nsImapMailFolder::CopyMessages(nsIMsgFolder* srcFolder, >- nsISupportsArray* messages, >+ nsIMutableArray* messages, Nit: I don't see this array being mutated. > nsImapMailFolder::InitCopyState(nsISupports* srcSupport, >- nsISupportsArray* messages, >+ nsIMutableArray* messages, Nit: Or this one. Maybe the mutation isn't within the patch context? >- if (message ) >+ if (message) Nit: unrelated whitespace cleanup confuses blame. >- nsCOMPtr <nsISupportsArray> messages = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv); >+ nsCOMPtr <nsIMutableArray> messages = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv); > if (messages && NS_SUCCEEDED(rv)) > { >- NS_NewISupportsArray(getter_AddRefs(messages)); Oh wow! Don't let timeless see this ;-) > nsCOMPtr<nsISupports> iSupports = do_QueryInterface(msgHdr); >- messages->AppendElement(iSupports); >+ messages->AppendElement(iSupports, PR_FALSE); Nit: back to the useless QI again? nsCOMPtr<nsISupports> iSupports = do_QueryInterface(msgHdr); - messages->AppendElement(iSupports); + messages->AppendElement(iSupports, PR_FALSE); Nit: and another?

Heh, I didn't change what was already there too much. ;) I'll fix all of this up. >> nsImapMailFolder::CopyMessages(nsIMsgFolder* srcFolder, >>- nsISupportsArray* messages, >>+ nsIMutableArray* messages, >Nit: I don't see this array being mutated. > >> nsImapMailFolder::InitCopyState(nsISupports* srcSupport, >>- nsISupportsArray* messages, >>+ nsIMutableArray* messages, >Nit: Or this one. Maybe the mutation isn't within the patch context? IIRC those two arrays had to be mutable, there is a mutation 3-4 levels down the call stack.

Fixed some JS stuff as well. A known issue with this patch is that dragging and dropping messages on to a folder doesn't work. This is because of a call somewhere in the stack to RDF, which only knows about nsISupportsArray. A checkin of the patch to bug 421443, plus some tweaks to messenderdnd.js, should fix this. I've tried to fix some of the SeaMonkey JS stuff as well, though I haven't tested it. Unfortunately mailnews/base/resources/content/mailCommands.js still uses RDF, so I couldn't fix it too much. The Thunderbird version, mail/base/content/mailCommands.js, doesn't have RDF, so it could be fixed.

There's one small thing I missed, line 512 in mail/base/content/mailWindowOverlay.js, messages.clear() should be with a small 'c'. It'll be fixed "in the next version" ;)

Comment on attachment 322296 [details] [diff] [review] fixed a few style nits that weren't fixed earlier Is there a bug for fixing SeaMonkey's mailCommands.js not to use RDF?

None tracked by the killrdf bug 420506, at least. Should I file a bug for it?

Comment on attachment 322550 [details] [diff] [review] more stuff fixed, SM RDF usage removed I'm asking for r/sr again as I've added a bit more stuff that wasn't there earlier, killing RDF usage in SM mailCommands.js. Possible regressions include those involving operations on messages and folders in the front end. The problems will most probably be in the front end.

Eh, I meant "The problems will most probably be in JS."

There was just one function that mutated the array, nsMsgLocalMailFolder::SortMessagesBasedOnKey(), and it didn't *need* to mutate the array either.

Comment on attachment 322612 [details] [diff] [review] nsIMutableArray removed from nsIMsgFolder::CopyMessages >+ messages = sortedMsgs; This doesn't work because messages is an in parameter. I think the best move here is to move the sorting into CopyMessages just before the call to InitCopyState (for which you would create a new array). You could always check for more than one message before sorting to avoid extra work.

Comment on attachment 322664 [details] [diff] [review] stupid, stupid, stupid Both r+sr=me for the suite-specific portions only; you should get one additional backend review.

Comment on attachment 322664 [details] [diff] [review] stupid, stupid, stupid Thanks Neil. (sorry for asking for another r, bienvenu, but I guess you'd be most familiar with the code)

Comment on attachment 322664 [details] [diff] [review] stupid, stupid, stupid nvm, Mark has consented :)

Comment on attachment 322664 [details] [diff] [review] stupid, stupid, stupid This patch has mail/ components as well (which I can't review), unfortunately I didn't realise that at the time of accepting, so pushing back to David.

Comment on attachment 322664 [details] [diff] [review] stupid, stupid, stupid Thx for doing this! r=bienvenu for mail parts. A couple nits I noticed in other parts of the patch: + var folder = RDF.GetResource(uri); + folder = folder.QueryInterface(Components.interfaces.nsIMsgFolder); these two lines can be combined (if you want...) - folder.propagateDelete(children.GetElementAt(i), true, msgWindow); - - children.Clear(); + folder.propagateDelete(iter.getNext(), true, msgWindow); the indentation of the folder.propagateDelete line looks odd - it could just be the diff, or it could be a tab, or just too many spaces - could you double check?

Comment on attachment 322664 [details] [diff] [review] stupid, stupid, stupid + [noscript] void markMsgsOnPop3Server(in nsIArray aMessages, in PRInt32 aMark); I can't think of any reason this needs to be [noscript]. This looks wrong - you should be creating NS_ARRAY_CONTRACTID for partialMsgs: + nsCOMPtr<nsIMutableArray> partialMsgs; // Delete the partial headers. They're useless now // that the server copy is being deleted. for (PRUint32 msgIndex = 0; msgIndex < m_searchHits.Length(); msgIndex++) { nsCOMPtr <nsIMsgDBHdr> msgHdr; m_searchHitHdrs->QueryElementAt(msgIndex, NS_GET_IID(nsIMsgDBHdr), getter_AddRefs(msgHdr)); if (msgHdr) { PRUint32 flags; msgHdr->GetFlags(&flags); if (flags & MSG_FLAG_PARTIAL) { if (!partialMsgs) partialMsgs = do_CreateInstance(NS_SUPPORTSARRAY_CONTRACTID, &rv); NS_ENSURE_SUCCESS(rv, rv); r=bienvenu for the mailnews core code, with those problems addressed. These changes look pretty straightforward, but they touch a lot of code. We might want to coordinate with a QA test day effort once these changes are landed.

Thanks, Neil and David, for your time! (carrying forward r=bienvenu+sr=neil for non-suite specific code, r+sr=neil for suite specific code) Addressed all the remaining comments. hg patch, so patch -p1 please. Again, possible regressions include anything to do with messages or folders in the frontend -- both Thunderbird and SeaMonkey. The changes to SeaMonkey JS are similar to the changes in Tb JS, but I've tested only Tb.

I've just checked this in, this was slightly bitrotted by my patch earlier today, but as the changes were simple include/uuid fixes, I fixed it up for you. Checking in mailnews/base/public/nsIMsgCopyService.idl; /cvsroot/mozilla/mailnews/base/public/nsIMsgCopyService.idl,v <-- nsIMsgCopyService.idl new revision: 1.21; previous revision: 1.20 done Checking in mailnews/base/public/nsIMsgFolder.idl; /cvsroot/mozilla/mailnews/base/public/nsIMsgFolder.idl,v <-- nsIMsgFolder.idl new revision: 1.197; previous revision: 1.196 done Checking in mailnews/base/public/nsIMsgFolderListener.idl; /cvsroot/mozilla/mailnews/base/public/nsIMsgFolderListener.idl,v <-- nsIMsgFolderListener.idl new revision: 1.3; previous revision: 1.2 done Checking in mailnews/base/public/nsIMsgFolderNotificationService.idl; /cvsroot/mozilla/mailnews/base/public/nsIMsgFolderNotificationService.idl,v <-- nsIMsgFolderNotificationService.idl new revision: 1.4; previous revision: 1.3 done Checking in mailnews/base/resources/content/mail3PaneWindowCommands.js; /cvsroot/mozilla/mailnews/base/resources/content/mail3PaneWindowCommands.js,v <-- mail3PaneWindowCommands.js new revision: 1.160; previous revision: 1.159 done Checking in mailnews/base/resources/content/mailCommands.js; /cvsroot/mozilla/mailnews/base/resources/content/mailCommands.js,v <-- mailCommands.js new revision: 1.113; previous revision: 1.112 done Checking in mailnews/base/resources/content/mailWindowOverlay.js; /cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v <-- mailWindowOverlay.js new revision: 1.268; previous revision: 1.267 done Checking in mailnews/base/resources/content/markByDate.js; /cvsroot/mozilla/mailnews/base/resources/content/markByDate.js,v <-- markByDate.js new revision: 1.3; previous revision: 1.2 done Checking in mailnews/base/resources/content/messageWindow.js; /cvsroot/mozilla/mailnews/base/resources/content/messageWindow.js,v <-- messageWindow.js new revision: 1.120; previous revision: 1.119 done Checking in mailnews/base/resources/content/messengerdnd.js; /cvsroot/mozilla/mailnews/base/resources/content/messengerdnd.js,v <-- messengerdnd.js new revision: 1.65; previous revision: 1.64 done Checking in mailnews/base/search/src/nsMsgFilterService.cpp; /cvsroot/mozilla/mailnews/base/search/src/nsMsgFilterService.cpp,v <-- nsMsgFilterService.cpp new revision: 1.67; previous revision: 1.66 done Checking in mailnews/base/src/nsMessenger.cpp; /cvsroot/mozilla/mailnews/base/src/nsMessenger.cpp,v <-- nsMessenger.cpp new revision: 1.389; previous revision: 1.388 done Checking in mailnews/base/src/nsMsgCopyService.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgCopyService.cpp,v <-- nsMsgCopyService.cpp new revision: 1.61; previous revision: 1.60 done Checking in mailnews/base/src/nsMsgCopyService.h; /cvsroot/mozilla/mailnews/base/src/nsMsgCopyService.h,v <-- nsMsgCopyService.h new revision: 1.16; previous revision: 1.15 done Checking in mailnews/base/src/nsMsgDBView.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgDBView.cpp,v <-- nsMsgDBView.cpp new revision: 1.312; previous revision: 1.311 done Checking in mailnews/base/src/nsMsgFolderDataSource.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgFolderDataSource.cpp,v <-- nsMsgFolderDataSource.cpp new revision: 1.222; previous revision: 1.221 done Checking in mailnews/base/src/nsMsgFolderDataSource.h; /cvsroot/mozilla/mailnews/base/src/nsMsgFolderDataSource.h,v <-- nsMsgFolderDataSource.h new revision: 1.84; previous revision: 1.83 done Checking in mailnews/base/src/nsMsgFolderNotificationService.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgFolderNotificationService.cpp,v <-- nsMsgFolderNotificationService.cpp new revision: 1.4; previous revision: 1.3 done Checking in mailnews/base/src/nsMsgPurgeService.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgPurgeService.cpp,v <-- nsMsgPurgeService.cpp new revision: 1.32; previous revision: 1.31 done Checking in mailnews/base/src/nsMsgPurgeService.h; /cvsroot/mozilla/mailnews/base/src/nsMsgPurgeService.h,v <-- nsMsgPurgeService.h new revision: 1.5; previous revision: 1.4 done Checking in mailnews/base/src/nsMsgSearchDBView.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgSearchDBView.cpp,v <-- nsMsgSearchDBView.cpp new revision: 1.65; previous revision: 1.64 done Checking in mailnews/base/util/nsImapMoveCoalescer.cpp; /cvsroot/mozilla/mailnews/base/util/nsImapMoveCoalescer.cpp,v <-- nsImapMoveCoalescer.cpp new revision: 1.18; previous revision: 1.17 done Checking in mailnews/base/util/nsMsgDBFolder.cpp; /cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v <-- nsMsgDBFolder.cpp new revision: 1.343; previous revision: 1.342 done Checking in mailnews/compose/resources/content/MsgComposeCommands.js; /cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v <-- MsgComposeCommands.js new revision: 1.417; previous revision: 1.416 done Checking in mailnews/compose/src/nsMsgCompose.cpp; /cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v <-- nsMsgCompose.cpp new revision: 1.563; previous revision: 1.562 done Checking in mailnews/compose/src/nsMsgSendLater.cpp; /cvsroot/mozilla/mailnews/compose/src/nsMsgSendLater.cpp,v <-- nsMsgSendLater.cpp new revision: 1.122; previous revision: 1.121 done Checking in mailnews/db/msgdb/public/nsMsgDatabase.h; /cvsroot/mozilla/mailnews/db/msgdb/public/nsMsgDatabase.h,v <-- nsMsgDatabase.h new revision: 1.133; previous revision: 1.132 done Checking in mailnews/db/msgdb/src/nsMsgDatabase.cpp; /cvsroot/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp,v <-- nsMsgDatabase.cpp new revision: 1.399; previous revision: 1.398 done Checking in mailnews/imap/src/nsImapMailFolder.cpp; /cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v <-- nsImapMailFolder.cpp new revision: 1.814; previous revision: 1.813 done Checking in mailnews/imap/src/nsImapMailFolder.h; /cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.h,v <-- nsImapMailFolder.h new revision: 1.251; previous revision: 1.250 done Checking in mailnews/imap/src/nsImapOfflineSync.cpp; /cvsroot/mozilla/mailnews/imap/src/nsImapOfflineSync.cpp,v <-- nsImapOfflineSync.cpp new revision: 1.69; previous revision: 1.68 done Checking in mailnews/local/public/nsIMsgLocalMailFolder.idl; /cvsroot/mozilla/mailnews/local/public/nsIMsgLocalMailFolder.idl,v <-- nsIMsgLocalMailFolder.idl new revision: 1.21; previous revision: 1.20 done Checking in mailnews/local/src/nsLocalMailFolder.cpp; /cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v <-- nsLocalMailFolder.cpp new revision: 1.584; previous revision: 1.583 done Checking in mailnews/local/src/nsLocalMailFolder.h; /cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.h,v <-- nsLocalMailFolder.h new revision: 1.165; previous revision: 1.164 done Checking in mailnews/local/src/nsLocalUndoTxn.cpp; /cvsroot/mozilla/mailnews/local/src/nsLocalUndoTxn.cpp,v <-- nsLocalUndoTxn.cpp new revision: 1.56; previous revision: 1.55 done Checking in mailnews/local/src/nsParseMailbox.cpp; /cvsroot/mozilla/mailnews/local/src/nsParseMailbox.cpp,v <-- nsParseMailbox.cpp new revision: 1.302; previous revision: 1.301 done Checking in mailnews/news/src/nsNNTPNewsgroupList.cpp; /cvsroot/mozilla/mailnews/news/src/nsNNTPNewsgroupList.cpp,v <-- nsNNTPNewsgroupList.cpp new revision: 1.143; previous revision: 1.142 done Checking in mailnews/news/src/nsNewsFolder.cpp; /cvsroot/mozilla/mailnews/news/src/nsNewsFolder.cpp,v <-- nsNewsFolder.cpp new revision: 1.297; previous revision: 1.296 done Checking in mailnews/news/src/nsNewsFolder.h; /cvsroot/mozilla/mailnews/news/src/nsNewsFolder.h,v <-- nsNewsFolder.h new revision: 1.102; previous revision: 1.101 done Checking in mail/base/content/mail3PaneWindowCommands.js; /cvsroot/mozilla/mail/base/content/mail3PaneWindowCommands.js,v <-- mail3PaneWindowCommands.js new revision: 1.46; previous revision: 1.45 done Checking in mail/base/content/mailCommands.js; /cvsroot/mozilla/mail/base/content/mailCommands.js,v <-- mailCommands.js new revision: 1.48; previous revision: 1.47 done Checking in mail/base/content/mailWindowOverlay.js; /cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v <-- mailWindowOverlay.js new revision: 1.200; previous revision: 1.199 done Checking in mail/components/compose/content/MsgComposeCommands.js; /cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v <-- MsgComposeCommands.js new revision: 1.138; previous revision: 1.137 done

Thanks Mark

I have a regression from this. In mailCommands.js, we have: function performActionsOnJunkMsgs(aFolder, aMsgHdrs) { if (!aMsgHdrs.Count()) return; But aMsgHdrs is now an isMutableArray/nsIArray The correct version should be: if (!aMsgHdrs.length) if I understand this correctly. But i'm fairly new to nsIMutableArray.

Thanks rkent for reporting this, you're right. This also fixes the SM version, which didn't require a change in the first place. BTW bug 373270 should definitely be looked into for SM as well.

Comment on attachment 323686 [details] [diff] [review] Patch to fix mailCommands.js regression introduced cvsblame link for mail/base/content/mailCommands.js: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mail/base/content/mailCommands.js#538 (this hasn't been ported to SM)

Checking in mail/base/content/mailCommands.js; /cvsroot/mozilla/mail/base/content/mailCommands.js,v <-- mailCommands.js new revision: 1.49; previous revision: 1.48 done Checking in mailnews/base/resources/content/mailCommands.js; /cvsroot/mozilla/mailnews/base/resources/content/mailCommands.js,v <-- mailCommands.js new revision: 1.114; previous revision: 1.113 done