TB now has a UI (part of bug 449691) for the backend mail archive function, SeaMonkey needs an equivalent UI. Also see Bug 473212

Since we don't have message buttons (yet?) I guess we should do: 1. menu item with shortcut (en-US: 'A') like TB 2. context menu entry (TB bug 473214). Later we could add icons in follow-up bugs (IMO out of scope here because we'd need graphics for Classic and Modern and a discussion whether to have it visible by default or only as optional customizable toolbar buttons). The first point should be easy, simple port from TB (ignoring the buttons stuff). I think we should do this first, including the bugfixes (relevant changesets follow). Depending on what we agree upon and the patch writer we could do the context menu addition either in the same patch, an additional one or a new bug. Changesets (comm-central): 20af028c35be (menu item and key from messenger.dtd) 3e6578d421de (cmd_archive and BatchMessageMover) 339a8dd61afc (bugfix for local Archive folders) e2ce0613a997 (RSS special case) ecd6564e7caa (same folder case)

(In reply to comment #1) > 1. menu item with shortcut (en-US: 'A') like TB It's ok to have a keyboard shortcut for this, but please don't make it a single-key shortcut (especially not next to the Shift/Caps Lock keys). See bug 476590 for the related discussion on Thunderbird's shortcut, just too easy to hit accidentally for a function that may not be used as frequently. Also, a toolbar button/icon (TB bug 480470) would be good to have and may be the optimum solution (similar to the current File button, maybe even part of the default set).

Jens, the suggestion of starting with what we can easily port is good, the menu entry sounds fine for now, context menu or toolbar button as said in comment #2 can be added in followups, I guess.

(In reply to comment #2) > (In reply to comment #1) > > 1. menu item with shortcut (en-US: 'A') like TB > > It's ok to have a keyboard shortcut for this, but please don't make it a > single-key shortcut (especially not next to the Shift/Caps Lock keys). > See bug 476590 for the related discussion on Thunderbird's shortcut, just too > easy to hit accidentally for a function that may not be used as frequently. I agree with your proposal in that bug to use Shift+A instead. Please keep monitoring the discussion there, maybe they come up with a better solution.

This adds a menu item, context menu entry and shortcut (Shift+A). Note: I replaced cmd_close at that position because it is listed again equally further down (TB has the same bug).

Comment on attachment 376638 [details] [diff] [review] added Archive to context/menu Looks basically good, "just" a ton of comments... >diff --git a/suite/mailnews/mail3PaneWindowCommands.js b/suite/mailnews/mail3PaneWindowCommands.js > supportsCommand: function(command) > { > > switch ( command ) > { > case "cmd_createFilterFromPopup": >- case "cmd_close": >+ case "cmd_archive": Please use the correct level 3 indent here (=6 spaces, like cmd_createFilterFromPopup and others). Most of the items are still misindented, we'll correct this on touch to avoid mere whitespace changes. >diff --git a/suite/mailnews/mailCommands.js b/suite/mailnews/mailCommands.js >+function getIdentityForHeader(hdr, type) (Especially new) Functions should start with a an uppercase letter. Arguments should have an 'a' prefix, e.g. aMsgHdr, aType. >+ // If we treat reply from sent specially, do we check for that folder flag here ? >+ var isTemplate = (type == Components.interfaces.nsIMsgCompType.Template); No braces needed. >+ var hintForIdentity = isTemplate ? hdr.author : hdr.recipients + hdr.ccList; >+ var identity = null; >+ var server; Please initialize server. >+ var accountKey = hdr.accountKey; >+ if (accountKey.length > 0) >+ { >+ var account = accountManager.getAccount(accountKey); Please use let for subscope variables. >+ if (!identity) >+ { >+ var allIdentities = accountManager.allIdentities; >+ identity = getBestIdentity(allIdentities, hintForIdentity); >+ } Drop "var allIdentities" and the braces, just align the call parameters vertically, if necessary. >diff --git a/suite/mailnews/mailWindowOverlay.js b/suite/mailnews/mailWindowOverlay.js >+BatchMessageMover.prototype = { Curly braces should go onto their own line, including this one. >+ archiveSelectedMessages: function() >+ { Should start with an uppercase letter, especially since most of the other member functions do as well! >+ // subtle: >+ SetNextMessageAfterDelete(); // we're just pretending Subtle? Maybe. Umcomprehensible? Definitely! Please replace these two comments with something actually describing what is happing and/or why. >+ this.messageToSelectAfterWereDone = gNextMessageViewIndexAfterDelete; >+ gNextMessageViewIndexAfterDelete = -2; Meh. What does -2 mean? (Yes, I know this evil pattern is used elsewhere.) Define a meaningful const along nsMsgViewIndex_None (which symbolizes -1) for this and use it in the new code. >+ for (let i = 0; i < selectedMsgUris.length; ++i) >+ { >+ let msgHdr = messenger.msgHdrFromURI(selectedMsgUris[i]); >+ >+ let rootFolder = msgHdr.folder.server.rootFolder; >+ You don't use rootFolder, just drop it along with the empty lines. >+ let msgDate = new Date(msgHdr.date / 1000); // convert date to JS date object >+ let msgYear = msgDate.getFullYear().toString(); >+ let monthFolderName = msgDate.toLocaleFormat("%Y-%m") Missing ; But why do you define monthFolderName at all if you only use it to initialize dstFolderName? >+ let dstFolderName = monthFolderName; >+ Drop the empty line. >+ let copyBatchKey = msgHdr.folder.URI + '\000' + dstFolderName; >+ if (!(copyBatchKey in this._batches)) >+ this._batches[copyBatchKey] = [msgHdr.folder, msgYear, dstFolderName]; >+ this._batches[copyBatchKey].push(msgHdr); >+ } >+ // Now we launch the code that will iterate over all of the message copies >+ // one in turn Might as well put the comment on one line. Comments starting with an uppercase word should end with dot etc. if they're sentences. >+ processNextBatch: function() >+ { Should start with an uppercase letter, especially since most of the other member functions do as well! >+ for (let key in this._batches) >+ { >+ this._currentKey = key; >+ let batch = this._batches[key]; >+ let srcFolder = batch[0]; >+ let msgYear = batch[1]; >+ let msgMonth = batch[2]; >+ let msgs = batch.slice(3,batch.length); >+ let subFolder, dstFolder; AFAICS, subFolder is only used on a deeper level. Define it there. >+ let Ci = Components.interfaces; >+ // rss servers don't have an identity so we special case the archives URI >+ let archiveFolderUri = (srcFolder.server.type == 'rss') >+ ? srcFolder.server.serverURI + "/Archives" >+ : getIdentityForHeader(msgs[0], Ci.nsIMsgCompType >+ .ReplyAll).archiveFolder; Using an if construct will greatly enhance readability. >+ let archiveFolder = GetMsgFolderFromUri(archiveFolderUri, false); >+ let granularity = archiveFolder.server.archiveGranularity; >+ // for imap folders, we need to create the sub-folders asynchronously, >+ // so we chain the urls using the listener called back from >+ // createStorageIfMissing. For local, creatStorageIfMissing is >+ // synchronous. OTOH, sentences ending in a dot should start with an uppercase letter... Also, seek and destroy the typo. ;-) >+ if (!archiveFolder.canCreateSubfolders) >+ granularity = Ci.nsIMsgIncomingServer.singleArchiveFolder; >+ if (granularity >= Ci.nsIMsgIncomingServer.perYearArchiveFolders) ... >+ if (granularity >= Ci.nsIMsgIncomingServer.perMonthArchiveFolders) On a sidenote: how are these server settings configured? All I got was a flat top level Archive folder with no subfolders... Part of a follow-up bug? >+ } >+ else >+ dstFolder = subFolder; >+ } >+ else >+ dstFolder = archiveFolder; Twice: If the "if" block has braces, the "else" block should have, too. >+ if (dstFolder != srcFolder) >+ { >+ var mutablearray = Components.classes["@mozilla.org/array;1"] >+ .createInstance(Components.interfaces.nsIMutableArray); Sublevel variables should use let, the . should align vertically. Also, you already defined Ci, might as well use it here... >+ msgs.forEach(function (item) { >+ mutablearray.appendElement(item, false); >+ }); Make this one line (with no spaces after 'function' and '{' and before '}'). >+ else >+ delete this._batches[key]; If the "if" block has braces, the "else" block should have, too. >+ OnStartRunningUrl: function(url) { >+ }, Braces should go onto their own line. Parameters should have an "a" prefix. >+ OnStopRunningUrl: function(url, exitCode) >+ { Parameters should have an "a" prefix. >+ // this will always be a create folder url, afaik. Sentences ending in a dot should start with an uppercase letter... >+ // also implements nsIMsgCopyServiceListener, but we only care >+ // about the OnStopCopy >+ OnStartCopy: function() { >+ }, >+ OnProgress: function(aProgress, aProgressMax) { >+ }, >+ SetMessageKey: function(aKey) { >+ }, >+ GetMessageId: function() { >+ }, Braces should go onto their own line. >+ OnStopCopy: function(aStatus) >+ { >+ if (aStatus == Components.results.NS_OK) >+ { >+ // remove batch we just finished >+ delete this._batches[this._currentKey]; >+ this._currentKey = null; >+ >+ // is there a safe way to test whether this._batches is empty? Sentences ending in a dot should start with an uppercase letter... >+ let empty = true; >+ for (let key in this._batches) >+ empty = false; You can break; out of the for loop on first sight of a false, it won't get any emptier after that. ;-) >+ Drop empty line. >+ if (empty) >+ { >+ // we're just going to select the message now >+ var treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView); >+ var treeSelection = treeView.selection; You don't need treeSelection. >+ treeSelection.select(this.messageToSelectAfterWereDone); >+ treeView.selectionChanged(); >+ } >+ else >+ this.processNextBatch(); Braces should go onto their own line. >+ QueryInterface: function(iid) >+ { Parameters should have an "a" prefix. >+ if (!iid.equals(Components.interfaces.nsIUrlListener) && >+ !iid.equals(Components.interfaces.nsIMsgCopyServiceListener) && >+ !iid.equals(Components.interfaces.nsISupports)) >+ throw Components.results.NS_ERROR_NO_INTERFACE; Please follow the usual pattern of if (aIID.equals(foo) || ...) return this; throw Components.results.NS_ERROR_NOINTERFACE; >diff --git a/suite/mailnews/mailWindowOverlay.xul b/suite/mailnews/mailWindowOverlay.xul > <key id="key_previousUnreadMsg" key="&prevUnreadMsgCmd.key;" oncommand="goDoCommand('cmd_previousUnreadMsg')"/> >+ <key id="key_archive" key="&archiveMsgCmd.key;" oncommand="goDoCommand('cmd_archive')" >+ modifiers="shift"/> Please follow the pattern below (ugly as it is) and just add the modifiers at the end of the line. (Or use one line per attribute, with oncommand last and all attributes aligned vertically.) > <key id="key_reply" key="&replyMsgCmd.key;" oncommand="goDoCommand('cmd_reply')" modifiers="accel"/> > <key id="key_replyall" key="&replyToAllMsgCmd.key;" oncommand="goDoCommand('cmd_replyall')" modifiers="accel, shift"/> >+ <menuitem id="archiveMainMenu" label="&archiveMsgCmd.label;" >+ accesskey="&archiveMsgCmd.accesskey;" >+ key="key_archive" >+ command="cmd_archive"/> The label needs to go onto a line of its own. Thanks for tackling this! :)

(In reply to comment #6) > >+ else > >+ this.processNextBatch(); > > Braces should go onto their own line. Uh, C&P error. Should read: If the if branch has braces, the else branch should have as well. > + if (granularity >= Ci.nsIMsgIncomingServer.perMonthArchiveFolders) Oh, and drop the stray secon space after >=. ;-)

>>+function getIdentityForHeader(hdr, type) > (Especially new) Functions should start with a an uppercase letter. >>+ archiveSelectedMessages: function() >>+ processNextBatch: function() However we should use the same case as Thunderbird for consistency and for extension compatibility. >+ let Ci = Components.interfaces; ... > Also, you already defined Ci, might as well use it here... I don't see |Ci = foo| getting past Neil anytime this millennium.

(In reply to comment #8) > However we should use the same case as Thunderbird for consistency and for > extension compatibility. I considered that, but these are only called internally or by a helper function. And you can't hook into this object member function from an extension anyway, it'd be mess regardless of case. > >+ let Ci = Components.interfaces; > ... > > Also, you already defined Ci, might as well use it here... > > I don't see |Ci = foo| getting past Neil anytime this millennium. Hm? It's just a local variable here... But he'll no doubt voice himself on that. *g*

(In reply to comment #9) > And you can't hook into this object member function from an extension Should read: And you can't hook easily into object member functions from an extension

> <bigeyes>It's just a local variable here...</bigeyes> It's been tried on Neil. Didn't work. > And you can't hook easily into object member functions from an extension I've looked into the innards of a lot of extensions while porting them to SeaMonkey and I can tell you that "hooking into object member functions from an extension" is a commonly used technique.

(In reply to comment #11) > > And you can't hook easily into object member functions from an extension > I've looked into the innards of a lot of extensions while porting them to > SeaMonkey and I can tell you that "hooking into object member functions from an > extension" is a commonly used technique. I know. I do. But it's ugly, error-prone and usually causes harm once two extensions fight for the same resource. Nothing to take care of here, imo.

My previous patch was mostly a rip-off from TB; especially the comments in BatchMessageMover were not by me. I made it slightly worse by trying to be smart adapting the brace style (there are braces around the else blocks in TB's version). The new patch should be better overall. (In reply to comment #6) > >+ // subtle: > >+ SetNextMessageAfterDelete(); // we're just pretending > > Subtle? Maybe. Umcomprehensible? Definitely! > Please replace these two comments with something actually describing what is > happing and/or why. I tried. Please see bug 451995 comment 13 yourself; neither the original patch nor the comment were by me. > >+ this.messageToSelectAfterWereDone = gNextMessageViewIndexAfterDelete; > >+ gNextMessageViewIndexAfterDelete = -2; > > Meh. What does -2 mean? (Yes, I know this evil pattern is used elsewhere.) > Define a meaningful const along nsMsgViewIndex_None (which symbolizes -1) for > this and use it in the new code. I didn't find a source that describes exactly what it means. Lacking a clue for a name I consequently didn't define a const for it. Beyond that I think this may be a bit out of scope here because, as you said, it's used in multiple other places (multiple files, even!). Hints: msgMail3PaneWindow.js#371, msgMail3PaneWindow.js#379. BTW: nsMsgViewIndex_None looks more like MAXINT rather than -1. ;-) > On a sidenote: how are these server settings configured? > All I got was a flat top level Archive folder with no subfolders... > Part of a follow-up bug? It seems subfolders (the default setting archives by year) aren't automatically subscribed. I'm not sure whether any bug exists for it (be it for TB or SM), though. > >+ if (empty) > >+ { > >+ // we're just going to select the message now > >+ var treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView); > >+ var treeSelection = treeView.selection; > > You don't need treeSelection. Is treeView a 'let' candidate here? Just guessing, I don't really get the point of when to use which (except for var for globals). Neil pointed out on IRC that he doesn't like 'let' outside loops so I left it untouched for now. > Thanks for tackling this! :) You're welcome! I just didn't want this feature to stay half-in-half-out for SM 2 (we've been able to select the Archive folder per account for quite some time now but otherwise it was a dead end).

Comment on attachment 378390 [details] [diff] [review] patch v2 (In reply to comment #13) > > Meh. What does -2 mean? (Yes, I know this evil pattern is used elsewhere.) > > Define a meaningful const along nsMsgViewIndex_None (which symbolizes -1) > > for this and use it in the new code. > > I didn't find a source that describes exactly what it means. Lacking a clue > for a name I consequently didn't define a const for it. I filed bug 494242 on the issue. > BTW: nsMsgViewIndex_None looks more like MAXINT rather than -1. ;-) Well, nsMsgViewIndex is "typedef unsigned long", which is 32 bit... > It seems subfolders (the default setting archives by year) aren't > automatically subscribed. I'm not sure whether any bug exists for it > (be it for TB or SM), though. Care to file one? O:-) > Is treeView a 'let' candidate here? Just guessing, I don't really get the > point of when to use which (except for var for globals). Neil pointed out on > IRC that he doesn't like 'let' outside loops so I left it untouched for now. Generally, "let" should be preferred in subscopes, where the outer scope doesn't need to know about the variable. On a function-global scope, they work the same, so actually no reason to use it there either. IMO, "var" is pretty much dead since the invention of "let". OTOH, older (web) clients or debuggers may have problems with it - but this doesn't apply for chrome code (and we're going to fix Venkman/JSD eventually). And, finally, as for the new patch itself: >diff --git a/suite/locales/en-US/chrome/mailnews/messenger.dtd >+ else { >+ dstFolder = archiveFolder; ... >+ else { >+ delete this._batches[key]; ... >+ OnStartRunningUrl: function(aUrl) { These braces should go onto the next line as well. r=me with that fixed on checkin.

(In reply to comment #14) > On a function-global scope, they work the same, so actually no reason to use > it there either. it=var > OTOH, older (web) clients or debuggers may have problems with it it=let

(In reply to comment #14) > > It seems subfolders (the default setting archives by year) aren't > > automatically subscribed. I'm not sure whether any bug exists for it > > (be it for TB or SM), though. > > Care to file one? O:-) On second glance it seems the folder is actually subscribed but not shown directly whereas it works correctly in TB. Filed bug 494266. > > Is treeView a 'let' candidate here? Just guessing, I don't really get the > > point of when to use which (except for var for globals). Neil pointed out on > > IRC that he doesn't like 'let' outside loops so I left it untouched for now. > > Generally, "let" should be preferred in subscopes, where the outer scope > doesn't need to know about the variable. My example obviously matches your description so I changed my mind and replaced 'var' by 'let' here. Also fits better with the surrounding lines. > These braces should go onto the next line as well. Habit clash. ;-) Fixed. > r=me with that fixed on checkin. I cannot check in myself and people doing that for me prefer complete patches, thus I'm attaching a new one with the brace style fixes and the single var->let change.

(In reply to comment #16) > > r=me with that fixed on checkin. Just a habit of saying "no need to attach a new patch unless required by other circumstances". I'll try to avoid that in future. ;-)

(In reply to comment #16) > Created an attachment (id=378948) [details] > patch v2a, r=mnyromyr > > My example obviously matches your description so I changed my mind and replaced > 'var' by 'let' here. Also fits better with the surrounding lines. > (...) > thus I'm attaching a new one with the brace style fixes and the single var->let > change. Well, I did the var->let change locally but it didn't make it into the patch. I'll wait for Neil's feedback, then we can decide whether this needs another go or we can "let" it be. :-)

Comment on attachment 378948 [details] [diff] [review] patch v2a, r=mnyromyr >+<!ENTITY archiveMsgCmd.key "a"> Nit: two spaces >+ identity = folder.customIdentity; Could return early if identity is not null. >+ var accountKey = aMsgHdr.accountKey; >+ if (accountKey.length > 0) >+ { >+ let account = accountManager.getAccount(accountKey); >+ if (account) >+ server = account.incomingServer; >+ } >+ >+ if (server && !identity) >+ identity = getIdentityForServer(server, hintForIdentity); Or wrap the whole block in an if (!identity) check. >+BatchMessageMover.prototype = >+{ >+ Nit: unnecessary blank line >+ SetNextMessageAfterDelete(); >+ this.messageToSelectAfterWereDone = gNextMessageViewIndexAfterDelete; >+ gNextMessageViewIndexAfterDelete = -2; >+ >+ let selectedMsgUris = GetSelectedMessages(); >+ if (!selectedMsgUris.length) >+ return; Should do this first, no? >+ let msgs = batch.slice(3,batch.length); Don't need batch.length, it's the default >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer.perYearArchiveFolders) >+ { >+ archiveFolderUri += "/" + msgYear; >+ let subFolder = GetMsgFolderFromUri(archiveFolderUri, false); >+ if (!subFolder.parent) >+ { >+ subFolder.createStorageIfMissing(this); >+ if (isImap) >+ return; >+ } >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer.perMonthArchiveFolders) Don't bother nesting this, so we don't need two else's >+ let mutablearray = Components.classes["@mozilla.org/array;1"] >+ .createInstance(Components.interfaces.nsIMutableArray); Could just call this array? >+ msgs.forEach(function(item){mutablearray.appendElement(item, false);}); >+ gCopyService.CopyMessages(srcFolder, mutablearray, >+ dstFolder, true, this, msgWindow, true); >+ this._currentKey = key; Why not delete the batch first, so we don't have to do it later? Otherwise, set the current key earlier please. >+ break; // only do one. Should be return, see below. >+ } >+ else Don't need else after break/return. >+ if (aStatus == Components.results.NS_OK) What if this fails? >+ // Is there a safe way to test whether this._batches is empty? No point, just call ProcessNextBatch() again... >+ // We're just going to select the message now. >+ var treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView); >+ treeView.selection.select(this.messageToSelectAfterWereDone); >+ treeView.selectionChanged(); And do this when ProcessNextBatch runs out of batches. >+ QueryInterface: function(aIid) Nit: aIID >+ <menuitem id="mailContext-archive" >+ label="&contextArchive.label;" >+ accesskey="&contextArchive.accesskey;" >+ oncommand="MsgArchiveSelectedMessages(event);"/> No mailContextMenu.js code to enable/disable this menuitem...

(In reply to comment #19) > >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer.perYearArchiveFolders) > >+ { > >+ archiveFolderUri += "/" + msgYear; > >+ let subFolder = GetMsgFolderFromUri(archiveFolderUri, false); > >+ if (!subFolder.parent) > >+ { > >+ subFolder.createStorageIfMissing(this); > >+ if (isImap) > >+ return; > >+ } > >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer.perMonthArchiveFolders) > Don't bother nesting this, so we don't need two else's Actually I think without nesting we need none, see new patch. > >+ msgs.forEach(function(item){mutablearray.appendElement(item, false);}); > >+ gCopyService.CopyMessages(srcFolder, mutablearray, > >+ dstFolder, true, this, msgWindow, true); > >+ this._currentKey = key; > Why not delete the batch first, so we don't have to do it later? Currently it's deleted in OnStopCopy() which seems logical since only there we know the action succeeded, no? > Otherwise, set the current key earlier please. Actually it's already set at the beginning of the loop but CopyMessages() triggers OnStopCopy() which in turn calls ProcessNextBatch() again so _currentKey is overwritten. The line after CopyMessages() ensures that _currentKey is reset to the same value as before the call. At least that's what I understand so I left it in. Please clarify if I'm wrong. > >+ if (aStatus == Components.results.NS_OK) > What if this fails? I have no idea (I just ported all this from TB). Looks like, since the entry is not removed from the batches collection, it's retried as often as ProcessNextBatch is called again afterwards. > >+ // Is there a safe way to test whether this._batches is empty? > No point, just call ProcessNextBatch() again... > > >+ // We're just going to select the message now. > >+ var treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView); > >+ treeView.selection.select(this.messageToSelectAfterWereDone); > >+ treeView.selectionChanged(); > And do this when ProcessNextBatch runs out of batches. "runs out of batches" = "after the loop" as in the new patch or do I need to move the check with the "empty" variable there? > >+ <menuitem id="mailContext-archive" > >+ label="&contextArchive.label;" > >+ accesskey="&contextArchive.accesskey;" > >+ oncommand="MsgArchiveSelectedMessages(event);"/> > No mailContextMenu.js code to enable/disable this menuitem... Good catch, corrected.

(In reply to comment #20) > (In reply to comment #19) > > >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer.perMonthArchiveFolders) > > Don't bother nesting this, so we don't need two else's > Actually I think without nesting we need none, see new patch. Better still :-) > > >+ gCopyService.CopyMessages(srcFolder, mutablearray, > > >+ dstFolder, true, this, msgWindow, true); > > >+ this._currentKey = key; > > Otherwise, set the current key earlier please. > Actually it's already set at the beginning of the loop but CopyMessages() > triggers OnStopCopy() which in turn calls ProcessNextBatch() again so > _currentKey is overwritten. The line after CopyMessages() ensures that > _currentKey is reset to the same value as before the call. At least that's what > I understand so I left it in. Please clarify if I'm wrong. Before OnStopCopy calls ProcessNextBatch it deletes the current batch. To do this it needs the current key set here. It looks odd to set the current key after the call to the copy service whose callback uses it. (It would fail in the unlikely event of someone changing the copy service to be synchronous.) > > >+ if (aStatus == Components.results.NS_OK) > > What if this fails? > I have no idea (I just ported all this from TB). Looks like, since the entry is > not removed from the batches collection, it's retried as often as > ProcessNextBatch is called again afterwards. Sure, but nobody's around to call it... I guess all we can do is to copy the OnStopRunningUrl code.

Comment on attachment 379605 [details] [diff] [review] patch v3, r=mnyromyr >+ OnStopRunningUrl: function(aUrl, aExitCode) >+ { >+ // This will always be a create folder url, afaik. >+ if (aExitCode >= 0) >+ OnStopCopy: function(aStatus) >+ { >+ if (aStatus == Components.results.NS_OK) Sorry I overlooked this before, but I went and checked the interface definitions, and both of these codes are defined as nsresult, so >= 0 is definitely wrong, and == NS_OK isn't correct; you should use Components.isSuccessCode instead. sr=me with that fixed.

Pushed as http://hg.mozilla.org/comm-central/rev/583c02e1441a - Jens, I'll leave it up to you to mark this bug fixed if everything to be done in here has been done. Thanks a lot for the patch!

I filed an enhancement bug for the addition of a customizable toolbar button so we're done here.

Comment on attachment 379997 [details] [diff] [review] patch v3a, r=mnyromyr, sr=neil >+BatchMessageMover.prototype = >+{ >+ ArchiveSelectedMessages: function() >+ { >(...) >+ let messages = Components.classes["@mozilla.org/array;1"] >+ .createInstance(Components.interfaces.nsIMutableArray); Hmm, seems no-one noticed this is never used. Same with TB, dead from the start: <http://hg.mozilla.org/comm-central/rev/3e6578d421de>. I copied it blindly, sorry, my fault. Filed bug 571713 to get rid of it.