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

Issue 29344551: Issue 4071 - Speed up ElemHide.getSelectorsForDomain for domains with no filters (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 9 months ago by kzar
Modified:
3 years, 9 months ago
Visibility:
Public.

Description

Issue 4071 - Speed up ElemHide.getSelectorsForDomain for domains with no filters

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -2 lines) Patch
M lib/elemHide.js View 6 chunks +55 lines, -2 lines 4 comments Download

Messages

Total messages: 3
kzar
Patch Set 1 Very rough so far, also not 100% convinced this idea makes sense. ...
3 years, 9 months ago (2016-05-25 17:56:53 UTC) #1
kzar
As discussed in IRC this change sped up my test from 5601ms to 2114ms with ...
3 years, 9 months ago (2016-05-25 18:10:54 UTC) #2
Sebastian Noack
3 years, 9 months ago (2016-05-26 11:46:44 UTC) #3
Message was sent while issue was closed.
https://codereview.adblockplus.org/29344551/diff/29344552/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29344551/diff/29344552/lib/elemHide.js#new...
lib/elemHide.js:289: else if (count)
It seems the condition to check for non-zero count is redundant. Then you also
don't need the count variable.

However, I wonder whether we should delete properties from
exceptionCountByDomain anyway. It seems we don't do that for filtersByDomain
either.

https://codereview.adblockplus.org/29344551/diff/29344552/lib/elemHide.js#new...
lib/elemHide.js:556: if (filter)
Just for my understanding, what is the scenario when "filter" evaluates to false
here?

https://codereview.adblockplus.org/29344551/diff/29344552/lib/elemHide.js#new...
lib/elemHide.js:557: selectors.push(filters[filterKey].selector);
Use the "filter" variable here?

https://codereview.adblockplus.org/29344551/diff/29344552/lib/elemHide.js#new...
lib/elemHide.js:562: anyExceptions = anyExceptions ||
exceptionCountByDomain[currentDomain];
I think it's preferable to write it like this:

 if (exceptionCountByDomain[currentDomain])
    anyExceptions = true;

If for nothing else, to not change the type of the "anyExceptions" variable.
Sign in to reply to this message.

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