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

Issue 29331619: Issue 2397 - Integrate CSS property filters into adblockpluschrome (Closed)

Created:
Nov. 30, 2015, 3:46 p.m. by kzar
Modified:
Dec. 12, 2015, 1:28 p.m.
Reviewers:
Sebastian Noack
CC:
Thomas Greiner, Wladimir Palant
Visibility:
Public.

Description

Issue 2397 - Integrate CSS property filters into adblockpluschrome

Patch Set 1 #

Total comments: 5

Patch Set 2 : Adjust code now that CSSPropertyFilters is a class #

Total comments: 6

Patch Set 3 : Addressed some of Sebastian's feedback #

Total comments: 3

Patch Set 4 : Include cssProperties.js for Safari too #

Patch Set 5 : Include the CSS property filter unit tests #

Patch Set 6 : Updated adblockplus dependency again #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -44 lines) Patch
M dependencies View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M include.preload.js View 1 2 2 chunks +63 lines, -39 lines 0 comments Download
M metadata.common View 1 2 3 4 3 chunks +4 lines, -1 line 0 comments Download
M metadata.safari View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 14
kzar
Patch Set 1 https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js#newcode419 include.preload.js:419: new CSSPropertyFilters(window, addElemHideSelectors); The changes to ...
Nov. 30, 2015, 3:51 p.m. (2015-11-30 15:51:11 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js#newcode361 include.preload.js:361: CSSPropertyFilters.apply(); This method doesn't exist anymore, since we turned ...
Nov. 30, 2015, 4:06 p.m. (2015-11-30 16:06:29 UTC) #2
kzar
Patch Set 2 : Adjust code now that CSSPropertyFilters is a class https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js File include.preload.js ...
Nov. 30, 2015, 5:39 p.m. (2015-11-30 17:39:07 UTC) #3
kzar
https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#newcode342 include.preload.js:342: var updateStylesheet = function(propertyFilters) I deliberately added a typo ...
Nov. 30, 2015, 5:57 p.m. (2015-11-30 17:57:32 UTC) #4
kzar
https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#newcode342 include.preload.js:342: var updateStylesheet = function(propertyFilters) On 2015/11/30 17:57:32, kzar wrote: ...
Dec. 1, 2015, 10:46 a.m. (2015-12-01 10:46:04 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#newcode377 include.preload.js:377: ext.backgroundPage.sendMessage({type: "get-selectors"}, addElemHideSelectors); Any reason we use different logic ...
Dec. 10, 2015, 10:30 a.m. (2015-12-10 10:30:49 UTC) #6
kzar
Patch Set 3 : Addressed some of Sebastian's feedback https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#newcode377 include.preload.js:377: ...
Dec. 10, 2015, 3:16 p.m. (2015-12-10 15:16:20 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js#newcode288 include.preload.js:288: var propertyFilters = new CSSPropertyFilters(window, addElemHideSelectors); Note that the ...
Dec. 10, 2015, 4:35 p.m. (2015-12-10 16:35:37 UTC) #8
kzar
Patch Set 4 : Include cssProperties.js for Safari too https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js#newcode288 include.preload.js:288: ...
Dec. 10, 2015, 7:31 p.m. (2015-12-10 19:31:06 UTC) #9
kzar
Patch Set 5 : Include the CSS property filter unit tests
Dec. 11, 2015, 2 p.m. (2015-12-11 14:00:00 UTC) #10
Sebastian Noack
LGTM, with the discussed changes to the cssProperttFilters.js content script.
Dec. 12, 2015, 12:50 a.m. (2015-12-12 00:50:02 UTC) #11
kzar
On 2015/12/12 00:50:02, Sebastian Noack wrote: > LGTM, with the discussed changes to the cssProperttFilters.js ...
Dec. 12, 2015, 9:45 a.m. (2015-12-12 09:45:14 UTC) #12
kzar
Patch Set 6 : Updated adblockplus dependency again
Dec. 12, 2015, 12:50 p.m. (2015-12-12 12:50:07 UTC) #13
Sebastian Noack
Dec. 12, 2015, 1:09 p.m. (2015-12-12 13:09:10 UTC) #14
LGTM

Powered by Google App Engine
This is Rietveld