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

Issue 29342837: Issue 4054 - Remove filters from filtersByDomain (Closed)

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

Description

Issue 4054 - Remove filters from filtersByDomain

Patch Set 1 #

Total comments: 12

Patch Set 2 : Corrected mistake in code ordering #

Total comments: 2

Patch Set 3 : Use for ... in #

Total comments: 2

Patch Set 4 : Rename domainMatches for consistency #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -3 lines) Patch
M lib/elemHide.js View 1 2 3 2 chunks +14 lines, -3 lines 0 comments Download

Messages

Total messages: 13
kzar
Patch Set 1 https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#newcode213 lib/elemHide.js:213: delete filters[filter.text]; Note: There is a ...
3 years, 10 months ago (2016-05-20 11:36:12 UTC) #1
Wladimir Palant
LGTM https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#newcode213 lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 11:36:12, kzar wrote: > ...
3 years, 10 months ago (2016-05-20 12:17:51 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#newcode208 lib/elemHide.js:208: let domains = Object.keys(filter.domains || defaultDomains); So in the ...
3 years, 10 months ago (2016-05-20 12:20:44 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#newcode213 lib/elemHide.js:213: delete filters[filter.text]; What if an element hiding filter is ...
3 years, 10 months ago (2016-05-20 12:29:25 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#newcode213 lib/elemHide.js:213: delete filters[filter.text]; On 2016/05/20 12:29:24, Sebastian Noack wrote: > ...
3 years, 10 months ago (2016-05-20 12:40:05 UTC) #5
kzar
Patch Set 2 : Corrected mistake in code ordering https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#newcode208 lib/elemHide.js:208: ...
3 years, 10 months ago (2016-05-20 12:49:29 UTC) #6
Wladimir Palant
https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js#newcode208 lib/elemHide.js:208: for (let domain of domains) So how about making ...
3 years, 10 months ago (2016-05-20 13:12:34 UTC) #7
kzar
Patch Set 3 : Use for ... in https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342838/lib/elemHide.js#newcode208 lib/elemHide.js:208: let ...
3 years, 10 months ago (2016-05-20 13:13:38 UTC) #8
kzar
https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342840/lib/elemHide.js#newcode208 lib/elemHide.js:208: for (let domain of domains) On 2016/05/20 13:12:34, Wladimir ...
3 years, 10 months ago (2016-05-20 13:14:25 UTC) #9
Sebastian Noack
LGTM
3 years, 10 months ago (2016-05-20 13:15:56 UTC) #10
Wladimir Palant
https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js#newcode207 lib/elemHide.js:207: let domains = filter.domains || defaultDomains; The same variable ...
3 years, 10 months ago (2016-05-20 13:26:07 UTC) #11
kzar
Patch Set 4 : Rename domainMatches for consistency https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342837/diff/29342842/lib/elemHide.js#newcode207 lib/elemHide.js:207: let ...
3 years, 10 months ago (2016-05-20 13:31:23 UTC) #12
Wladimir Palant
3 years, 10 months ago (2016-05-20 13:35:46 UTC) #13
LGTM
Sign in to reply to this message.

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