Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(974)

Issue 29369459: Issue 4752 - Improve options page performance for lots of custom filters (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by kzar
Modified:
2 years, 11 months ago
Reviewers:
Sebastian Noack
CC:
Thomas Greiner
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 7

Patch Set 2 : Improve raw filter box toggle performance #

Total comments: 1

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -30 lines) Patch
M options.js View 1 2 5 chunks +21 lines, -30 lines 0 comments Download

Messages

Total messages: 5
kzar
Patch Set 1
2 years, 11 months ago (2016-12-20 19:46:47 UTC) #1
Sebastian Noack
Nice! It also simplifies the code a bit. I just hope that we don't regress ...
2 years, 11 months ago (2016-12-20 21:38:19 UTC) #2
kzar
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 ...
2 years, 11 months ago (2016-12-21 10:44:02 UTC) #3
kzar
Patch Set 3 : Make sure the raw filters text area displays first time
2 years, 11 months ago (2016-12-21 15:17:32 UTC) #4
Sebastian Noack
2 years, 11 months ago (2016-12-21 16:13:38 UTC) #5
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5