The style in mailWindowOverlay.js is quite mixed, and especially around the tabmail code indention is off to the point the code's quite hard to read. The patch should have no functional change, applies on top of the latest patch from bug 440616.

Comment on attachment 345883 [details] [diff] [review] proposed fix All this cleanup is great, but I'm a bit concerned about destroying a lot of cvs/hg blame here. I'd want bienvenu to probably sign off on doing this. Assuming he's ok with it, and since you're touching so much, most of these comments are about better things we can do for those lines, while we're messing with them.

Oops, most of my review didn't make the copy/paste: if (message_menuitem) { - var message_menuitem_hidden = message_menuitem.getAttribute("hidden"); - if(message_menuitem_hidden != "true"){ - message_menuitem.setAttribute('checked', !IsMessagePaneCollapsed()); - message_menuitem.setAttribute('disabled', gAccountCentralLoaded); - } + var message_menuitem_hidden = message_menuitem.getAttribute("hidden"); + if (message_menuitem_hidden != "true") { If we're going to work on this style stuff, we should probably figure out what this file's policy will be on braces (newline or same line) // Disable some menus if account manager is showing var sort_menuitem = document.getElementById('viewSortMenu'); - if (sort_menuitem) { + if (sort_menuitem) sort_menuitem.setAttribute("disabled", gAccountCentralLoaded); - } I can't find a scenario where these items won't exist. We should just remove them entirely (and that means we can probably get rid of the temp variable too). - // hide the views menu item if the user doesn't have the views toolbar button visible + // Hide the views menu item if the user doesn't have the views toolbar button visible. Capitalization and punctuation are great, but this comment needs to be wrapped. // Initialize the View Attachment Inline menu var viewAttachmentInline = pref.getBoolPref("mail.inline_attachments"); - document.getElementById("viewAttachmentsInlineMenuitem").setAttribute("checked", viewAttachmentInline ? "true" : "false"); + document.getElementById("viewAttachmentsInlineMenuitem") + .setAttribute("checked", viewAttachmentInline ? "true" : "false"); I'm not clear on the need for the ternary/string conversion. var sortOrder = gDBView.sortOrder; - var sortTypeSupportsGrouping = (sortType == nsMsgViewSortType.byAuthor - || sortType == nsMsgViewSortType.byDate || sortType == nsMsgViewSortType.byReceived || sortType == nsMsgViewSortType.byPriority + var sortTypeSupportsGrouping = (sortType == nsMsgViewSortType.byAuthor + || sortType == nsMsgViewSortType.byDate || sortType == nsMsgViewSortType.byReceived + || sortType == nsMsgViewSortType.byPriority Operators go at the end of the line, not the beginning if(allMenuItem) - allMenuItem.setAttribute("checked", (viewFlags & nsMsgViewFlagsType.kUnreadOnly) == 0 && (viewType == nsMsgViewType.eShowAllThreads)); + allMenuItem.setAttribute("checked", + (viewFlags & nsMsgViewFlagsType.kUnreadOnly) == 0 && + (viewType == nsMsgViewType.eShowAllThreads)); Throughout this function we have a bunch more of what seem to be unnecessary if checks. At a minimum, can you add a space between the ifs and the (s, but better to remove them. if(unreadMenuItem) - unreadMenuItem.setAttribute("checked", (viewFlags & nsMsgViewFlagsType.kUnreadOnly) != 0); + unreadMenuItem.setAttribute("checked", (viewFlags & nsMsgViewFlagsType.kUnreadOnly) != 0); All of these lines that you're touching in this function need to be wrapped. function InitMessageMark() { var areMessagesRead = SelectedMessagesAreRead(); var readItem = document.getElementById("cmd_markAsRead"); if(readItem) - readItem.setAttribute("checked", areMessagesRead); + readItem.setAttribute("checked", areMessagesRead); This function has the same problems as the above: unnecessary, or at least incorrectly formatted if() checks. function SelectedMessagesAreDeleted() { - try { - return gDBView.hdrForFirstSelectedMessage.flags & MSG_FLAG_IMAP_DELETED; - } - catch (ex) { - return 0; - } + try { + return gDBView.hdrForFirstSelectedMessage.flags & MSG_FLAG_IMAP_DELETED; + } + catch (ex) { + return 0; + } How about just if (gDBView), rather than try? + var isJunk; + try { + var junkScore = gDBView.hdrForFirstSelectedMessage.getStringProperty("junkscore"); + isJunk = ((junkScore != "") && (junkScore != "0")); + } + catch (ex) { + isJunk = false; + } + return isJunk; There's no need for isJunk here, just return at the end of the try and in the catch. The next 2 functions also have the same format, and the same comment applies. Bonus points if you can get rid of the try/catch in them with an if(). + var allServers = accountManager.allServers; + // array of isupportsarrays of servers for a particular folder + var pop3DownloadServersArray = new Array(); + // parallel isupports array of folders to download to... + var localFoldersToDownloadTo = Components.classes["@mozilla.org/supports-array;1"] + .createInstance(Components.interfaces.nsISupportsArray); The createInstance isn't aligned properly. (I'm nearly certain that 'align the dots' trumps '80 chars'. + var currentServer = allServers.GetElementAt(i).QueryInterface(Components.interfaces.nsIMsgIncomingServer); QueryElementAt, or this might be a good stop to use iteratorUtils.jsm. + dump(currentServer.serverURI + "...skipping, already opened\n"); Is there a compelling reason to keep this dump? + for (var i = 0; i < pop3DownloadServersArray.length; ++i) { - dump(ex + "\n"); + // any ol' pop3Server will do - the serversArray specifies which servers to download from + pop3Server.downloadMailFromServers(pop3DownloadServersArray[i], msgWindow, This comment still needs to be wrapped. ComposeMessage(aCompType, Components.interfaces.nsIMsgCompFormat.OppositeOfDefault, GetFirstSelectedMsgFolder(), GetSelectedMessages()); - } else { + } + else { Here it looks like you are trying to enforce a bracing structure... but elsewhere you weren't? + window.openDialog("chrome://messenger/content/newFolderDialog.xul", + "", "chrome,titlebar,modal", + {folder: destinationFolder, + dualUseFolders: dualUseFolders, + okCallback:callBackFunctionName}); |window| isn't really necessary, and would make writing this under 80 char easier. -const nsIFilePicker = Components.interfaces.nsIFilePicker; Awesome. function MsgMarkMsgAsRead(markRead) { - if (!markRead) { - markRead = !SelectedMessagesAreRead(); - } - MarkSelectedMessagesRead(markRead); + if (!markRead) + markRead = !SelectedMessagesAreRead(); + MarkSelectedMessagesRead(markRead); } These next functions are just wrong. If markRead is false, we should respect that. The if() block should explicitly check for undefined or null. Fortunately, no one has ever actually tried to use this parameter, in which case we might be better off just removing it. - var folder = GetMsgFolderFromUri(GetSelectedFolderURI(), true); + var folder = GetMsgFolderFromUri(GetSelectedFolderURI(), true); GetSelectedMsgFolders()[0]; + // If the user hits ok in the filterEditor dialog we set args.refresh=true + // there we check this here in args to show filterList dialog. Period after "there", capital W. - var selectedFolders = Components.classes["@mozilla.org/supports-array;1"].createInstance(Components.interfaces.nsISupportsArray); + var selectedFolders = Components.classes["@mozilla.org/supports-array;1"] + .createInstance(Components.interfaces.nsISupportsArray); selectedFolders.AppendElement(preselectedFolder); Another good spot for iteratorUtils. + gPrefBranch.setIntPref("mailnews.display.disallow_mime_handlers", + gDisallow_classes_no_html); Fix the alignment of gDisallow. + gPrefBranch.setIntPref("mailnews.display.disallow_mime_handlers", gDisallow_classes_no_html); This line is too long. function PrintEnginePrint() { - var messageList = GetSelectedMessages(); - return PrintEnginePrintInternal(messageList, messageList.length, false, Components.interfaces.nsIMsgPrintEngine.MNAB_PRINT_MSG); + var messageList = GetSelectedMessages(); + return PrintEnginePrintInternal(messageList, messageList.length, false, + Components.interfaces.nsIMsgPrintEngine.MNAB_PRINT_MSG); } function PrintEnginePrintPreview() { - var messageList = GetSelectedMessages(); - return PrintEnginePrintInternal(messageList, 1, true, Components.interfaces.nsIMsgPrintEngine.MNAB_PRINTPREVIEW_MSG); + var messageList = GetSelectedMessages(); + return PrintEnginePrintInternal(messageList, 1, true, + Components.interfaces.nsIMsgPrintEngine.MNAB_PRINTPREVIEW_MSG); Boy, there's really no reason for both these functions to do the messageList dance. How about moving that into Internal(), making this much easier to wrap? + var selectedFolders = GetSelectedMsgFolders(); + var numFolders = selectedFolders.length; + if (numFolders != 1) + return false; - var folder = selectedFolders[0]; - if (!folder) - return false; + var folder = selectedFolders[0]; + if (!folder) + return false; - var server = folder.server; - var serverType = server.type; - - if((serverType == "nntp")) - return false; - else return true; + return (folder.server.type != "nntp"); This can be simplified a lot, e.g. (You can do much the same with the next function too.) var selectedFolders = GetSelectedMsgFolders(); var folder = selectedFolders.length ? selectedFolders[0] : null; return folder && folder.server.type != "nntp"; + // Right now, all identities point to the same unsent messages // folder, so to avoid sending multiple copies of the // unsent messages, we only call messenger.SendUnsentMessages() once - // see bug #89150 for details + // see bug #89150 for details. Period after "once", capital S. -function CoalesceGetMsgsForPop3ServersByDestFolder(currentServer, pop3DownloadServersArray, localFoldersToDownloadTo) +function CoalesceGetMsgsForPop3ServersByDestFolder(currentServer, + pop3DownloadServersArray, localFoldersToDownloadTo) Boy, this function name makes it almost impossible to do the right thing. I think our best option here is to put each param on its own line, and try to line them each up with curreServer. + pop3DownloadServersArray[index] = Components.classes["@mozilla.org/supports-array;1"] + .createInstance(Components.interfaces.nsISupportsArray); } pop3DownloadServersArray[index].AppendElement(currentServer); Hey, another iterator. function CommandUpdate_UndoRedo() { - ShowMenuItem("menu_undo", true); - EnableMenuItem("menu_undo", SetupUndoRedoCommand("cmd_undo")); - ShowMenuItem("menu_redo", true); - EnableMenuItem("menu_redo", SetupUndoRedoCommand("cmd_redo")); + ShowMenuItem("menu_undo", true); + EnableMenuItem("menu_undo", SetupUndoRedoCommand("cmd_undo")); + ShowMenuItem("menu_redo", true); + EnableMenuItem("menu_redo", SetupUndoRedoCommand("cmd_redo")); menu_undo never gets hidden, from what I can tell. This is just hurting Ts. - var loadedFolder = GetLoadedMsgFolder(); + var loadedFolder = GetLoadedMsgFolder(); - // if we have selected a server, and are viewing account central - // there is no loaded folder - if (!loadedFolder) - return false; + // if we have selected a server, and are viewing account central + // there is no loaded folder + if (!loadedFolder) + return false; - var server = loadedFolder.server; - if (!(server.canUndoDeleteOnServer)) - return false; + var server = loadedFolder.server; + if (!(server.canUndoDeleteOnServer)) + return false; How about var loadedFolder = GetLoadedMsgFolder(); if (!loadFolder || !loadedFolder.server.canUndoDeleteOnServer) return false; + var canUndoOrRedo = false; + var txnType = 0; No need to assign these here, they get set in all code-paths. + if (canUndoOrRedo) + { + switch (txnType) { - canUndoOrRedo = messenger.canUndo(); - txnType = messenger.getUndoTransactionType(); + default: + case 0: + goSetMenuValue(command, 'valueDefault'); + break; + case 1: + goSetMenuValue(command, 'valueDeleteMsg'); + break; + case 2: + goSetMenuValue(command, 'valueMoveMsg'); + break; + case 3: + goSetMenuValue(command, 'valueCopyMsg'); + break; This can be nicely done in an array. var commands = ['valueDefault', 'valueDeleteMsg', 'valueMoveMsg', 'valueCopyMsg']; goSetMenuValue(command, commands[txnType]); + var sanitizeJunkMail = gPrefBranch.getBoolPref("mail.spam.display.sanitize"); + if (sanitizeJunkMail) // only bother doing this if we are modifying the html for junk mail.... This comment is too long. It should probably go on its own line. + // junk. Furthermore, if we are about to move the message that was just + // marked as junk then don't bother reloading it. Extra space before marked. + + // Reload the message if we've updated the remote content policy for the sender. Unfortunately just a bit too long. +/** + * IgnorePhishingWarning - set the msg hdr flag to ignore the phishing warning You previously removed function names from their comments. We should be consistent. + var wintype = document.documentElement.getAttribute('windowtype'); + if (wintype == "mail:messageWindow" || GetThreadTree().view.selection.currentIndex != gSelectedIndexWhenDeleting) This line is too long. At least move the second condition to its own line. + var currentMsgFolder = folder.QueryInterface(Components.interfaces.nsIMsgFolder); This seems unecessary, given that .folder already returns an nsIMsgFolder, no? http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgMailNewsUrl.idl#84 + res = outputPFC.copyMessages(currentMsgFolder, messages, false /*isMove*/, + msgWindow /*nsIMsgWindow*/, null /*listener*/, + false /*isFolder*/, false /*allowUndo*/ ); I missed where res got declared as a var. Also, no one does anything with it...

