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

Issue 29317059: Issue 2395 - Added content script for CSS property filters (Closed)

Created:
June 18, 2015, 6:44 a.m. by Sebastian Noack
Modified:
Nov. 30, 2015, 3:32 p.m.
Reviewers:
kzar, Wladimir Palant
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue 2395 - Added content script for CSS property filters

Patch Set 1 #

Total comments: 8

Patch Set 2 : Only init when runing in window context #

Patch Set 3 : Turned namespace into a class with configurable window object #

Patch Set 4 : RegExp apparently is on the global scope #

Total comments: 8

Patch Set 5 : Added missing this. #

Total comments: 3

Patch Set 6 : Make function to add CSS selectors configurable in the constructor #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -0 lines) Patch
A chrome/content/cssProperties.js View 1 2 3 4 5 1 chunk +88 lines, -0 lines 0 comments Download

Messages

Total messages: 20
Sebastian Noack
This code is still untested! So feel free to ignore the review for now. I ...
June 18, 2015, 6:46 a.m. (2015-06-18 06:46:59 UTC) #1
Felix Dahlke
Looks pretty good to me FWIW, particularly considering that this will handle dynamically added style ...
June 18, 2015, 9:21 p.m. (2015-06-18 21:21:55 UTC) #2
Felix Dahlke
We can move me to CC here now. Wladimir and/or Dave make more sense as ...
Oct. 22, 2015, 3:30 p.m. (2015-10-22 15:30:16 UTC) #3
Wladimir Palant
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js#newcode35 chrome/content/cssProperties.js:35: regexp = pattern.regexp = new RegExp(regexp); What about invalid ...
Oct. 23, 2015, 6:36 p.m. (2015-10-23 18:36:37 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js#newcode35 chrome/content/cssProperties.js:35: regexp = pattern.regexp = new RegExp(regexp); On 2015/10/23 18:36:37, ...
Oct. 23, 2015, 8:49 p.m. (2015-10-23 20:49:31 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js#newcode88 chrome/content/cssProperties.js:88: CSSPropertyFilters.init(); On 2015/10/23 20:49:31, Sebastian Noack wrote: > Do ...
Oct. 23, 2015, 9:04 p.m. (2015-10-23 21:04:40 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssProperties.js#newcode88 chrome/content/cssProperties.js:88: CSSPropertyFilters.init(); On 2015/10/23 21:04:40, Wladimir Palant wrote: > On ...
Oct. 23, 2015, 9:23 p.m. (2015-10-23 21:23:22 UTC) #7
Sebastian Noack
As discussed on IRC, RegExp is available on the the global scope.
Oct. 23, 2015, 9:59 p.m. (2015-10-23 21:59:52 UTC) #8
Wladimir Palant
Looks ok, we'll hopefully get a chance to test this soon.
Oct. 23, 2015, 11:16 p.m. (2015-10-23 23:16:29 UTC) #9
kzar
(I'm having a go at integrating this, but so far I'm just trying to get ...
Oct. 24, 2015, 9:24 a.m. (2015-10-24 09:24:29 UTC) #10
kzar
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js#newcode78 chrome/content/cssProperties.js:78: apply: function() Perhaps apply is a bad name for ...
Oct. 24, 2015, 10:57 a.m. (2015-10-24 10:57:09 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js#newcode53 chrome/content/cssProperties.js:53: addElemHideSelectors(selectors); On 2015/10/24 09:24:29, kzar wrote: > Where does ...
Oct. 24, 2015, 6:31 p.m. (2015-10-24 18:31:14 UTC) #12
Wladimir Palant
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js#newcode53 chrome/content/cssProperties.js:53: addElemHideSelectors(selectors); On 2015/10/24 18:31:14, Sebastian Noack wrote: > This ...
Oct. 26, 2015, 11:59 a.m. (2015-10-26 11:59:28 UTC) #13
kzar
https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssProperties.js#newcode68 chrome/content/cssProperties.js:68: what: "cssproperties" We need to pass the domain here ...
Nov. 2, 2015, 8:30 p.m. (2015-11-02 20:30:57 UTC) #14
Wladimir Palant
https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssProperties.js#newcode68 chrome/content/cssProperties.js:68: what: "cssproperties" On 2015/11/02 20:30:56, kzar wrote: > We ...
Nov. 5, 2015, 1:10 p.m. (2015-11-05 13:10:04 UTC) #15
kzar
https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssProperties.js#newcode68 chrome/content/cssProperties.js:68: what: "cssproperties" On 2015/11/05 13:10:04, Wladimir Palant wrote: > ...
Nov. 5, 2015, 2:35 p.m. (2015-11-05 14:35:32 UTC) #16
Sebastian Noack
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssProperties.js#newcode53 chrome/content/cssProperties.js:53: addElemHideSelectors(selectors); On 2015/10/26 11:59:27, Wladimir Palant wrote: > On ...
Nov. 13, 2015, 1:16 a.m. (2015-11-13 01:16:30 UTC) #17
kzar
LGTM
Nov. 18, 2015, 4:40 p.m. (2015-11-18 16:40:25 UTC) #18
Wladimir Palant
LGTM assuming it was tested now.
Nov. 30, 2015, 3:29 p.m. (2015-11-30 15:29:09 UTC) #19
Sebastian Noack
Nov. 30, 2015, 3:32 p.m. (2015-11-30 15:32:52 UTC) #20
Message was sent while issue was closed.
Dave tested the content script with his integration changes in Chrome. However,
even if we missed something, it shouldn't break anything yet, as it's just a
file not being used anywhere for now.

Powered by Google App Engine
This is Rietveld