Issue 5840485868371968: Issue 616 - Add $generichide + $genericblock filter options and enforce them. (Closed)
|
Created:
March 6, 2015, 9:45 p.m. by kzar Modified:
Sept. 28, 2015, 12:37 p.m. CC:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 616 - Add $generichide + $genericblock filter options and enforce them.
If you want to you can use this page for testing http://holly.cat/genericblockhide-test.html I added these custom filters:
~somewhere.com,static.kzar.co.uk###kzarNonGenericHideWithException
kzar_exception_specific_block.jpg$domain=~blah.kzar.co.uk|kzar.co.uk
||static.kzar.co.uk/genericblockhide-test/kzar_specific_block.jpg
kzar.co.uk###kzarAd
~somewhere.com###kzarGenericHideOnlyExceptions
?KzarPopupBlock=$popup,domain=static.kzar.co.uk
@@holly.cat$genericblock
@@holly.cat$generichide
Note: Now depends on this review https://codereview.adblockplus.org/5991150368325632/ because we make use of the specificOnly parameter.
Patch Set 1 #Patch Set 2 : Rebased. #Patch Set 3 : Tidied up genericblock logic in contentPolicy code. #Patch Set 4 : Rebased onto typeMask changes #
Total comments: 5
Patch Set 5 : Simplified generic block logic #
Total comments: 4
Patch Set 6 : Use camelCase for genericblockMatch variable name #
Total comments: 7
Patch Set 7 : Fixed interaction with sitekey and generichide logic #
Total comments: 8
Patch Set 8 : Addressed Thomas' feedback #
Total comments: 8
Patch Set 9 : Improved logic based on Wladimir's suggestions #Patch Set 10 : Removed duplicate comment #
Total comments: 2
Patch Set 11 : Remove extra parenthesis #MessagesTotal messages: 28
Patch Set 3 : Tidied up genericblock logic in contentPolicy code.
I don't see anything obvious here. But I'm not too familiar with this code.
Patch Set 4 : Rebased onto typeMask changes
https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/content... lib/contentPolicy.js:165: let genericblock = { match: null }; Nit: We don't add spaces at the beginning/end of an object literal. https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/content... lib/contentPolicy.js:165: let genericblock = { match: null }; Also it might be a bad idea performance-wise to create an object in the first place, assuming this code is a hotspot. https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/content... lib/contentPolicy.js:184: else if (!(genericblock.match instanceof WhitelistFilter) && contentType != Policy.type.ELEMHIDE) Wouldn't genericblock.match alway be null here, hence the type check unneeded?
Patch Set 5 : Simplified generic block logic https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/content... lib/contentPolicy.js:165: let genericblock = { match: null }; On 2015/07/14 17:23:33, Sebastian Noack wrote: > Nit: We don't add spaces at the beginning/end of an object literal. Acknowledged. https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/content... lib/contentPolicy.js:184: else if (!(genericblock.match instanceof WhitelistFilter) && contentType != Policy.type.ELEMHIDE) On 2015/07/14 17:23:33, Sebastian Noack wrote: > Wouldn't genericblock.match alway be null here, hence the type check unneeded? You're right, I think I was emulating some of the surrounding code which performed a few extra checks (like `if (!match...` when match is always null) to avoid future mistakes. Anyway I agree, it's better to avoid creating a genericblock object unnecessarily or performing too many extra tests on it. Done.
Patch Set 5 : Simplified generic block logic (This patchset is good, sorry I had accidentally included an unrelated commit last time.)
https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/content... lib/contentPolicy.js:187: let genericblock_match = defaultMatcher.matchesAny(testWndLocation, RegExpFilter.typeMap.GENERICBLOCK, parentDocDomain, false); Nit: camelcase? https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/content... lib/contentPolicy.js:189: { Nit: Feel free to remove the redundant braces. Though I don't mind either way.
Patch Set 6 : Use camelCase for genericblockMatch variable name https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/content... lib/contentPolicy.js:187: let genericblock_match = defaultMatcher.matchesAny(testWndLocation, RegExpFilter.typeMap.GENERICBLOCK, parentDocDomain, false); On 2015/07/15 14:57:19, Sebastian Noack wrote: > Nit: camelcase? Done. https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/content... lib/contentPolicy.js:189: { On 2015/07/15 14:57:19, Sebastian Noack wrote: > Nit: Feel free to remove the redundant braces. Though I don't mind either way. I think in this instance it looks nicer with them.
LGTM from my side. Wladimir's go now.
Talked to Wladimir: He won't find the time to review this, so we agreed to have Sebastian, Thomas and me review instead.
https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/content... lib/contentPolicy.js:187: let genericblockMatch = defaultMatcher.matchesAny(testWndLocation, RegExpFilter.typeMap.GENERICBLOCK, parentDocDomain, false); You forgot to pass the sitekey as the last parameter for `defaultMatcher.matchesAny()`. Not passing the sitekey would cause filters of the form `@@...$genericblock,sitekey=...` not to match on pages that provide a correct sitekey. https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/content... lib/contentPolicy.js:229: if (!(match instanceof WhitelistFilter) && (!location.domains || location.domains[""])) I guess you meant to write (!match.domains || match.domains[""]) because `location` is a `nsIRUI` which doesn't even have a "domains" property. https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/content... lib/contentPolicy.js:229: if (!(match instanceof WhitelistFilter) && (!location.domains || location.domains[""])) What about filters that don't specify domains but a sitekey instead? This logic would consider them to be generic even though they aren't. https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/filterC... lib/filterClasses.js:762: // DOCUMENT, ELEMHIDE, POPUP, GENERICHIDE and GENERICBLOCK options shouldn't be there by default Detail: Mind the 80 character line limitation (also in other places).
Patch Set 7 : Fixed interaction with sitekey and generichide logic https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/content... lib/contentPolicy.js:187: let genericblockMatch = defaultMatcher.matchesAny(testWndLocation, RegExpFilter.typeMap.GENERICBLOCK, parentDocDomain, false); On 2015/08/20 11:36:06, Thomas Greiner wrote: > You forgot to pass the sitekey as the last parameter for > `defaultMatcher.matchesAny()`. Not passing the sitekey would cause filters of > the form `@@...$genericblock,sitekey=...` not to match on pages that provide a > correct sitekey. Done. https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/content... lib/contentPolicy.js:229: if (!(match instanceof WhitelistFilter) && (!location.domains || location.domains[""])) On 2015/08/20 11:36:06, Thomas Greiner wrote: > I guess you meant to write > > (!match.domains || match.domains[""]) > > because `location` is a `nsIRUI` which doesn't even have a "domains" property. Done. https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/filterC... lib/filterClasses.js:762: // DOCUMENT, ELEMHIDE, POPUP, GENERICHIDE and GENERICBLOCK options shouldn't be there by default On 2015/08/20 11:36:07, Thomas Greiner wrote: > Detail: Mind the 80 character line limitation (also in other places). Done.
https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/content... lib/contentPolicy.js:186: else if (contentType != Policy.type.ELEMHIDE) Detail: The "else" keyword is redundant here due to the "return" in the block before. https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/content... lib/contentPolicy.js:276: if (match && match.isGeneric() && generichide) I'd assume that checking for "generichide" first would lead to a quicker result here because it's less likely to be truthy and also doesn't require a function call. https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... lib/filterClasses.js:484: isGeneric: function() /**Boolean*/ Detail: Documentation is great but please avoid duplication.
Patch Set 8 : Addressed Thomas' feedback https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/content... lib/contentPolicy.js:186: else if (contentType != Policy.type.ELEMHIDE) On 2015/08/28 10:28:24, Thomas Greiner wrote: > Detail: The "else" keyword is redundant here due to the "return" in the block > before. Done. https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/content... lib/contentPolicy.js:276: if (match && match.isGeneric() && generichide) On 2015/08/28 10:28:24, Thomas Greiner wrote: > I'd assume that checking for "generichide" first would lead to a quicker result > here because it's less likely to be truthy and also doesn't require a function > call. Done. https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... lib/filterClasses.js:484: isGeneric: function() /**Boolean*/ On 2015/08/28 10:28:24, Thomas Greiner wrote: > Detail: Documentation is great but please avoid duplication. I'm not sure where the duplication is here, but I'm open to suggestions if there's something that you'd like to change.
https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:166: let generichide = null; It's either a blocking rule or hiding rule - so at most one of these variables will be used. I suggest having a single variable called nogeneric here. https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:254: } This is simply duplicating the logic from above. I suggest that you merge this with the check above - for contentType == Policy.type.ELEMHIDE you should check for RegExpFilter.typeMap.GENERICHIDE, otherwise RegExpFilter.typeMap.GENERICBLOCK. And the same variable as mentioned above. https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:298: if (genericblock && match.isGeneric()) What if there are multiple filters potentially matching here, one that is generic and another that isn't? You cannot rely on Matcher.matchesAny() to always return the latter.
https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:298: if (genericblock && match.isGeneric()) On 2015/09/03 13:09:43, Wladimir Palant wrote: > What if there are multiple filters potentially matching here, one that is > generic and another that isn't? You cannot rely on Matcher.matchesAny() to > always return the latter. Hmm good point, any suggestions? In the other review I add a specificOnly parameter to matchesAny(), so one option would be to search for a specific rule here before deciding the genericblock exception should be used. That would mean adding another search however.
https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:298: if (genericblock && match.isGeneric()) On 2015/09/04 15:49:10, kzar wrote: > Hmm good point, any suggestions? In the other review I add a specificOnly > parameter to matchesAny(), so one option would be to search for a specific rule > here before deciding the genericblock exception should be used. That would mean > adding another search however. Why another search? Just pass specificOnly and be done with it. Yes, you won't know whether the $genericelemhide exception really had any effect - but you can just count this hit unconditionally. It's sufficient to know that the $genericelemhide exception applied on that page, no real need to know whether it had any effect (see $document exceptions, they work the same). Note that there are the same issues with $genericelemhide currently, these cannot be solved in Firefox however.
Patch Set 9 : Improved logic based on Wladimir's suggestions https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:166: let generichide = null; On 2015/09/03 13:09:42, Wladimir Palant wrote: > It's either a blocking rule or hiding rule - so at most one of these variables > will be used. I suggest having a single variable called nogeneric here. Done. https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:254: } On 2015/09/03 13:09:42, Wladimir Palant wrote: > This is simply duplicating the logic from above. I suggest that you merge this > with the check above - for contentType == Policy.type.ELEMHIDE you should check > for RegExpFilter.typeMap.GENERICHIDE, otherwise > RegExpFilter.typeMap.GENERICBLOCK. And the same variable as mentioned above. Done. https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/content... lib/contentPolicy.js:298: if (genericblock && match.isGeneric()) On 2015/09/04 16:16:23, Wladimir Palant wrote: > On 2015/09/04 15:49:10, kzar wrote: > > Hmm good point, any suggestions? In the other review I add a specificOnly > > parameter to matchesAny(), so one option would be to search for a specific > rule > > here before deciding the genericblock exception should be used. That would > mean > > adding another search however. > > Why another search? Just pass specificOnly and be done with it. Yes, you won't > know whether the $genericelemhide exception really had any effect - but you can > just count this hit unconditionally. It's sufficient to know that the > $genericelemhide exception applied on that page, no real need to know whether it > had any effect (see $document exceptions, they work the same). > > Note that there are the same issues with $genericelemhide currently, these > cannot be solved in Firefox however. Oh, of course! Done.
LGTM
https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... lib/filterClasses.js:484: isGeneric: function() /**Boolean*/ On 2015/09/03 12:37:02, kzar wrote: > On 2015/08/28 10:28:24, Thomas Greiner wrote: > > Detail: Documentation is great but please avoid duplication. > > I'm not sure where the duplication is here, but I'm open to suggestions if > there's something that you'd like to change. The duplication I meant is that you specify the return type both in the comment block above as well as inline on this line.
Patch Set 10 : Removed duplicate comment https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterC... lib/filterClasses.js:484: isGeneric: function() /**Boolean*/ On 2015/09/21 16:53:36, Thomas Greiner wrote: > On 2015/09/03 12:37:02, kzar wrote: > > On 2015/08/28 10:28:24, Thomas Greiner wrote: > > > Detail: Documentation is great but please avoid duplication. > > > > I'm not sure where the duplication is here, but I'm open to suggestions if > > there's something that you'd like to change. > > The duplication I meant is that you specify the return type both in the comment > block above as well as inline on this line. I see, Done.
LGTM
LGTM
Looks good to me, just a small nit. https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterC... lib/filterClasses.js:485: return (!(this.sitekeys && this.sitekeys.length) && Nit: Superfluous parentheses around the expression.
Patch Set 11 : Remove extra parenthesis https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterC... File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterC... lib/filterClasses.js:485: return (!(this.sitekeys && this.sitekeys.length) && On 2015/09/28 08:28:45, Felix Dahlke wrote: > Nit: Superfluous parentheses around the expression. Done.
LGTM
LGTM 
|
