Issue 29324599: Issue 2392/2393 - Created container for CSS property filters (Closed)
|
Created:
Aug. 25, 2015, 3:35 p.m. by Thomas Greiner Modified:
Nov. 30, 2015, 1:51 p.m. Visibility:
Public. |
DescriptionI combined the changes for #2392 and #2393 in this review since I implemented both side-by-side and since they're still distinguishable by file.
This review also includes a change in the signature of the `ElemHideBase.fromText` method since this appeared to have been overlooked in the past.
Patch Set 1 #
Total comments: 15
Patch Set 2 : Rebased and addressed comments #
Total comments: 2
MessagesTotal messages: 11
https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:22: let {ElemHide: {getException}} = require("elemHide"); This results in a syntax error when building the script into adblockplus.js for the Chrome extension. Changing it to `let {ElemHide} = require("elemHide");` and later `ElemHide.getException(...` fixed it for me.
https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:95: if (specificOnly && (!filter.domains || filter.domains[""])) Use filter.isGeneric() instead? https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:99: result.push(filter.selector); Mind just pushing `filter` instead of `filter.selector`? It would be useful to have access to `filter.regexpString`, `filter.selectorPrefix` and `filter.selectorSuffix` for the message responder in #2396.
https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:86: * @param {Boolean} specificOnly Aren't CSS property filters _always_ specific? We enforce that they have a domain as they're inefficient right? https://github.com/adblockplus/adblockplus/blob/master/lib/filterClasses.js#L933
https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:34: let keyByFilter = Object.create(null); The two maps filterByKey and keyByFilter seem to be completely unnecessary overhead here - unlike with usual element hiding rules, the filters are never referenced by key. If the point is merely excluding duplicate filters then a single map filters would be sufficient (mapping filter text to true). Note that actually storing the filter objects is unnecessary, these can be retrieved via Filter.fromText() when needed. https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:95: if (specificOnly && (!filter.domains || filter.domains[""])) On 2015/11/02 19:33:45, kzar wrote: > Use filter.isGeneric() instead? Actually, filters that are generic shouldn't even be added in the first place. These should be rejected by the filter parser already so that the filter type would be InvalidFilter. https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:100: } Note: This loop is rather inefficient but that's ok for now - we don't expect too many of these rules. We can make it more efficient when we make the ElemHide code more efficient (https://issues.adblockplus.org/ticket/235). https://codereview.adblockplus.org/29324599/diff/29324600/lib/filterListener.js File lib/filterListener.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/filterListener.... lib/filterListener.js:31: require("filterClasses"); Nit: While I couldn't find anything in our style guide, I think that we usually use double indentation in cases like this.
https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:95: if (specificOnly && (!filter.domains || filter.domains[""])) On 2015/11/05 12:46:19, Wladimir Palant wrote: > On 2015/11/02 19:33:45, kzar wrote: > > Use filter.isGeneric() instead? > > Actually, filters that are generic shouldn't even be added in the first place. > These should be rejected by the filter parser already so that the filter type > would be InvalidFilter. Yea, I noticed that later, see my above comment :p
In case it helps, here's a diff for what I needed to change to get things working https://gist.github.com/kzar/b7c6d83eac0be774bb7c
Note that nothing has changed in lib/filterClasses.js. The intermediate diff is caused by the rebase. I tested the changes with an adapted version of the unit tests which I will update separately in the corresponding review. https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:22: let {ElemHide: {getException}} = require("elemHide"); On 2015/10/24 17:14:51, kzar wrote: > This results in a syntax error when building the script into adblockplus.js for > the Chrome extension. Changing it to `let {ElemHide} = require("elemHide");` and > later `ElemHide.getException(...` fixed it for me. Done. Since this is valid ES6 syntax, I'd consider this to be an issue with JSHydra. However, I agree that for now I can make the change that you proposed to avoid complications. https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:34: let keyByFilter = Object.create(null); On 2015/11/05 12:46:18, Wladimir Palant wrote: > The two maps filterByKey and keyByFilter seem to be completely unnecessary > overhead here - unlike with usual element hiding rules, the filters are never > referenced by key. If the point is merely excluding duplicate filters then a > single map filters would be sufficient (mapping filter text to true). Note that > actually storing the filter objects is unnecessary, these can be retrieved via > Filter.fromText() when needed. You're absolutely right. I'd love to use `Set` for that since that's basically all the functionality that's needed, except for the `getRulesForDomain` function, of course. However, it's not supported on most platforms yet so yeah, an Object it is then. https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:86: * @param {Boolean} specificOnly On 2015/11/04 13:03:20, kzar wrote: > Aren't CSS property filters _always_ specific? We enforce that they have a > domain as they're inefficient right? > https://github.com/adblockplus/adblockplus/blob/master/lib/filterClasses.js#L933 Done. https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:99: result.push(filter.selector); On 2015/11/02 19:33:45, kzar wrote: > Mind just pushing `filter` instead of `filter.selector`? It would be useful to > have access to `filter.regexpString`, `filter.selectorPrefix` and > `filter.selectorSuffix` for the message responder in #2396. Done. I must've overseen that those are required as well, thanks. https://codereview.adblockplus.org/29324599/diff/29324600/lib/filterListener.js File lib/filterListener.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/filterListener.... lib/filterListener.js:31: require("filterClasses"); On 2015/11/05 12:46:19, Wladimir Palant wrote: > Nit: While I couldn't find anything in our style guide, I think that we usually > use double indentation in cases like this. Done. I looked through our code and unfortunately, it's quite inconsistent throughout. Anyway, double indentation makes more sense, true.
This now works for me without modifications and the code LGTM :) https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29324600/lib/cssRules.js#new... lib/cssRules.js:22: let {ElemHide: {getException}} = require("elemHide"); On 2015/11/25 11:20:00, Thomas Greiner wrote: > On 2015/10/24 17:14:51, kzar wrote: > > This results in a syntax error when building the script into adblockplus.js > for > > the Chrome extension. Changing it to `let {ElemHide} = require("elemHide");` > and > > later `ElemHide.getException(...` fixed it for me. > > Done. Since this is valid ES6 syntax, I'd consider this to be an issue with > JSHydra. However, I agree that for now I can make the change that you proposed > to avoid complications. Thanks, yea I agree that it looks like more of a problem with JSHydra than your code.
LGTM but see nit below. https://codereview.adblockplus.org/29324599/diff/29330769/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29330769/lib/cssRules.js#new... lib/cssRules.js:62: * @return {Array.<CSSPropertyFilter>} Nit: We usually use CSSPropertyFilter[] which is shorter and more like JavaScript. For sets or maps the awkward template syntax isn't avoidable, here it is.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29324599/diff/29330769/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29324599/diff/29330769/lib/cssRules.js#new... lib/cssRules.js:62: * @return {Array.<CSSPropertyFilter>} On 2015/11/26 12:52:46, Wladimir Palant wrote: > Nit: We usually use CSSPropertyFilter[] which is shorter and more like > JavaScript. For sets or maps the awkward template syntax isn't avoidable, here > it is. Done. Will include that change in the commit. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
