Currently it is possible to enter letters into the "For at most X days" field (which get converted to 0). Also the functions setupMailOnServerUI() and setupAgeMsgOnServerUI() could be reduced to onCheckItem() as proposed in bug 728681. Also, on the same pane, the server port field should not allow setting a port of 0.

Converting setupAgeMsgOnServerUI() will need to extend onCheckItem() to allow several "master" elements that must all be checked to enable the slave element. That will also be usefull on e.g. the Junk settings panel (for "delete junk older than X days").

Comment on attachment 626589 [details] [diff] [review] patch >+++ b/mailnews/base/prefs/content/am-prefs.js >+function onCheckItem(changeElementId, checkElementIds) If you are changing this line you may as well make the arguments of the form aVariableName. > { >+ let elementToControl = document.getElementById(changeElementId); > >+ let enabled = true; >+ if (!(checkElementIds instanceof Array)) >+ checkElementIds = [ checkElementIds ]; My preference is you change all the callers so they pass an array (even it is an array of one). >+++ b/mailnews/base/prefs/content/am-server.js > function setupMailOnServerUI() >+{ >+ onCheckItem('pop3.deleteMailLeftOnServer', 'pop3.leaveMessagesOnServer'); Nit: this file uses double quotes not single quotes, so please use double. >+ setupAgeMsgOnServerUI(); > } > > function setupAgeMsgOnServerUI() >+{ >+ const leaveMsgsId = 'pop3.leaveMessagesOnServer'; >+ const deleteByAgeId = 'pop3.deleteByAgeFromServer'; >+ onCheckItem(deleteByAgeId, leaveMsgsId); >+ onCheckItem('daysEnd', leaveMsgsId); >+ onCheckItem('pop3.numDaysToLeaveOnServer', [ leaveMsgsId, deleteByAgeId ]); Nit: this file uses double quotes not single quotes, so please use double. Nit: you don't need spaces between [ and the const. My preference is not to use the constants here but just the actual value each time. r- for the moment as I would like to see the new patch.

(In reply to Ian Neal from comment #3) > > function setupAgeMsgOnServerUI() > >+{ > >+ const leaveMsgsId = 'pop3.leaveMessagesOnServer'; > >+ const deleteByAgeId = 'pop3.deleteByAgeFromServer'; > >+ onCheckItem(deleteByAgeId, leaveMsgsId); > >+ onCheckItem('daysEnd', leaveMsgsId); > >+ onCheckItem('pop3.numDaysToLeaveOnServer', [ leaveMsgsId, deleteByAgeId ]); > My preference is not to use the constants here but just the actual value > each time. Why? Isn't it cleaner and more obvious with constants? I have personally made a mistake in the IDs that only got visible when I converted it to constants.

Comment on attachment 627463 [details] [diff] [review] patch v2 What about am-offline.js and am-newsblog.js/xul?

Thanks for catching feeds. But what's up with am-offline? It has its own copy of onCheckItem and will be migrated in bug 755885.

Comment on attachment 627500 [details] [diff] [review] patch v3 >+++ b/mailnews/base/prefs/content/am-server.js > function setupAgeMsgOnServerUI() >+{ >+ const leaveMsgsId = "pop3.leaveMessagesOnServer"; >+ const deleteByAgeId = "pop3.deleteByAgeFromServer"; >+ onCheckItem(deleteByAgeId, [leaveMsgsId]); >+ onCheckItem("daysEnd", [leaveMsgsId]); Nit: extra space after , >+ onCheckItem("pop3.numDaysToLeaveOnServer", [leaveMsgsId, deleteByAgeId]); > } r=me with that fixed

Thanks, done.

Comment on attachment 627555 [details] [diff] [review] patch v4 Review of attachment 627555 [details] [diff] [review]: ----------------------------------------------------------------- Just one super tiny nit. r=me with that fixed. Good work! ::: mailnews/base/prefs/content/am-server.js @@ +222,5 @@ > } > > function setupAgeMsgOnServerUI() > +{ > + const leaveMsgsId = "pop3.leaveMessagesOnServer"; const variables should begin with a k - see Variable Prefixes under https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Naming_and_formatting_code @@ +226,5 @@ > + const leaveMsgsId = "pop3.leaveMessagesOnServer"; > + const deleteByAgeId = "pop3.deleteByAgeFromServer"; > + onCheckItem(deleteByAgeId, [leaveMsgsId]); > + onCheckItem("daysEnd", [leaveMsgsId]); > + onCheckItem("pop3.numDaysToLeaveOnServer", [leaveMsgsId, deleteByAgeId]); I like how you've axed a lot of the extraneous logic from this function, and put it into onCheckItem instead. That's good stuff.

Thanks! The new code is probably a tiny bit slower but is not used much and is probably more readable. I hope to migrate more stuff to the onCheckItem and use the cache of locked prefs so it gets fast again (bug 755885).