Bug 780473 - Optimize list rebuilding and filter update/movement operations in the filter list. r=mkmelin
authoraceman <acelists@atlas.sk>
Fri, 31 Aug 2012 06:27:50 -0400
changeset 10994 83616ca185bba6150105a8994e8ab3a020b07f40
parent 10993 d66004eb6054448f8b85b72aa50623cedba32488
child 10995 9e6f4b4667468e34eef1fc5615efc5b7955e293d
push id8245
push userryanvm@gmail.com
push dateFri, 31 Aug 2012 10:27:51 +0000
treeherdercomm-central@83616ca185bb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmkmelin
bugs780473
Bug 780473 - Optimize list rebuilding and filter update/movement operations in the filter list. r=mkmelin
mail/base/content/FilterListDialog.js
mail/base/content/FilterListDialog.xul
--- a/mail/base/content/FilterListDialog.js
+++ b/mail/base/content/FilterListDialog.js
@@ -118,17 +118,16 @@ function onFilterFolderClick(aFolder)
     if (!aFolder || aFolder == gCurrentFolder)
       return;
 
     // Save the current filters to disk before switching because
     // the dialog may be closed and we'll lose current filters.
     gCurrentFilterList.saveToDefaultFile();
 
     selectFolder(aFolder);
-    onFindFilter(false);
 }
 
 function CanRunFiltersAfterTheFact(aServer)
 {
   // filter after the fact is implement using search
   // so if you can't search, you can't filter after the fact
   return aServer.canSearchMessages;
 }