This should address most of your comments. Removed a bunch of unneeded checks, fixed a few other... I'd prefer to keep window in window.openDialog, just to make it clear it's global... I've not made it strictly 80 chars, I think when readability wins over a hard limit. Doing cleanup like this we loose some history, the reason I did it now is with the cvs/hg switchover the lost is fairly limited.

Comment on attachment 345965 [details] [diff] [review] proposed fix, v2 Almost there... The last set of changes showed me a couple more things. I'm sympathetic to the "we haven't lost much blame since hg" argument, but since blame can't be backed out, I'd just want a quick sign-off on the concept from bienvenu. - var labelMenu = document.getElementById("labelMenu"); - if(labelMenu) - labelMenu.setAttribute("disabled", !aMessage); + // Disable the Tag menu item if no message is selected or when we're + // not in a folder. + document.getElementById("tagMenu").disabled = !(selectedMsg && msgFolder); A very nice thing to catch. Anyone who doubts the danger of these if()s should see this. var areMessagesRead = SelectedMessagesAreRead(); - var readItem = document.getElementById("cmd_markAsRead"); - if(readItem) - readItem.setAttribute("checked", areMessagesRead); + document.getElementById("cmd_markAsRead") + .setAttribute("checked", areMessagesRead); var areMessagesFlagged = SelectedMessagesAreFlagged(); - var flaggedItem = document.getElementById("cmd_markAsFlagged"); - if(flaggedItem) - flaggedItem.setAttribute("checked", areMessagesFlagged); + document.getElementById("cmd_markAsFlagged") + .setAttribute("checked", areMessagesFlagged); areMessagesRead and areMessagesFlagged can go away. + return (gDBView && gDBView.numSelected) ? + gDBView.hdrForFirstSelectedMessage.isFlagged : false; These are much better than they were before, but I don't need the ternary. JS should return early if one side of an && fails, so just return gDBView && gDBView.numSelected && gDBView.hdrForFirstSelectedMessage.isFlagged; (parallel comments for the other functions) + var folder = GetFirstSelectedMsgFolder(); + if (folder) GetNextNMessages(folder); GetNextNMessages also checks for a valid folder-arg, so either the caller or the callee should stop doing that. } -const nsIFilePicker = Components.interfaces.nsIFilePicker; function MsgOpenFromFile() Just realized you're going to end up with 2 empty lines between functions here, remove one of them. function MsgViewAllHeaders() { - gPrefBranch.setIntPref("mail.show_headers",2); - ReloadMessage(); - return true; + gPrefBranch.setIntPref("mail.show_headers",2); + ReloadMessage(); + return true; From what I can tell, no one cares what these functions return. (parallel comment for the next few functions). - if (accountManager) { + if (accountManager) { allIdentities = accountManager.allIdentities; That's really gross. How about we just get the accountManager ourselves, and we'll know it exists then? - return (pref.getCharPref("mail.last_msg_movecopy_target_uri")); + var loadedFolder = GetLoadedMsgFolder(); + if (!loadedFolder || (pref.getBoolPref("mail.last_msg_movecopy_was_move") && + !loadedFolder.canDeleteMessages)) + return false; + return pref.getCharPref("mail.last_msg_movecopy_target_uri"); This looks like extra bits that found their way into this patch.

