Closed
Bug 783491
Opened 12 years ago
Closed 12 years ago
tweak outdated code in mail/base/content/FilterListDialog.js
Categories
(MailNews Core :: Filters, defect)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 19.0
People
(Reporter: aceman, Assigned: aceman)
References
Details
Attachments
(2 files)
32.71 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
894 bytes,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
I think this lines should be changed: *In function setFolder(msgFolder): - This code should be removed. The comment does not belong here and the functions calls are already part of rebuildFilterList() call above: // Get the first folder for this server. INBOX for // imap and pop accts and 1st news group for news. updateButtons(); updateCountBox(); *In function updateButtons(): - This variable could be inlined with the next code line: var filter = currentFilter(); - This comment does not match the code (the "unparsable" part has inverted logic): // "edit" only enabled when one filter selected or if we couldn't parse the filter var disabled = !oneFilterSelected || filter.unparseable - The variable name could also be improved, as the other ones in the function. Actually we could probably use one variable for all the disabling decisions as they are sequential. - Add spaces to: var downDisabled = !(oneFilterSelected && list.currentIndex < list.getRowCount()-1); - These can probably be inlined (variables removed) as the other buttons are: // special buttons var buttonTop = document.getElementById("reorderTopButton"); var buttonBottom = document.getElementById("reorderBottomButton"); buttonTop.disabled = upDisabled; buttonBottom.disabled = downDisabled; *In function updateCountBox(): - The second argument to Replace does not need to be an array, also add a space. .replace("#1",[len]); Mconley, are these proposals sane?
I also think this comment is not true (in current versions these elements are exposed in News too, probably CanRunFiltersAfterTheFact is true now): // run filters after the fact not supported by news if (CanRunFiltersAfterTheFact(msgFolder.server)) { document.getElementById("runFiltersFolder").removeAttribute("hidden"); document.getElementById("runFiltersButton").removeAttribute("hidden"); document.getElementById("folderPickerPrefix").removeAttribute("hidden"); // for POP3 and IMAP, select the first folder, which is the INBOX document.getElementById("runFiltersFolder").selectedIndex = 0; runMenu.selectFolder(getFirstFolder(msgFolder)); } else { document.getElementById("runFiltersFolder").setAttribute("hidden", "true"); document.getElementById("runFiltersButton").setAttribute("hidden", "true"); document.getElementById("folderPickerPrefix").setAttribute("hidden", "true"); } This code could also be optimized by setting hidden on the parent hbox, that contains these 3 elements. And use something like hbox.hidden = !CanRunFiltersAfterTheFact(msgFolder.server);
Comment 2•12 years ago
|
||
IMHO "- This variable could be inlined with the next code line:" is not a reason that we should be touching working code, unless there is a concurrent fix that should be done. Code churn can cause its own problems, as well as take effort.
The variable var nsMsgSearchScope = Components.interfaces.nsMsgSearchScope; is not used at all places where it could be. In function: function initializeDialog(filter) var gSearchScope = getFilterScope(getScope(filter), filter.filterType, filter.filterList); I don't see a reason for this variable to have 'g' prefix, it is not global.
Attachment #668605 -
Flags: review?(mkmelin+mozilla)
Comment 6•12 years ago
|
||
Comment on attachment 668605 [details] [diff] [review] patch Review of attachment 668605 [details] [diff] [review]: ----------------------------------------------------------------- Ok i suppose, but i'm not super exited about using global variables, though in this case it may make things easier... ::: mail/base/content/FilterListDialog.js @@ +792,5 @@ > { > if (event.charCode == KeyEvent.DOM_VK_SPACE) > { > + for each (let item in gFilterListbox.selectedItems) > + toggleFilter(item); add braces for the for-statement
Attachment #668605 -
Flags: review?(mkmelin+mozilla) → review+
This using of global variable for elements that are often references seems to be standard practice (see also Seamonkey files and the filter list dialog). Only when all of the code is inside an object then also these variables are inside the object. How would you solve it better?
Comment 8•12 years ago
|
||
That's just because the code is old, and practices used to be bad :) In general they make coding hard as the code gets so much less self contained. Since this is just for inside the dialog I think it's ok, but in general just using document.getElementById is almost zero cost and should be preferred.
I like the global vars for the elements better as you can see you use the same element in all functions and can change the ID easily if needed. I'd like to know how costly document.getElementById is and will benchmark it somehow.
Keywords: checkin-needed
Comment 10•12 years ago
|
||
Last i checked it's hardly measurable, you need to many many calls and to get it up to 1ms.
Comment 11•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/fbead5325860
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•12 years ago
|
||
Sorry, forgot to address the nit from review.
Attachment #671593 -
Flags: review+
Comment 13•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/6a8917ac00c9
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•