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

Issue 29370970: [adblockpluschrome] Issue 3596 - Added support for CSS property filters to devtools panel (Closed)

Created:
Jan. 10, 2017, 10:54 a.m. by wspee
Modified:
Feb. 23, 2017, 3:57 p.m.
Reviewers:
Sebastian Noack
CC:
kzar
Visibility:
Public.

Description

Issue 3596 - Added support for CSS property filters to devtools panel

Patch Set 1 #

Total comments: 24

Patch Set 2 : No longer use ElementHidingTracer to log ElemHideEmulationFilter in the devpanel #

Total comments: 4

Patch Set 3 : Made first approach work "properly" #

Patch Set 4 : Use map instead of object #

Patch Set 5 : Changed map to two arrays #

Total comments: 7

Patch Set 6 : Use Array of Arrays instead of Map in ElementHidingTracer #

Patch Set 7 : Use two Arrays insteaf of Array of Arrays in ElemHidingTracer #

Total comments: 8

Patch Set 8 : Moved ElementHidindTracer related logic into ElementHidindTracer and addressed review comments #

Total comments: 21

Patch Set 9 : renamed addFilters to addSelectors and moved empty filters initialization to the top #

Patch Set 10 : Removed start logic and made filter loading asynchronous again #

Total comments: 10

Patch Set 11 : Removed obsolete ElemHideEmulation.load and addressed review comments #

Total comments: 4

Patch Set 12 : Addressed review comments #

Total comments: 3

Patch Set 13 : Rebased updated dependecies and addressed review comment #

Total comments: 6

Patch Set 14 : Removed obsolete variable and blank line #

Patch Set 15 : Addressed review comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -47 lines) Patch
M dependencies View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M include.preload.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 9 chunks +36 lines, -44 lines 0 comments Download
M lib/devtools.js View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +8 lines, -2 lines 0 comments Download

Messages