(In reply to comment #4) > (From update of attachment 345965 [details] [diff] [review]) > Almost there... The last set of changes showed me a couple more things. I'm > sympathetic to the "we haven't lost much blame since hg" argument, but since > blame can't be backed out, I'd just want a quick sign-off on the concept from > bienvenu. I'll let him stamp his sr! (hopefully) > + var folder = GetFirstSelectedMsgFolder(); > + if (folder) > GetNextNMessages(folder); > GetNextNMessages also checks for a valid folder-arg, so either the caller or > the callee should stop doing that. Removed from callee. > function MsgViewAllHeaders() > { > - gPrefBranch.setIntPref("mail.show_headers",2); > - ReloadMessage(); > - return true; > + gPrefBranch.setIntPref("mail.show_headers",2); > + ReloadMessage(); > + return true; > From what I can tell, no one cares what these functions return. (parallel > comment for the next few functions). And one of the fuctions was unused altogether. > - if (accountManager) { > + if (accountManager) { > allIdentities = accountManager.allIdentities; > That's really gross. How about we just get the accountManager ourselves, and > we'll know it exists then? Sounds good. > - return (pref.getCharPref("mail.last_msg_movecopy_target_uri")); > + var loadedFolder = GetLoadedMsgFolder(); > + if (!loadedFolder || > (pref.getBoolPref("mail.last_msg_movecopy_was_move") && > + !loadedFolder.canDeleteMessages)) > + return false; > + return pref.getCharPref("mail.last_msg_movecopy_target_uri"); > This looks like extra bits that found their way into this patch. Yes, and no. I noticed while testing I didn't break .eml files, the Copy/Move to Again was enabled - calling it will crash... like bp-dd28c6ee-a905-11dd-bde3-001cc4e2bf68 BTW, i didn't to the iteratorUtils changes - I'm not up to date enough with them to do it in this patch. Re the braces styles: I changed only one (or a couple?) of the "} else" style. If we want to make them all consistent I can make a followup for that. My preference would be the if () { } else { } style. But I'm totally fine with all on new lines too, as long as it's consistent.

David, can you sign of on this? Loss of some hg blame vs. style/consistency.

I don't mind losing the hg blame since it's not a long history.

Comment on attachment 346087 [details] [diff] [review] proposed fix, v3 thx for doing this; a few nits: I think isCreateSubfolders is only used once, and so the temp var can be removed: - var isCreateSubfolders = preselectedFolder.canCreateSubfolders; - if (!isCreateSubfolders) + var isCreateSubfolders = preselectedFolder.canCreateSubfolders; + if (!isCreateSubfolders) + { you maybe don't need the temp var srcFolder here, or the braces: + // If from the toolbar, return right away if this is a news message + // only allow cancel from the menu: "Edit | Cancel / Delete Message". + if (fromToolbar) + { + var srcFolder = GetLoadedMsgFolder(); + if (isNewsURI(srcFolder.URI)) { + // if news, don't delete + return; } + } do you need var curFilterList here?: + var curFilterList = folder.getFilterList(msgWindow); + args = {filterList: curFilterList}; + args.filterName = emailAddress; it would be nice to land this pretty soon. It all looks OK, but there's definitely a little potential for regressions.

