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

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

[kzar, 2016-12-20 19:46:47]

Patch Set 1

[Sebastian Noack, 2016-12-20 21:38:19]

Nice! It also simplifies the code a bit. I just hope that we don't regress with
the new new options page. The current/outdated implementation of the new options
page seems to not consider performance in this scenario at all. :(

https://codereview.adblockplus.org/29369459/diff/29369460/options.js
File options.js (right):

https://codereview.adblockplus.org/29369459/diff/29369460/options.js#newcode523
options.js:523: document.getElementById(id).innerHTML = "";
IMO, this isn't worth a function anymore, and should rather be inlined.

https://codereview.adblockplus.org/29369459/diff/29369460/options.js#newcode539
options.js:539: let list = document.getElementById(boxId);
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.

https://codereview.adblockplus.org/29369459/diff/29369460/options.js#newcode608
options.js:608: text += option.value + "\n";
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:02]

Patch Set 2 : Improve raw filter box toggle performance

https://codereview.adblockplus.org/29369459/diff/29369460/options.js
File options.js (right):

https://codereview.adblockplus.org/29369459/diff/29369460/options.js#newcode523
options.js:523: document.getElementById(id).innerHTML = "";
On 2016/12/20 21:38:18, Sebastian Noack wrote:
> IMO, this isn't worth a function anymore, and should rather be inlined.

Done.

https://codereview.adblockplus.org/29369459/diff/29369460/options.js#newcode539
options.js:539: let list = document.getElementById(boxId);
On 2016/12/20 21:38:18, Sebastian Noack wrote:
> 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.

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.

https://codereview.adblockplus.org/29369459/diff/29369460/options.js#newcode608
options.js:608: text += option.value + "\n";
On 2016/12/20 21:38:18, Sebastian Noack wrote:
> 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.

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.

https://codereview.adblockplus.org/29369459/diff/29369462/options.js
File options.js (right):

https://codereview.adblockplus.org/29369459/diff/29369462/options.js#newcode602
options.js:602: rawFilters.style.display = "table-row";
I checked what jQuery was doing when the toggle method was called, it gave the
display of "table-row" which is why I've done that here.

[kzar, 2016-12-21 15:17:32]

Patch Set 3 : Make sure the raw filters text area displays first time

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

LGTM

https://codereview.adblockplus.org/29369459/diff/29369460/options.js
File options.js (right):

https://codereview.adblockplus.org/29369459/diff/29369460/options.js#newcode539
options.js:539: let list = document.getElementById(boxId);
On 2016/12/21 10:44:01, kzar wrote:
> On 2016/12/20 21:38:18, Sebastian Noack wrote:
> > 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.
> 
> 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.

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?