Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29550662: Issue 5735 - Use JS Map instead of Object for property domains of Filter objects (Closed)

Created:
Sept. 20, 2017, 12:59 p.m. by sergei
Modified:
Sept. 26, 2017, 1:58 p.m.
Reviewers:
wspee, kzar, hub, Wladimir Palant
CC:
Felix Dahlke
Base URL:
https://github.com/adblockplus/adblockpluscore.git
Visibility:
Public.

Description

# based on https://codereview.adblockplus.org/29550598/

Patch Set 1 #

Total comments: 9

Patch Set 2 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -22 lines) Patch
M lib/elemHide.js View 1 3 chunks +4 lines, -5 lines 0 comments Download
M lib/filterClasses.js View 1 7 chunks +14 lines, -15 lines 0 comments Download
M test/filterClasses.js View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 7
sergei
Sept. 20, 2017, 1:03 p.m. (2017-09-20 13:03:04 UTC) #1
kzar
When I was working on getSelectorsForDomain previously I found using Maps worked better for Chrome ...
Sept. 20, 2017, 3:02 p.m. (2017-09-20 15:02:41 UTC) #2
Wladimir Palant
From all I know, Safari is the only browser where we have to worry about ...
Sept. 21, 2017, 8:11 a.m. (2017-09-21 08:11:43 UTC) #3
sergei
https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29550662/diff/29550663/lib/elemHide.js#newcode70 lib/elemHide.js:70: defaultDomains.set("", true); On 2017/09/21 08:11:44, Wladimir Palant wrote: > ...
Sept. 21, 2017, 10:50 a.m. (2017-09-21 10:50:34 UTC) #4
sergei
On 2017/09/20 15:02:41, kzar wrote: > it would be nice if you could confirm this ...
Sept. 21, 2017, 10:52 a.m. (2017-09-21 10:52:45 UTC) #5
Wladimir Palant
LGTM https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29550662/diff/29550663/lib/filterClasses.js#newcode390 lib/filterClasses.js:390: domains.set(list[0], true); On 2017/09/21 10:50:34, sergei wrote: > ...
Sept. 21, 2017, 10:53 a.m. (2017-09-21 10:53:01 UTC) #6
kzar
Sept. 22, 2017, 10:42 a.m. (2017-09-22 10:42:57 UTC) #7
On 2017/09/21 10:52:45, sergei wrote:
> Thanks for pointing on it, I will definitely try to estimate how it affects
the
> performance but if there are no objections a bit later.

Well IMO if we're going to test the performance of the change, we might as well
do so before pushing it.

Powered by Google App Engine
This is Rietveld