Issue 29329677: Issue 2396 - Add CSS property filter message responder (Closed)
DescriptionIssue 2396 - Add CSS property filter message responder
(This depends on the other CSS property filters code reviews that are still ongoing.)
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed feedback from Thomas #
Total comments: 2
Patch Set 3 : Ensure that the frame isn't whitelisted #
Total comments: 2
Patch Set 4 : Avoid breaking the background.html test page #
Total comments: 4
Patch Set 5 : Addressed Wladimir's feedback #Patch Set 6 : Remove specificOnly parameter from getRulesForDomain stub #MessagesTotal messages: 13
Patch Set 1
https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:198: if (message.what == "cssproperties" && "domain" in message) This condition allows the following invalid message to succeed even though it shouldn't: { what: "cssproperties", subscriptionUrl: "http://..." } https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:200: callback(CSSRules.getRulesForDomain( Please avoid forcing everything into a single statement. Splitting it up into several statements gives us not only better readability but also more useful stack traces in case of errors. https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:201: message.domain, message.specificOnly).map(function(filter) The content script doesn't know whether it's supposed to request only specific filters or not. Currently, that is being determined by the background page right before calling `ElemHide.getSelectorsByDomain()` (see "get-selectors" message type in adblockpluschrome/background.js). https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:206: regexp: filter.regexpString() `CSSPropertyFilter.prototype.regexpString` is a string, not a function.
Patch Set 2 : Addressed feedback from Thomas https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:198: if (message.what == "cssproperties" && "domain" in message) On 2015/11/03 10:57:02, Thomas Greiner wrote: > This condition allows the following invalid message to succeed even though it > shouldn't: > > { > what: "cssproperties", > subscriptionUrl: "http://..." > } Done. https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:200: callback(CSSRules.getRulesForDomain( On 2015/11/03 10:57:02, Thomas Greiner wrote: > Please avoid forcing everything into a single statement. Splitting it up into > several statements gives us not only better readability but also more useful > stack traces in case of errors. I thought it was pretty readable as it was but fair enough, Done. https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:201: message.domain, message.specificOnly).map(function(filter) On 2015/11/03 10:57:03, Thomas Greiner wrote: > The content script doesn't know whether it's supposed to request only specific > filters or not. > > Currently, that is being determined by the background page right before calling > `ElemHide.getSelectorsByDomain()` (see "get-selectors" message type in > adblockpluschrome/background.js). Done. https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:206: regexp: filter.regexpString() On 2015/11/03 10:57:03, Thomas Greiner wrote: > `CSSPropertyFilter.prototype.regexpString` is a string, not a function. Whoops good point, Done.
https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:200: callback(CSSRules.getRulesForDomain( On 2015/11/03 14:09:51, kzar wrote: > On 2015/11/03 10:57:02, Thomas Greiner wrote: > > Please avoid forcing everything into a single statement. Splitting it up into > > several statements gives us not only better readability but also more useful > > stack traces in case of errors. > > I thought it was pretty readable as it was but fair enough, Done. Thanks, I appreciate it. It would've even allowed you to break and call the callback once but that'd be more of a personal preference so I'll leave that up to you. e.g. var filters = []; if ("domain" in message) { ... filters = ... } callback(filters); break; https://codereview.adblockplus.org/29329677/diff/29329712/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329712/messageResponder.js... messageResponder.js:206: var specificOnly = require("whitelisting").isFrameWhitelisted( I noticed that in "get-selectors" (see https://hg.adblockplus.org/adblockpluschrome/file/tip/background.js#l302), it also checks whether the frame is whitelisted to avoid injecting element hiding filters into whitelisted frames. Because the issue is that content scripts are running in every frame regardless of whether it is whitelisted or not.
Patch Set 3 : Ensure that the frame isn't whitelisted https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329678/messageResponder.js... messageResponder.js:200: callback(CSSRules.getRulesForDomain( On 2015/11/03 14:26:01, Thomas Greiner wrote: > On 2015/11/03 14:09:51, kzar wrote: > > On 2015/11/03 10:57:02, Thomas Greiner wrote: > > > Please avoid forcing everything into a single statement. Splitting it up > into > > > several statements gives us not only better readability but also more useful > > > stack traces in case of errors. > > > > I thought it was pretty readable as it was but fair enough, Done. > > Thanks, I appreciate it. It would've even allowed you to break and call the > callback once but that'd be more of a personal preference so I'll leave that up > to you. > > e.g. > var filters = []; > if ("domain" in message) > { > ... > filters = ... > } > callback(filters); > break; Done. https://codereview.adblockplus.org/29329677/diff/29329712/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329712/messageResponder.js... messageResponder.js:206: var specificOnly = require("whitelisting").isFrameWhitelisted( On 2015/11/03 14:26:02, Thomas Greiner wrote: > I noticed that in "get-selectors" (see > https://hg.adblockplus.org/adblockpluschrome/file/tip/background.js#l302), it > also checks whether the frame is whitelisted to avoid injecting element hiding > filters into whitelisted frames. Because the issue is that content scripts are > running in every frame regardless of whether it is whitelisted or not. Done.
Only one more thing https://codereview.adblockplus.org/29329677/diff/29329714/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329714/messageResponder.js... messageResponder.js:28: var CSSRules = require("cssRules").CSSRules; This breaks our test environment because there's no "cssRules" mock module in adblockplusui/background.js. Similar case for `filterClasses.RegExpFilter` below.
Patch Set 4 : Avoid breaking the background.html test page https://codereview.adblockplus.org/29329677/diff/29329714/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329714/messageResponder.js... messageResponder.js:28: var CSSRules = require("cssRules").CSSRules; On 2015/11/03 16:03:44, Thomas Greiner wrote: > This breaks our test environment because there's no "cssRules" mock module in > adblockplusui/background.js. Similar case for `filterClasses.RegExpFilter` > below. Stubbing out cssRules.CSSRules.getRulesForDomain() was enough to avoid breaking the background.html page, look OK?
LGTM
https://codereview.adblockplus.org/29329677/diff/29329716/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329716/messageResponder.js... messageResponder.js:207: RegExpFilter.typeMap.ELEMHIDE)) This is going to be rather problematic in Firefox. sender.frame isn't currently being set, and the whitelisting module doesn't exist. But given the current direction of Firefox development I guess that the best solution will be to provide this functionality (or something similar). https://codereview.adblockplus.org/29329677/diff/29329716/messageResponder.js... messageResponder.js:211: ); This check makes no sense, CSS property rules are always "specific only." This definitely needs to be fixed in the container so I would just omit the specificOnly parameter here.
Patch Set 5 : Addressed Wladimir's feedback https://codereview.adblockplus.org/29329677/diff/29329716/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29329677/diff/29329716/messageResponder.js... messageResponder.js:207: RegExpFilter.typeMap.ELEMHIDE)) On 2015/11/05 13:02:13, Wladimir Palant wrote: > This is going to be rather problematic in Firefox. sender.frame isn't currently > being set, and the whitelisting module doesn't exist. But given the current > direction of Firefox development I guess that the best solution will be to > provide this functionality (or something similar). Acknowledged. https://codereview.adblockplus.org/29329677/diff/29329716/messageResponder.js... messageResponder.js:211: ); On 2015/11/05 13:02:13, Wladimir Palant wrote: > This check makes no sense, CSS property rules are always "specific only." This > definitely needs to be fixed in the container so I would just omit the > specificOnly parameter here. Done.
Patch Set 6 : Remove specificOnly parameter from getRulesForDomain stub
LGTM
LGTM 
|