Comment on attachment 346087 [details] [diff] [review] proposed fix, v3 + document.getElementById("cmd_markAsFlagged") + .setAttribute("checked", SelectedMessagesAreFlagged); You need some parens there to actually call the function. + // Each tab gets its own messenger instance; I assume this is so each one + // gets its own undo/redo stack? + messenger = Components.classes["@mozilla.org/messenger;1"] + .createInstance(Components.interfaces.nsIMessenger); + messenger.setWindow(window, msgWindow); aTab.messenger = messenger; This is way outside the scope of this bug, but this scares me to be using the global variable here. Might want to file a bug to investigate this... + var isJunk = (junkScore != "") && (junkScore != "0"); if (isNewsURI(hdr.folder.URI)) isJunk = true; That's awkward, how about some || love? +function MsgMarkAsFlagged() { - if (!markFlagged) { - markFlagged = !SelectedMessagesAreFlagged(); - } - MarkSelectedMessagesFlagged(markFlagged); + MarkSelectedMessagesFlagged(!SelectedMessagesAreFlagged()); } You can bet I'll be coming for these functions soon in my "remove functions that only make a single call" patches. :-) + var storeReadMailInPFC = imapServer.storeReadMailInPFC; + if (storeReadMailInPFC) Unnecessary temp var. + var messageID = msgHdr.messageId; + if (messageID.length > 0) This one is slightly more acceptable, because it's actually used again a line later, but it still seems over the top. + var readMailDB = outputPFC.getMsgDatabase(msgWindow); + if (readMailDB) Definitely no excuses here. + var hdrInDestDB = readMailDB.getMsgHdrForMessageID(messageID); + if (hdrInDestDB) Or here. (In fact, can probably combine this with the previous if()) + copyToOfflineFolder = false; Really, we don't need this variable either, just return; once you make your way through that if() forest. r=jminta with those.