@@ -136,18 +135,18 @@ function CanRunFiltersAfterTheFact(aServ
 // roots the tree at the specified folder
 function setFolder(msgFolder)
 {
    if (msgFolder == gCurrentFolder)
      return;
    gCurrentFolder = msgFolder;
 
    //Calling getFilterList will detect any errors in rules.dat, backup the file, and alert the user
-   var filterList = msgFolder.getEditableFilterList(gFilterListMsgWindow);
-   rebuildFilterList(filterList);
+   gCurrentFilterList = msgFolder.getEditableFilterList(gFilterListMsgWindow);
+   rebuildFilterList();
 
    // Select the first item in the list, if there is one.
    var list = document.getElementById("filterList");
    if (list.getRowCount())
      list.selectItem(list.getItemAtIndex(0));
 
    // This will get the deferred to account root folder, if server is deferred.
    // We intentionally do this after setting gCurrentFolder, as we want
@@ -177,17 +176,16 @@ function setFolder(msgFolder)
      document.getElementById("runFiltersFolder").setAttribute("hidden", "true");
      document.getElementById("runFiltersButton").setAttribute("hidden", "true");
      document.getElementById("folderPickerPrefix").setAttribute("hidden", "true");
    }
 
    // Get the first folder for this server. INBOX for
    // imap and pop accts and 1st news group for news.
    updateButtons();
-   updateCountBox();
 }
 
 function toggleFilter(aFilter, aIndex)
 {
     if (aFilter.unparseable)
     {
       var bundle = document.getElementById("bundle_filter");
       var promptSvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
@@ -223,62 +221,51 @@ function onEditFilter()
   var selectedFilter = currentFilter();
   var args = {filter: selectedFilter, filterList: gCurrentFilterList};
 
   window.openDialog("chrome://messenger/content/FilterEditor.xul", "FilterEditor", "chrome,modal,titlebar,resizable,centerscreen", args);
 
   if ("refresh" in args && args.refresh) {
     // reset search if edit was okay (name change might lead to hidden entry!)
     document.getElementById("searchBox").value = "";
-    rebuildFilterList(gCurrentFilterList);
+    rebuildFilterList();
   }
 }
 
 function onNewFilter(emailAddress)
 {
   let list = document.getElementById("filterList");
   let filterNodes = list.childNodes;
   let selectedFilter = currentFilter();
-  // if no filter is selected use the first position, starting at 1
-  let position = 1;
+  // If no filter is selected use the first position.
+  let position = 0;
   if (selectedFilter) {
     // Get the position in the unfiltered list.
     // - this is where the new filter should be inserted!
-    rebuildFilterList(gCurrentFilterList);
-
-    // The filterNodes[0] item is the list header, skip it.
-    for (let i = 1; i < filterNodes.length; i++) {
-      if (filterNodes[i]._filter == selectedFilter) {
+    let filterCount = gCurrentFilterList.filterCount;
+    for (let i = 0; i < filterCount; i++) {
+      if (gCurrentFilterList.getFilterAt(i) == selectedFilter) {
         position = i;
         break;
       }
     }
   }
-  // The returned position is offset by 1 (due to the list header)
-  // compared to filter indexes in gCurrentFilterList.
-  let args = {filterList: gCurrentFilterList, filterPosition: position - 1};
+
+  let args = {filterList: gCurrentFilterList, filterPosition: position};
 
   window.openDialog("chrome://messenger/content/FilterEditor.xul", "FilterEditor", "chrome,modal,titlebar,resizable,centerscreen", args);
 
   if ("refresh" in args && args.refresh) {
     // On success: reset the search box!
     document.getElementById("searchBox").value = "";
-    rebuildFilterList(gCurrentFilterList);
+    rebuildFilterList();
 
     // Select the new filter, it is at the position of previous selection.
-    list.clearSelection();
-    list.addItemToSelection(list.childNodes[position]);
-    updateViewPosition(position);
-    updateCountBox();
+    list.selectItem(list.getItemAtIndex(position));
   }
-  else {
-    // If no filter created, let's search again.
-    onFindFilter(false);
-  }
-
 }
 
 /**
  * Delete selected filters.
  *  'Selected' is not to be confused with active (checkbox checked)
  */
 function onDeleteFilter()
 {
@@ -312,25 +299,25 @@ function onDeleteFilter()
     newSelection = null;
 
   // Must reverse the loop, as the items list shrinks when we delete.
   for (let index = items.length - 1; index >= 0; --index) {
     let item = items[index];
     gCurrentFilterList.removeFilter(item._filter);
     document.getElementById("filterList").removeChild(item);
   }
+  updateCountBox();
 
   // Select filter above previously selected if one existed, otherwise the first one.
   if (!newSelection && list.itemCount)
     newSelection = list.childNodes[1];
   if (newSelection) {
     list.addItemToSelection(newSelection);
     updateViewPosition(-1);
   }
-  updateCountBox();
 }
 
 /**
  * Move filter one step up in visible list.
  */
 function onUp(event) {
   moveFilter(msgMoveMotion.Up);
 }
@@ -381,76 +368,65 @@ function moveFilter(motion) {
   var relativeStep = 0;
   var moveFilterNative = null;
 
   switch(motion) {
     case msgMoveMotion.Top:
       if (activeFilter) {
         gCurrentFilterList.removeFilter(activeFilter);
         gCurrentFilterList.insertFilterAt(0, activeFilter);
-        rebuildFilterList(gCurrentFilterList);
-        onFindFilter(false); // re-filter list
+        rebuildFilterList();
         document.getElementById("reorderTopButton").disabled = true;
       }
       return;
     case msgMoveMotion.Bottom:
       if (activeFilter) {
         gCurrentFilterList.removeFilter(activeFilter);
         gCurrentFilterList.insertFilterAt(gCurrentFilterList.filterCount, activeFilter);
-        rebuildFilterList(gCurrentFilterList);
-        onFindFilter(false);
+        rebuildFilterList();
         document.getElementById("reorderBottomButton").disabled = true;
       }
       return;
     case msgMoveMotion.Up:
       relativeStep = -1;
       moveFilterNative = Components.interfaces.nsMsgFilterMotion.up;
       break;
     case msgMoveMotion.Down:
       relativeStep = +1;
       moveFilterNative = Components.interfaces.nsMsgFilterMotion.down;
       break;
   }
 
-  var searchBox = document.getElementById("searchBox");
-  if (searchBox.value) {
-    if (activeFilter) {
-      let nextIndex = list.selectedIndex + relativeStep;
-      let nextFilter = list.getItemAtIndex(nextIndex)._filter;
-      rebuildFilterList(gCurrentFilterList);
-
-      // assumption: item stays selected even after removing the search condition
-      let newIndex = list.selectedIndex + relativeStep;
-      gCurrentFilterList.removeFilter(activeFilter);
-
-      // insert after/before next visible item
-      switch(motion) {
-        case msgMoveMotion.Up:
-          // go up from selected index until finding the correct filter name
-          while (nextFilter.filterName != list.getItemAtIndex(newIndex)._filter.filterName && nextIndex < list.itemCount)
-            newIndex--;
-          break;
-        case msgMoveMotion.Down:
-          // go down from selected index until finding the correct filter name
-          while (nextFilter.filterName != list.getItemAtIndex(newIndex)._filter.filterName && nextIndex < list.itemCount)
-            newIndex++;
-          break;
-        case msgMoveMotion.Top: break; // obsolete, dealt with above
-        case msgMoveMotion.Bottom: break; // obsolete, dealt with above
-      }
-      gCurrentFilterList.insertFilterAt(newIndex, activeFilter);
-      rebuildFilterList(gCurrentFilterList);
-      list.selectedIndex = newIndex;
-    }
-    onFindFilter(false);
-  }
-  else {
+  if (!document.getElementById("searchBox").value) {
     // use legacy move filter code: up, down; only if searchBox is empty
     moveCurrentFilter(moveFilterNative);
+    return;
   }
+
+  let nextIndex = list.selectedIndex + relativeStep;
+  let nextFilter = list.getItemAtIndex(nextIndex)._filter;
+
+  gCurrentFilterList.removeFilter(activeFilter);
+
+  // Find the index of the filter we want to insert at.
+  let newIndex = -1;
+  let filterCount = gCurrentFilterList.filterCount;
+  for (let i = 0; i < filterCount; i++) {
+    if (gCurrentFilterList.getFilterAt(i) == nextFilter) {
+      newIndex = i;
+      break;
+    }
+  }
+
+  if (motion == msgMoveMotion.Down)
+    newIndex += relativeStep;
+
+  gCurrentFilterList.insertFilterAt(newIndex, activeFilter);
+
+  rebuildFilterList();
 }
 
 function viewLog()
 {
   var args = {filterList: gCurrentFilterList};
 
   window.openDialog("chrome://messenger/content/viewLog.xul", "FilterLog", "chrome,modal,titlebar,resizable,centerscreen", args);
 }
@@ -515,75 +491,127 @@ function runSelectedFilters()
     filterList.insertFilterAt(index++, item._filter);
   }
 
   filterService.applyFiltersToFolders(filterList, folders, gFilterListMsgWindow);
 }
 
 function moveCurrentFilter(motion)
 {
-    var filter = currentFilter();
-    if (!filter)
-      return;
+  let filter = currentFilter();
+  if (!filter)
+    return;
 
-    gCurrentFilterList.moveFilter(filter, motion);
-    rebuildFilterList(gCurrentFilterList);
+  gCurrentFilterList.moveFilter(filter, motion);
+  rebuildFilterList();
 }
 
-function rebuildFilterList(aFilterList)
+function rebuildFilterList()
 {
-  gCurrentFilterList = aFilterList;
-  var list = document.getElementById("filterList");
+  // This function should perform very fast even in case of high number of filters.
+  // Therefore there are some optimisations (e.g. listelement.children[] instead of
+  // list.getItemAtIndex()), that favour speed vs. semantical perfection.
+  let aTempFilterList = onFindFilter();
+
+  let searchBox = document.getElementById("searchBox");
+  let searchBoxFocus = false;
+  let activeElement = document.activeElement;
 
+  // Find if the currently focused element is a child inside the searchBox
+  // (probably html:input). Traverse up the parents until the first element
+  // with an ID is found. If it is not searchBox, return false.
+  while (activeElement != null) {
+    if (activeElement == searchBox) {
+      searchBoxFocus = true;
+      break;
+    }
+    else if (activeElement.id) {
+      searchBoxFocus = false;
+      break;
+    }
+    activeElement = activeElement.parentNode;
+  }
+
+  let list = document.getElementById("filterList");
   // Make a note of which filters were previously selected
-  var selectedNames = [];
-  for (var i = 0; i < list.selectedItems.length; i++)
+  let selectedNames = [];
+  for (let i = 0; i < list.selectedItems.length; i++)
     selectedNames.push(list.selectedItems[i]._filter.filterName);
 
   // Save scroll position so we can try to restore it later.
   // Doesn't work when the list is rebuilt after search box condition changed.
   let firstVisibleRowIndex = list.getIndexOfFirstVisibleRow();
 
-  // Remove any existing child nodes, but not our headers
-  for (var i = list.childNodes.length - 1; i > 0; i--) {
-    list.removeChild(list.childNodes[i]);
-  }
-
   // listbox.xml seems to cache the value of the first selected item in a
   // range at _selectionStart. The old value though is now obsolete,
   // since we will recreate all of the elements. We need to clear this,
   // and one way to do this is with a call to clearSelection. This might be
   // ugly from an accessibility perspective, since it fires an onSelect event.
   list.clearSelection();
 
-  for (i = 0; i < aFilterList.filterCount; i++) {
-    var filter = aFilterList.getFilterAt(i);
-    var listitem = document.createElement("listitem");
-    var nameCell = document.createElement("listcell");
+  let listitem, nameCell, enabledCell, filter;
+  let filterCount = gCurrentFilterList.filterCount;
+  let listitemCount = list.getRowCount();
+  let listitemIndex = 0;
+  let tempFilterListLength = aTempFilterList ? aTempFilterList.length - 1 : 0;
+  for (let i = 0; i < filterCount; i++) {
+    if (aTempFilterList && listitemIndex > tempFilterListLength)
+      break;
+
+    filter = gCurrentFilterList.getFilterAt(i);
+    if (aTempFilterList && aTempFilterList[listitemIndex] != i)
+      continue;
+
+    if (listitemCount > listitemIndex) {
+      // If there is a free existing listitem, reuse it.
+      // Use .children[] instead of .getItemAtIndex() as it is much faster.
+      listitem = list.children[listitemIndex + 1];
+      nameCell = listitem.childNodes[0];
+      enabledCell = listitem.childNodes[1];
+    }
+    else
+    {
+      // If there are not enough listitems in the list, create a new one.
+      listitem = document.createElement("listitem");
+      nameCell = document.createElement("listcell");
+      enabledCell = document.createElement("listcell");
+      enabledCell.setAttribute("class", "listcell-iconic");
+      listitem.appendChild(nameCell);
+      listitem.appendChild(enabledCell);
+      list.appendChild(listitem);
+      // We have to attach this listener to the listitem, even though we only care
+      // about clicks on the enabledCell. However, attaching to that item doesn't
+      // result in any events actually getting received.
+      listitem.addEventListener("click", onFilterClick, true);
+      listitem.addEventListener("dblclick", onFilterDoubleClick, true);
+    }
+    // Set the listitem values to represent the current filter.
     nameCell.setAttribute("label", filter.filterName);
-    var enabledCell = document.createElement("listcell");
     enabledCell.setAttribute("enabled", filter.enabled);
-    enabledCell.setAttribute("class", "listcell-iconic");
-    listitem.appendChild(nameCell);
-    listitem.appendChild(enabledCell);
-
-    // We have to attach this listener to the listitem, even though we only care
-    // about clicks on the enabledCell.  However, attaching to that item doesn't
-    // result in any events actually getting received
-    listitem.addEventListener("click", onFilterClick, true);
-
-    listitem.addEventListener("dblclick", onFilterDoubleClick, true);
     listitem._filter = filter;
-    list.appendChild(listitem);
 
     if (selectedNames.indexOf(filter.filterName) != -1)
       list.addItemToSelection(listitem);
+
+    listitemIndex++;
   }
+  // Remove any superfluous listitems, if the number of filters shrunk.
+  for (let i = listitemCount - 1; i >= listitemIndex; i--) {
+    list.removeChild(list.lastChild);
+  }
+
   updateViewPosition(firstVisibleRowIndex);
   updateCountBox();
+
+  // If before rebuilding the list the searchbox was focused, focus it again.
+  // In any other case, focus the list.
+  if (searchBoxFocus)
+    searchBox.focus();
+  else
+    list.focus();
 }
 
 function updateViewPosition(firstVisibleRowIndex)
 {
   let list = document.getElementById("filterList");
   if (firstVisibleRowIndex == -1)
     firstVisibleRowIndex = list.getIndexOfFirstVisibleRow();
 
@@ -596,17 +624,16 @@ function updateViewPosition(firstVisible
     list.ensureElementIsVisible(list.selectedItems[0]);
 
     // The current item should be the first selected item, so that keyboard
     // selection extension can work.
     list.currentItem = list.selectedItems[0];
   }
 
   updateButtons();
-  list.focus();
 }
 
 /**
  * Try to only enable buttons that make sense
  *  - moving filters is currently only enabled for single selection
  *    also movement is restricted by searchBox and current selection position
  *  - edit only for single filters
  *  - delete / run only for one or more selected filters
@@ -812,55 +839,41 @@ function getFirstFolder(msgFolder)
   }
   return msgFolder;
 }
 
 
 
 /**
  * Called when the search button is clicked, this will narrow down the amount
- * of filters displayed in the list, using the search term to filter the names
- *
- * @param focusSearchBox  if called from the button click event, return to searchbox
+ * of filters displayed in the list, using the search term to filter the names.
  */
-function onFindFilter(focusSearchBox)
+function onFindFilter()
 {
   let searchBox = document.getElementById("searchBox");
-  let filterList = document.getElementById("filterList");
   let keyWord = searchBox.value.toLocaleLowerCase();
 
-  // simplest case: if filter was added or removed and searchbox is empty
-  if (!keyWord && !focusSearchBox) {
-    updateCountBox();
-    return;
-  }
-  rebuildFilterList(gCurrentFilterList); // creates the unfiltered list
-  if (!keyWord) {
-    if (focusSearchBox)
-      searchBox.focus();
-    updateCountBox();
-    return;
+  // If searchbox is empty, just return and let rebuildFilterList
+  // create an unfiltered list.
+  if (!keyWord)
+    return null;
+
+  // Rematch everything in the list, remove what doesn't match the search box.
+  let rows = gCurrentFilterList.filterCount;
+  let matchingFilterList = [];
+  let item;
+  // Use the full gCurrentFilterList, not the filterList listbox,
+  // which may already be filtered.
+  for (let i = 0; i < rows; i++) {
+    item = gCurrentFilterList.getFilterAt(i).filterName;
+    if (item.toLocaleLowerCase().indexOf(keyWord) != -1)
+      matchingFilterList.push(i);
   }
 
-  // rematch everything in the list, remove what doesn't match the search box
-  let rows = filterList.getRowCount();
-
-  for(let i = rows - 1; i >= 0; i--) {
-    let matched = true;
-    let item = filterList.getItemAtIndex(i);
-    let title = item.firstChild.getAttribute("label");
-    if (title.toLocaleLowerCase().indexOf(keyWord) == -1)
-    {
-      matched = false;
-      filterList.removeChild(item);
-    }
-  }
-  updateCountBox();
-  if (focusSearchBox)
-    searchBox.focus();
+  return matchingFilterList;
 }
 
 /**
  * Display "1 item",  "11 items" or "4 of 10" if list is filtered via search box.
  */
 function updateCountBox()
 {
   let countBox = document.getElementById("countBox");
@@ -871,9 +884,8 @@ function updateCountBox()
   let bundle = document.getElementById("bundle_filter");
 
   if (len == sum) // "N items"
     countBox.value = PluralForm.get(len, bundle.getString("filterCountItems"))
                                .replace("#1",[len]);
   else // "N of M"
     countBox.value = bundle.getFormattedString("filterCountVisibleOfTotal", [len, sum]);
 }
-
--- a/mail/base/content/FilterListDialog.xul
+++ b/mail/base/content/FilterListDialog.xul
@@ -46,17 +46,17 @@
               ServerType="none"
               oncommand="onFilterFolderClick(event.target._folder);">
       <menupopup id="serverMenuPopup" type="folder" mode="filters"
                  expandFolders="nntp" headlabels="&choosethisnewsserver.label;"/>
     </menulist>
     <textbox id="searchBox"
              flex="1"
              type="search"
-             oncommand="onFindFilter(true);"
+             oncommand="rebuildFilterList();"
              emptytext="&searchBox.emptyText;"
              isempty="true"/>
   </hbox>
 
   <grid flex="1">
     <columns>
       <column flex="1"/>
       <column/>