Last Comment Bug 783491 - tweak outdated code in mail/base/content/FilterListDialog.js
: tweak outdated code in mail/base/content/FilterListDialog.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
Depends on: 780473
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


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

Description :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.
   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?
Comment 1 :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)) {
     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 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 :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 :aceman 2012-10-05 11:16:49 PDT
Comment 3 is invalid, that line is from another file.
Comment 5 :aceman 2012-10-05 14:04:27 PDT
Created attachment 668605 [details] [diff] [review]
patch
Comment 6 Magnus Melin 2012-10-11 12:55:14 PDT
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
Comment 7 :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 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 :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 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 Ryan VanderMeulen [:RyanVM] 2012-10-12 19:23:33 PDT
https://hg.mozilla.org/comm-central/rev/fbead5325860
Comment 12 :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 Ryan VanderMeulen [:RyanVM] 2012-10-19 17:19:28 PDT
https://hg.mozilla.org/comm-central/rev/6a8917ac00c9

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