Also, it would be beyond awesome if you wrote a MozMill test script that exercised some of these functions that you're touching.

(In reply to comment #9) > (From update of attachment 346087 [details] [diff] [review]) > + document.getElementById("cmd_markAsFlagged") > + .setAttribute("checked", SelectedMessagesAreFlagged); > You need some parens there to actually call the function. Whops, I'm pretty sure I fixed it locally, but must have gone unsaved. > + // Each tab gets its own messenger instance; I assume this is so each one > + // gets its own undo/redo stack? > + messenger = Components.classes["@mozilla.org/messenger;1"] > + .createInstance(Components.interfaces.nsIMessenger); > + messenger.setWindow(window, msgWindow); > aTab.messenger = messenger; > This is way outside the scope of this bug, but this scares me to be using the > global variable here. Might want to file a bug to investigate this... Will do. > + var readMailDB = outputPFC.getMsgDatabase(msgWindow); > + if (readMailDB) > Definitely no excuses here. This one is actually needed. Thanks for the reviews!

I've got one problem i noticed now on final testing, on creating a filter by right clicking an email header I get Error: document.getElementById("cmd_markAsRead") is null Source File: chrome://messenger/content/mailWindowOverlay.js Line: 676 Have to investigate that, I'm not sure why that would happen...

Additional one-liner to fix Comment #14. I don't know why that function call is there, cvs blame says it was changed from InitMessageLabel() in attachment 220473 [details] [diff] [review]. Looks phony to me... All it ever would do is cause the mailViewMenuItems commandset to update, but especially as the filter dialog is modal that seems utterly pointless.

changeset: 1031:5748fe6d63df http://hg.mozilla.org/comm-central/rev/5748fe6d63df ->FIXED Note to testers: this should have no functional change except for that the move/copy to again menu for .eml files is now enabled/disabled as appropriate. Check that things are working like before, and you don't get any new exceptions.