Issue 29331737: Issue 3363 - Implement IsDocumentWhitelisted and IsElemhideWhitelisted (Closed)
|
Created:
Dec. 2, 2015, 1:27 p.m. by René Jeschke Modified:
Dec. 4, 2015, 9:16 a.m. Reviewers:
Felix Dahlke CC:
sergei Visibility:
Public. |
DescriptionIssue 3363 - Implement IsDocumentWhitelisted and IsElemhideWhitelisted
Patch Set 1 #Patch Set 2 : Fixed a typo #Patch Set 3 : Removed unnecessary @throw doc param. #
Total comments: 16
Patch Set 4 : Addressed some issues. #Patch Set 5 : Even more issues #
Total comments: 15
Patch Set 6 : Even more issues #Patch Set 7 : Coerce and tests. #
Total comments: 2
Patch Set 8 : Reverted a whitespace removal #
MessagesTotal messages: 12
https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:302: * Checks if the supplied URL is whitelisted. "Checks whether the document at the supplied URL is whitelisted", or something along those lines? Makes more sense to me in combination with the function name. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:303: * @param url URL to match. I'd vote for: "URL of the document" instead, "matching" doesn't seem to make that much sense here, it's not Matches(). https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:304: * @param documentUrls Chain of documents requesting the resource, starting If you decide to go for "URL of the document" above, we should also do a s/resource/document here. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:316: * Checks if the element hiding is whitelisted for the supplied URL. Doesn't sound entirely right to me, how about: "Checks whether element hiding is disabled at the supplied URL"? https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:441: const std::string& parent, ContentType contentType) const; Nit: s/parent/documentUrl/, analogous to the two Matches() variants? https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:441: const std::string& parent, ContentType contentType) const; Nit: Switch parent/documentUrl and contentType, analogous to Matches()? Same below. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:442: std::string GetWhitelistingFilter(const std::string& urlArg, Nit: s/urlArg/url/?
Sorry, published too soon, here's more. Can you also please add unit tests for both functions? We've got pretty good test coverage in libadblockplus, I'd like to keep that up. https://codereview.adblockplus.org/29331737/diff/29331744/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29331737/diff/29331744/src/FilterEngine.cp... src/FilterEngine.cpp:487: return match->GetProperty("text")->AsString(); Why return the filter text and not the filter object here, and an empty one in the error case? IIRC we always work with filter objects, and there's no reason to go ahead and extract the filter text - we don't really care. Same below.
New patch set up, unit tests follow as soon as I figure out how to do and test this. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:302: * Checks if the supplied URL is whitelisted. On 2015/12/02 17:17:29, Felix Dahlke wrote: > "Checks whether the document at the supplied URL is whitelisted", or something > along those lines? Makes more sense to me in combination with the function name. Done. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:303: * @param url URL to match. On 2015/12/02 17:17:29, Felix Dahlke wrote: > I'd vote for: "URL of the document" instead, "matching" doesn't seem to make > that much sense here, it's not Matches(). Done. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:304: * @param documentUrls Chain of documents requesting the resource, starting On 2015/12/02 17:17:29, Felix Dahlke wrote: > If you decide to go for "URL of the document" above, we should also do a > s/resource/document here. Done. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:316: * Checks if the element hiding is whitelisted for the supplied URL. On 2015/12/02 17:17:29, Felix Dahlke wrote: > Doesn't sound entirely right to me, how about: "Checks whether element hiding is > disabled at the supplied URL"? Done. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:441: const std::string& parent, ContentType contentType) const; On 2015/12/02 17:17:29, Felix Dahlke wrote: > Nit: s/parent/documentUrl/, analogous to the two Matches() variants? Done. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:441: const std::string& parent, ContentType contentType) const; On 2015/12/02 17:17:29, Felix Dahlke wrote: > Nit: s/parent/documentUrl/, analogous to the two Matches() variants? Done. https://codereview.adblockplus.org/29331737/diff/29331744/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:442: std::string GetWhitelistingFilter(const std::string& urlArg, On 2015/12/02 17:17:29, Felix Dahlke wrote: > Nit: s/urlArg/url/? Done. https://codereview.adblockplus.org/29331737/diff/29331744/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29331737/diff/29331744/src/FilterEngine.cp... src/FilterEngine.cpp:487: return match->GetProperty("text")->AsString(); On 2015/12/02 17:27:44, Felix Dahlke wrote: > Why return the filter text and not the filter object here, and an empty one in > the error case? IIRC we always work with filter objects, and there's no reason > to go ahead and extract the filter text - we don't really care. > > Same below. Done.
https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:303: * @param url URL of the document Nit: Seems we usually end with a period. https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:305: * starting with the current documents's parent frame, ending with Typo: s/documents's/document's/ https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:317: * @param url URL to match. Maybe it's not entirely correct, but I think it'd be clearer if we go for "URL of the document" as well here. https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:318: * @param documentUrls Chain of documents requesting the resource, starting Same as above, we're not really talking about a "resources"here. https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cp... src/FilterEngine.cpp:331: return GetWhitelistingFilter(url, CONTENT_TYPE_DOCUMENT, documentUrls) != 0; Nit: != 0 is redundant, according to the Mozilla coding style it should be left out. (Just assuming that actually compiles and I'm not missing something here, I'm getting a bit rusty.) Same below. https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cp... src/FilterEngine.cpp:482: Micro nit: Superfluous whitespace? Can't tell if it's deliberate or not.
New patch set up. https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:303: * @param url URL of the document On 2015/12/02 18:02:01, Felix Dahlke wrote: > Nit: Seems we usually end with a period. Done. https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:305: * starting with the current documents's parent frame, ending with On 2015/12/02 18:02:02, Felix Dahlke wrote: > Typo: s/documents's/document's/ Done. https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:317: * @param url URL to match. On 2015/12/02 18:01:58, Felix Dahlke wrote: > Maybe it's not entirely correct, but I think it'd be clearer if we go for "URL > of the document" as well here. Done. https://codereview.adblockplus.org/29331737/diff/29331750/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:318: * @param documentUrls Chain of documents requesting the resource, starting On 2015/12/02 18:02:01, Felix Dahlke wrote: > Same as above, we're not really talking about a "resources"here. Done. https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cp... src/FilterEngine.cpp:331: return GetWhitelistingFilter(url, CONTENT_TYPE_DOCUMENT, documentUrls) != 0; On 2015/12/02 18:02:03, Felix Dahlke wrote: > Nit: != 0 is redundant, according to the Mozilla coding style it should be left > out. (Just assuming that actually compiles and I'm not missing something here, > I'm getting a bit rusty.) > > Same below. I know, and I wouldn't normally do this (as you know), but this time I insist on having the compare here, as we're returning a 'bool' and it feels/looks wrong returning an object instead. :-) https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cp... src/FilterEngine.cpp:482: On 2015/12/02 18:02:03, Felix Dahlke wrote: > Micro nit: Superfluous whitespace? Can't tell if it's deliberate or not. Done.
https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cp... src/FilterEngine.cpp:331: return GetWhitelistingFilter(url, CONTENT_TYPE_DOCUMENT, documentUrls) != 0; On 2015/12/02 18:09:42, René Jeschke wrote: > On 2015/12/02 18:02:03, Felix Dahlke wrote: > > Nit: != 0 is redundant, according to the Mozilla coding style it should be > left > > out. (Just assuming that actually compiles and I'm not missing something here, > > I'm getting a bit rusty.) > > > > Same below. > > I know, and I wouldn't normally do this (as you know), but this time I insist on > having the compare here, as we're returning a 'bool' and it feels/looks wrong > returning an object instead. :-) Let me take a part, I would compare with `nullptr` instead of `0` or use `!!` to indicate that we are adapting it to bool. BTW, having `explicit operator bool` of `FilterPtr` will cause a compilation error if we try to return in instance of `FilterPtr`.
Looks good now, don't care too much about the `!= 0` business. Mainly waiting for the tests now :) https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cp... src/FilterEngine.cpp:331: return GetWhitelistingFilter(url, CONTENT_TYPE_DOCUMENT, documentUrls) != 0; On 2015/12/02 18:21:57, sergei wrote: > On 2015/12/02 18:09:42, René Jeschke wrote: > > On 2015/12/02 18:02:03, Felix Dahlke wrote: > > > Nit: != 0 is redundant, according to the Mozilla coding style it should be > > left > > > out. (Just assuming that actually compiles and I'm not missing something > here, > > > I'm getting a bit rusty.) > > > > > > Same below. > > > > I know, and I wouldn't normally do this (as you know), but this time I insist > on > > having the compare here, as we're returning a 'bool' and it feels/looks wrong > > returning an object instead. :-) > > Let me take a part, I would compare with `nullptr` instead of `0` or use `!!` to > indicate that we are adapting it to bool. > BTW, having `explicit operator bool` of `FilterPtr` will cause a compilation > error if we try to return in instance of `FilterPtr`. Yeah, !! is pretty idiomatic for coercing an arbitrary value to boolean.
New patch set up, tests included. https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29331737/diff/29331750/src/FilterEngine.cp... src/FilterEngine.cpp:331: return GetWhitelistingFilter(url, CONTENT_TYPE_DOCUMENT, documentUrls) != 0; On 2015/12/02 18:50:26, Felix Dahlke wrote: > On 2015/12/02 18:21:57, sergei wrote: > > On 2015/12/02 18:09:42, René Jeschke wrote: > > > On 2015/12/02 18:02:03, Felix Dahlke wrote: > > > > Nit: != 0 is redundant, according to the Mozilla coding style it should be > > > left > > > > out. (Just assuming that actually compiles and I'm not missing something > > here, > > > > I'm getting a bit rusty.) > > > > > > > > Same below. > > > > > > I know, and I wouldn't normally do this (as you know), but this time I > insist > > on > > > having the compare here, as we're returning a 'bool' and it feels/looks > wrong > > > returning an object instead. :-) > > > > Let me take a part, I would compare with `nullptr` instead of `0` or use `!!` > to > > indicate that we are adapting it to bool. > > BTW, having `explicit operator bool` of `FilterPtr` will cause a compilation > > error if we try to return in instance of `FilterPtr`. > > Yeah, !! is pretty idiomatic for coercing an arbitrary value to boolean. Yep, fine with me. Went with !! in the end.
Tests look good, just one nit now. https://codereview.adblockplus.org/29331737/diff/29331819/src/FilterEngine.cpp File src/FilterEngine.cpp (left): https://codereview.adblockplus.org/29331737/diff/29331819/src/FilterEngine.cp... src/FilterEngine.cpp:137: FilterEngine::FilterEngine(JsEnginePtr jsEngine, Nit: Entirely unrelated, separate noissue commit please.
New patch set up. https://codereview.adblockplus.org/29331737/diff/29331819/src/FilterEngine.cpp File src/FilterEngine.cpp (left): https://codereview.adblockplus.org/29331737/diff/29331819/src/FilterEngine.cp... src/FilterEngine.cpp:137: FilterEngine::FilterEngine(JsEnginePtr jsEngine, On 2015/12/03 20:34:18, Felix Dahlke wrote: > Nit: Entirely unrelated, separate noissue commit please. Done.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
