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. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 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 #MessagesTotal messages: 20
This code is still untested! So feel free to ignore the review for now. I just uploaded it because Felix were curious, and I also felt kinda scary not having a backup of that script anyway.
Looks pretty good to me FWIW, particularly considering that this will handle dynamically added style and link tags.
We can move me to CC here now. Wladimir and/or Dave make more sense as reviewers.
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:35: regexp = pattern.regexp = new RegExp(regexp); What about invalid regexps? new RegExp() can throw. https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:78: document.addEventListener("load", this.onLoad, true); Are you sure that the "load" event is being triggered for stylesheets while the page is still loading? https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:88: CSSPropertyFilters.init(); As I said earlier, CSSPropertyFilters should ideally be a class. So this should be: if (typeof window == "object") new CSSPropertyFilters(window); In non-window context (Firefox) the class will be instantiated by outside code.
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:35: regexp = pattern.regexp = new RegExp(regexp); On 2015/10/23 18:36:37, Wladimir Palant wrote: > What about invalid regexps? new RegExp() can throw. The regular expression is generated by our code. See CSSPropertyFilter.regexpString in lib/filterClasses.js https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:78: document.addEventListener("load", this.onLoad, true); On 2015/10/23 18:36:36, Wladimir Palant wrote: > Are you sure that the "load" event is being triggered for stylesheets while the > page is still loading? Seems so. Tested using https://pastebin.mozilla.org/8850267 on Chrome 46 and Firefox 38. https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:88: CSSPropertyFilters.init(); On 2015/10/23 18:36:37, Wladimir Palant wrote: > As I said earlier, CSSPropertyFilters should ideally be a class. So this should > be: > > if (typeof window == "object") > new CSSPropertyFilters(window); > > In non-window context (Firefox) the class will be instantiated by outside code. I've added the check. Do we have to use a constructor? It's not that passing around instances seems to make sense. But maybe I miss something.
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:88: CSSPropertyFilters.init(); On 2015/10/23 20:49:31, Sebastian Noack wrote: > Do we have to use a constructor? It's not that passing > around instances seems to make sense. But maybe I miss something. In Firefox there will be more than one window/document handled in the same scope. So - yes, having one instance per window should be the simplest way.
https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29317060/chrome/content/cssP... chrome/content/cssProperties.js:88: CSSPropertyFilters.init(); On 2015/10/23 21:04:40, Wladimir Palant wrote: > On 2015/10/23 20:49:31, Sebastian Noack wrote: > > Do we have to use a constructor? It's not that passing > > around instances seems to make sense. But maybe I miss something. > > In Firefox there will be more than one window/document handled in the same > scope. So - yes, having one instance per window should be the simplest way. Gotya.
As discussed on IRC, RegExp is available on the the global scope.
Looks ok, we'll hopefully get a chance to test this soon.
(I'm having a go at integrating this, but so far I'm just trying to get my head around how all the parts are supposed to fit together.) https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:53: addElemHideSelectors(selectors); Where does this function come from, I can't find it. https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:80: if (patterns.length > 0) Should be this.patterns?
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:78: apply: function() Perhaps apply is a bad name for this function, could be confused with Function.prototype.apply() ?
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:53: addElemHideSelectors(selectors); On 2015/10/24 09:24:29, kzar wrote: > Where does this function come from, I can't find it. This function is supposed to be provided by include.preload.js when integrating this script for Chrome. Hindsight, I wonder whether it would be better to pass a function to the CSSPropertyFilters constructor instead. So we wouldn't call new CSSPropertyFilters(window) at the bottom of this file but make include.preload.js call the initializing passing a function to add selectors. I don't have a strong opinion though. @Wladimir: What do you think? https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:78: apply: function() On 2015/10/24 10:57:09, kzar wrote: > Perhaps apply is a bad name for this function, could be confused with > Function.prototype.apply() ? I don't see any issues with that. It's a different context here. But do you have any better name? https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:80: if (patterns.length > 0) On 2015/10/24 09:24:29, kzar wrote: > Should be this.patterns? You seem to be right. Fixed. As I said in the first comment on this review, this code couldn't been tested yet.
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:53: addElemHideSelectors(selectors); On 2015/10/24 18:31:14, Sebastian Noack wrote: > This function is supposed to be provided by include.preload.js when integrating > this script for Chrome. Hindsight, I wonder whether it would be better to pass a > function to the CSSPropertyFilters constructor instead. > > So we wouldn't call new CSSPropertyFilters(window) at the bottom of this file > but make include.preload.js call the initializing passing a function to add > selectors. Sounds like a good idea, would definitely make this cleaner.
https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssP... chrome/content/cssProperties.js:68: what: "cssproperties" We need to pass the domain here as well, according to #2396
https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssP... chrome/content/cssProperties.js:68: what: "cssproperties" On 2015/11/02 20:30:56, kzar wrote: > We need to pass the domain here as well, according to #2396 Actually, I don't think we do - sender.frame.url should do.
https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329361/chrome/content/cssP... chrome/content/cssProperties.js:68: what: "cssproperties" On 2015/11/05 13:10:04, Wladimir Palant wrote: > On 2015/11/02 20:30:56, kzar wrote: > > We need to pass the domain here as well, according to #2396 > > Actually, I don't think we do - sender.frame.url should do. OK I've updated the description for #2396 accordingly.
https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... File chrome/content/cssProperties.js (right): https://codereview.adblockplus.org/29317059/diff/29329359/chrome/content/cssP... chrome/content/cssProperties.js:53: addElemHideSelectors(selectors); On 2015/10/26 11:59:27, Wladimir Palant wrote: > On 2015/10/24 18:31:14, Sebastian Noack wrote: > > This function is supposed to be provided by include.preload.js when > integrating > > this script for Chrome. Hindsight, I wonder whether it would be better to pass > a > > function to the CSSPropertyFilters constructor instead. > > > > So we wouldn't call new CSSPropertyFilters(window) at the bottom of this file > > but make include.preload.js call the initializing passing a function to add > > selectors. > > Sounds like a good idea, would definitely make this cleaner. Done.
LGTM
LGTM assuming it was tested now.
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. 
|
