Issue 4752 - Improve options page performance for lots of custom filters

options.js

Index: options.js
diff --git a/options.js b/options.js
index 94357cbc88c8ed9ce4572b368a8aa6ad43a3d32e..a2cd337fb93217b10ed24620eb945eaa926e17b1 100644
--- a/options.js
+++ b/options.js
@@ -520,9 +520,7 @@ function onFilterMessage(action, filter)
 
 function clearListBox(id)
 {
-  var list = document.getElementById(id);
-  while (list.lastChild)
-    list.removeChild(list.lastChild);
+  document.getElementById(id).innerHTML = "";

[Sebastian Noack, 2016/12/20 21:38:18]

IMO, this isn't worth a function anymore, and should rather be inlined.

[kzar, 2016/12/21 10:44:01]

Done.

 }
 
 // Add a filter string to the list box.
@@ -538,10 +536,10 @@ function appendToListBox(boxId, text)
 // Remove a filter string from a list box.
 function removeFromListBox(boxId, text)
 {
-  var list = document.getElementById(boxId);
-  for (var i = 0; i < list.length; i++)
-    if (list.options[i].value == text)
-      list.remove(i--);
+  let list = document.getElementById(boxId);

[Sebastian Noack, 2016/12/20 21:38:18]

That is unrelated, but I suppose we should go and replace var with let across
the repo now, with a separate patch of course. If you don't want to write a
separate issue, I wouldn't mind if that review gets dropped into the issue where
we advanced the minimum Chromium version.

[kzar, 2016/12/21 10:44:01]

Yea I considered that too. It's not as trivial as it sounds since arrow
functions bind this and let variables have different scope, so we have to be
careful to avoid regressions. I will do this when I get a chance, it is on my
list now.

[Sebastian Noack, 2016/12/21 16:13:38]

Well, so far we used arrow functions only in code that got transpiled with
jsHydra, where we also used let consistently. So theoretically, there shouldn't
be any code using var in combination with arrow functions, or do I miss
something?

+  let selector = "option[value=" + CSS.escape(text) + "]";
+  for (let option of list.querySelectorAll(selector))
+    list.removeChild(option);
 }
 
 function addWhitelistDomain(event)
@@ -592,16 +590,8 @@ function removeSelectedExcludedDomain(event)
 function removeSelectedFilters(event)
 {
   event.preventDefault();
-  var userFiltersBox = document.getElementById("userFiltersBox");
-  var remove = [];
-  for (var i = 0; i < userFiltersBox.length; i++)
-    if (userFiltersBox.options[i].selected)
-      remove.push(userFiltersBox.options[i].value);
-  if (!remove.length)
-    return;
-
-  for (var i = 0; i < remove.length; i++)
-    removeFilter(remove[i]);
+  for (let option of document.querySelectorAll("#userFiltersBox > option:checked"))
+    removeFilter(option.value);
 }
 
 // Shows raw filters box and fills it with the current user filters
@@ -609,15 +599,16 @@ function toggleFiltersInRawFormat(event)
 {
   event.preventDefault();
 
+  let text = "";
+
   $("#rawFilters").toggle();
   if ($("#rawFilters").is(":visible"))
   {
-    var userFiltersBox = document.getElementById("userFiltersBox");
-    var text = "";
-    for (var i = 0; i < userFiltersBox.length; i++)
-      text += userFiltersBox.options[i].value + "\n";
-    document.getElementById("rawFiltersText").value = text;
+    for (let option of document.getElementById("userFiltersBox").options)
+      text += option.value + "\n";

[Sebastian Noack, 2016/12/20 21:38:18]

String concatenation using a loop is potentially slow. Just pointing it out
because this is about optimizing performance in the first place. You might want
to try out whether using an array and joining it, performs better.

[kzar, 2016/12/21 10:44:01]

Done, it does perform a little better. Interestingly it turns out
`$("#rawFilters").is(":visible")` was hugely slow so I've replaced the jQuery
stuff here too.

   }
+
+  document.getElementById("rawFiltersText").value = text;
 }
 
 // Imports filters in the raw text box