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

Issue 29361668: Issue 4394 - Create a filter class for element hiding emulation filters (Closed)

Created:
Nov. 3, 2016, 3:42 p.m. by Felix Dahlke
Modified:
Nov. 22, 2016, 11 a.m.
Reviewers:
kzar, Wladimir Palant
Base URL:
https://bitbucket.org/fhd/adblockpluscore
Visibility:
Public.

Description

Issue 4394 - Create a filter class for element hiding emulation filters

Patch Set 1 #

Total comments: 36

Patch Set 2 : Address kzar's comments #

Patch Set 3 : Rebase #

Total comments: 4

Patch Set 4 : Address Wladimir's comments #

Total comments: 8

Patch Set 5 : Address kzar's comments #

Patch Set 6 : Improve compliance with the 80 column rule #

Unified diffs Side-by-side diffs Delta from patch set Stats (+168 lines, -180 lines) Patch
M chrome/content/elemHideEmulation.js View 1 2 3 4 chunks +29 lines, -9 lines 0 comments Download
A lib/common.js View 1 2 3 4 1 chunk +50 lines, -0 lines 0 comments Download
M lib/elemHideEmulation.js View 1 chunk +8 lines, -8 lines 0 comments Download
M lib/filterClasses.js View 1 2 3 4 6 chunks +14 lines, -88 lines 0 comments Download
M lib/filterListener.js View 4 chunks +7 lines, -7 lines 0 comments Download
M test/elemHideEmulation.js View 1 2 chunks +17 lines, -15 lines 0 comments Download
M test/filterClasses.js View 1 2 3 4 5 7 chunks +28 lines, -38 lines 0 comments Download
M test/filterListener.js View 4 chunks +15 lines, -15 lines 0 comments Download

Messages

Total messages: 28
Felix Dahlke
Nov. 3, 2016, 3:42 p.m. (2016-11-03 15:42:43 UTC) #1
Felix Dahlke
Nov. 3, 2016, 3:47 p.m. (2016-11-03 15:47:13 UTC) #2
Felix Dahlke
Some pointers: 1. The biggest part here is a renaming from CSS Property Filter to ...
Nov. 3, 2016, 3:58 p.m. (2016-11-03 15:58:50 UTC) #3
kzar
(Just looking through these reviews today, sorry for the delay.) On 2016/11/03 15:58:50, Felix Dahlke ...
Nov. 4, 2016, 1:20 p.m. (2016-11-04 13:20:00 UTC) #4
Felix Dahlke
On 2016/11/04 13:20:00, kzar wrote: > (Just looking through these reviews today, sorry for the ...
Nov. 4, 2016, 1:37 p.m. (2016-11-04 13:37:57 UTC) #5
kzar
https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elemHideEmulation.js#newcode134 chrome/content/elemHideEmulation.js:134: if (pattern.features != elemHideEmulationFeatureMap.PROPERTY_SELECTOR) Before I suggested adding a ...
Nov. 4, 2016, 3:45 p.m. (2016-11-04 15:45:56 UTC) #6
Felix Dahlke
I've responded to all comments, but I haven't uploaded a new patch set yet, since ...
Nov. 4, 2016, 4:43 p.m. (2016-11-04 16:43:35 UTC) #7
kzar
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode918 lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) I guess my question is ...
Nov. 7, 2016, 12:36 p.m. (2016-11-07 12:36:26 UTC) #8
Felix Dahlke
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode918 lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 12:36:26, kzar wrote: ...
Nov. 7, 2016, 2:41 p.m. (2016-11-07 14:41:25 UTC) #9
kzar
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode918 lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 14:41:24, Felix Dahlke ...
Nov. 7, 2016, 3:59 p.m. (2016-11-07 15:59:44 UTC) #10
Felix Dahlke
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode918 lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 15:59:44, kzar wrote: ...
Nov. 7, 2016, 4:51 p.m. (2016-11-07 16:51:24 UTC) #11
kzar
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode918 lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 16:51:24, Felix Dahlke ...
Nov. 7, 2016, 5:10 p.m. (2016-11-07 17:10:22 UTC) #12
kzar
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode928 lib/filterClasses.js:928: return new ElemHideEmulationFilter(text, domain, selector, On 2016/11/04 16:43:35, Felix ...
Nov. 7, 2016, 5:19 p.m. (2016-11-07 17:19:26 UTC) #13
Felix Dahlke
Some comment responses, but I guess I have enough now to work on the next ...
Nov. 8, 2016, 5:45 p.m. (2016-11-08 17:45:35 UTC) #14
Felix Dahlke
New patch set is up, all comments addressed AFAICT. Note that I haven't rebased this ...
Nov. 11, 2016, 11:51 a.m. (2016-11-11 11:51:43 UTC) #15
kzar
> Note that I haven't rebased this on top of #4593, since I > want ...
Nov. 15, 2016, 4:54 p.m. (2016-11-15 16:54:13 UTC) #16
Felix Dahlke
Rebased, I believe everything has been addressed now, except for the open discussion where we ...
Nov. 15, 2016, 9:11 p.m. (2016-11-15 21:11:45 UTC) #17
kzar
On 2016/11/15 21:11:45, Felix Dahlke wrote: > But we're moving in circles again, so let's ...
Nov. 16, 2016, 10:53 a.m. (2016-11-16 10:53:40 UTC) #18
Felix Dahlke
On 2016/11/16 10:53:40, kzar wrote: > On 2016/11/15 21:11:45, Felix Dahlke wrote: > > But ...
Nov. 16, 2016, 10:59 a.m. (2016-11-16 10:59:18 UTC) #19
kzar
On 2016/11/16 10:59:18, Felix Dahlke wrote: > but in general I find your reviews valuable ...
Nov. 16, 2016, 11:10 a.m. (2016-11-16 11:10:17 UTC) #20
Wladimir Palant
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode918 lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) I actually have to agree ...
Nov. 21, 2016, 11:39 a.m. (2016-11-21 11:39:15 UTC) #21
Felix Dahlke
New patch set is up, all comments should be addressed. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js#newcode918 ...
Nov. 21, 2016, 2:38 p.m. (2016-11-21 14:38:59 UTC) #22
kzar
https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js#newcode49 lib/common.js:49: if (typeof exports != "undefined") Nit: The braces here ...
Nov. 21, 2016, 4:24 p.m. (2016-11-21 16:24:55 UTC) #23
Felix Dahlke
Thanks Dave, new patch set is up! https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js#newcode49 lib/common.js:49: if (typeof ...
Nov. 21, 2016, 5:26 p.m. (2016-11-21 17:26:13 UTC) #24
kzar
https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js#newcode43 test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") On 2016/11/21 17:26:12, Felix Dahlke ...
Nov. 21, 2016, 7:18 p.m. (2016-11-21 19:18:23 UTC) #25
Felix Dahlke
New patch set is up. https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js#newcode43 test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") ...
Nov. 21, 2016, 8:11 p.m. (2016-11-21 20:11:48 UTC) #26
kzar
LGTM
Nov. 21, 2016, 10:35 p.m. (2016-11-21 22:35:17 UTC) #27
Wladimir Palant
Nov. 22, 2016, 8:40 a.m. (2016-11-22 08:40:37 UTC) #28
LGTM

Powered by Google App Engine
This is Rietveld