Last Comment Bug 783491 - tweak outdated code in mail/base/content/FilterListDialog.js
: tweak outdated code in mail/base/content/FilterListDialog.js
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
-- trivial (vote)
: Thunderbird 19.0
Assigned To: :aceman
Depends on: 780473
  Show dependency treegraph
Reported: 2012-08-17 01:25 PDT by :aceman
Modified: 2012-10-19 17:19 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (32.71 KB, patch)
2012-10-05 14:04 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch fix (894 bytes, patch)
2012-10-15 14:00 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description User image :aceman 2012-08-17 01:25:34 PDT
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.

*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.

Mconley, are these proposals sane?
Comment 1 User image :aceman 2012-08-17 02:02:21 PDT
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)) {

     // for POP3 and IMAP, select the first folder, which is the INBOX
     document.getElementById("runFiltersFolder").selectedIndex = 0;
   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 User image Kent James (:rkent) 2012-08-17 09:04:29 PDT
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.
Comment 3 User image :aceman 2012-09-15 09:40:53 PDT
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.
Comment 4 User image :aceman 2012-10-05 11:16:49 PDT
Comment 3 is invalid, that line is from another file.
Comment 5 User image :aceman 2012-10-05 14:04:27 PDT
Created attachment 668605 [details] [diff] [review]
Comment 6 User image Magnus Melin 2012-10-11 12:55:14 PDT
Comment on attachment 668605 [details] [diff] [review]

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
Comment 7 User image :aceman 2012-10-11 14:07:24 PDT
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 User image Magnus Melin 2012-10-11 23:00:49 PDT
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.
Comment 9 User image :aceman 2012-10-11 23:22:28 PDT
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.
Comment 10 User image Magnus Melin 2012-10-11 23:53:12 PDT
Last i checked it's hardly measurable, you need to many many calls and to get it up to 1ms.
Comment 11 User image Ryan VanderMeulen [:RyanVM] 2012-10-12 19:23:33 PDT
Comment 12 User image :aceman 2012-10-15 14:00:14 PDT
Created attachment 671593 [details] [diff] [review]
patch fix

Sorry, forgot to address the nit from review.
Comment 13 User image Ryan VanderMeulen [:RyanVM] 2012-10-19 17:19:28 PDT

Note You need to log in before you can comment on or make changes to this bug.