Total messages: 31
wspee
Jan. 10, 2017, 10:57 a.m. (2017-01-10 10:57:13 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#newcode197 include.preload.js:197: this.selectors = selectors || []; It seems you removed ...
Jan. 11, 2017, 4:20 p.m. (2017-01-11 16:20:41 UTC) #2
wspee
https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#newcode197 include.preload.js:197: this.selectors = selectors || []; On 2017/01/11 16:20:40, Sebastian ...
Jan. 12, 2017, 1:39 p.m. (2017-01-12 13:39:59 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js#newcode503 include.preload.js:503: if (filters) Yeah, this is more like it works ...
Jan. 12, 2017, 5:03 p.m. (2017-01-12 17:03:29 UTC) #4
wspee
Back to the first approach but I changed ElemHideEmulation [0] to allow me to test ...
Jan. 19, 2017, 4:20 p.m. (2017-01-19 16:20:15 UTC) #5
wspee
Changed filters type from object to map, see https://codereview.adblockplus.org/29372676/#msg2.
Feb. 7, 2017, noon (2017-02-07 12:00:51 UTC) #6
wspee
Changed map to two arrays, see: https://codereview.adblockplus.org/29372676/#msg10
Feb. 9, 2017, 8:40 a.m. (2017-02-09 08:40:48 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#newcode20 lib/devtools.js:20: let { On 2017/01/12 13:39:58, wspee wrote: > On ...
Feb. 9, 2017, 11:07 a.m. (2017-02-09 11:07:40 UTC) #8
wspee
https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#newcode20 lib/devtools.js:20: let { On 2017/02/09 11:07:38, Sebastian Noack wrote: > ...
Feb. 10, 2017, 10:46 a.m. (2017-02-10 10:46:47 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#newcode197 include.preload.js:197: this.filters = new Map(); On 2017/02/10 10:46:46, wspee wrote: ...
Feb. 10, 2017, 2:52 p.m. (2017-02-10 14:52:55 UTC) #10
wspee
On 2017/02/10 14:52:55, Sebastian Noack wrote: > https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js > File include.preload.js (right): > > https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#newcode197 ...
Feb. 13, 2017, 10:34 a.m. (2017-02-13 10:34:07 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#newcode211 include.preload.js:211: this.trace(); Please correct me if I'm wrong, but it ...
Feb. 13, 2017, 3:09 p.m. (2017-02-13 15:09:17 UTC) #12
wspee
https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#newcode211 include.preload.js:211: this.trace(); On 2017/02/13 15:09:17, Sebastian Noack wrote: > Please ...
Feb. 14, 2017, 9:28 a.m. (2017-02-14 09:28:41 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#newcode208 include.preload.js:208: start: function() Do we even need the start method ...
Feb. 14, 2017, 10:58 a.m. (2017-02-14 10:58:16 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#oldcode232 include.preload.js:232: matchedSelectors.push(selector); Renaming this variable to matchedFilters seems inappropriate, since ...
Feb. 14, 2017, 11:27 a.m. (2017-02-14 11:27:58 UTC) #15
wspee
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#oldcode232 include.preload.js:232: matchedSelectors.push(selector); On 2017/02/14 11:27:58, Sebastian Noack wrote: > Renaming ...
Feb. 15, 2017, 2:09 p.m. (2017-02-15 14:09:57 UTC) #16
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#oldcode232 include.preload.js:232: matchedSelectors.push(selector); On 2017/02/15 14:09:57, wspee wrote: > On 2017/02/14 ...
Feb. 15, 2017, 6:49 p.m. (2017-02-15 18:49:04 UTC) #17
wspee
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#oldcode232 include.preload.js:232: matchedSelectors.push(selector); On 2017/02/15 18:49:03, Sebastian Noack wrote: > On ...
Feb. 17, 2017, 10:51 a.m. (2017-02-17 10:51:39 UTC) #18
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#oldcode232 include.preload.js:232: matchedSelectors.push(selector); On 2017/02/17 10:51:38, wspee wrote: > On 2017/02/15 ...
Feb. 17, 2017, 1:55 p.m. (2017-02-17 13:55:32 UTC) #19
wspee
https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#newcode212 include.preload.js:212: addSelectors: function(selectors, filters) On 2017/02/17 13:55:31, Sebastian Noack wrote: ...
Feb. 18, 2017, 11:20 a.m. (2017-02-18 11:20:24 UTC) #20
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#oldcode232 include.preload.js:232: matchedSelectors.push(selector); On 2017/02/17 13:55:30, Sebastian Noack wrote: > On ...
Feb. 18, 2017, 11:41 a.m. (2017-02-18 11:41:19 UTC) #21
wspee
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#oldcode232 include.preload.js:232: matchedSelectors.push(selector); On 2017/02/18 11:41:18, Sebastian Noack wrote: > On ...
Feb. 18, 2017, 1 p.m. (2017-02-18 13:00:15 UTC) #22
Sebastian Noack
The changes basically look good to me. But two more things (besides that nit): 1. ...
Feb. 18, 2017, 1:11 p.m. (2017-02-18 13:11:02 UTC) #23
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js#newcode215 include.preload.js:215: filters = new Array(selectors.length); In the end, it turns ...
Feb. 20, 2017, 12:19 p.m. (2017-02-20 12:19:55 UTC) #24
wspee
https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js#newcode215 include.preload.js:215: filters = new Array(selectors.length); On 2017/02/20 12:19:55, Sebastian Noack ...
Feb. 23, 2017, 10:29 a.m. (2017-02-23 10:29:39 UTC) #25
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#oldcode570 include.preload.js:570: Nit: I'd remove the blank line here (as it ...
Feb. 23, 2017, 11:08 a.m. (2017-02-23 11:08:23 UTC) #26
wspee
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#oldcode570 include.preload.js:570: On 2017/02/23 11:08:22, Sebastian Noack wrote: > Nit: I'd ...
Feb. 23, 2017, 11:20 a.m. (2017-02-23 11:20:21 UTC) #27
Sebastian Noack
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#newcode566 include.preload.js:566: this.addSelectors(response.selectors, response.selectors); I wonder whether it would be nicer ...
Feb. 23, 2017, 11:24 a.m. (2017-02-23 11:24:03 UTC) #28
Sebastian Noack
LGTM. I didn't see that you already uploaded a new patch. But let me know ...
Feb. 23, 2017, 11:27 a.m. (2017-02-23 11:27:59 UTC) #29
wspee
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#newcode566 include.preload.js:566: this.addSelectors(response.selectors, response.selectors); On 2017/02/23 11:24:03, Sebastian Noack wrote: > ...
Feb. 23, 2017, 2:24 p.m. (2017-02-23 14:24:49 UTC) #30
Sebastian Noack
Feb. 23, 2017, 2:35 p.m. (2017-02-23 14:35:30 UTC) #31
Still LGTM. Could you please send me the patch?

Powered by Google App Engine
This is Rietveld