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

options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 502 matching lines @@
       if (whitelistedDomainRegexp.test(filter.text))
         removeFromListBox("excludedDomainsBox", RegExp.$1);
       else
         removeFromListBox("userFiltersBox", filter.text);
       break;
   }
 }
 
 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.
 function appendToListBox(boxId, text)
 {
   // Note: document.createElement("option") is unreliable in Opera
   var elt = new Option();
   elt.text = text;
   elt.value = text;
   document.getElementById(boxId).appendChild(elt);
 }
 
 // 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)
 {
   event.preventDefault();
 
   var domain = document.getElementById("newWhitelistDomain").value.replace(/\s/g, "");
   document.getElementById("newWhitelistDomain").value = "";
   if (!domain)
     return;
@@ skipping 30 matching lines @@
     return;
 
   for (var i = 0; i < remove.length; i++)
     removeFilter("@@||" + remove[i] + "^$document");
 }
 
 // Removes all currently selected filters
 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
 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
 function importRawFiltersText()
 {
   var text = document.getElementById("rawFiltersText").value;
 
   importRawFilters(text, true, function(errors)
   {
     if (errors.length > 0)
@@ skipping 105 matching lines @@
       onFilterMessage(message.action, message.args[0]);
       break;
     case "prefs.respond":
       onPrefMessage(message.action, message.args[0]);
       break;
     case "subscriptions.respond":
       onSubscriptionMessage(message.action, message.args[0]);
       break;
   }
 });