Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183883002:
Implement deactivating SubresourceFilter for document. (Closed)
|
Created:
4 years, 4 months ago by pkalinnikov Modified:
4 years, 4 months ago CC:
mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@integrate_ruleset Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement deactivating SubresourceFilter for document.
BUG=609747
Committed: https://crrev.com/fb1e4c1329b610261b46cbfaa5027ffdcc3e2915
Cr-Commit-Position: refs/heads/master@{#409476}
Patch Set 1 #Patch Set 2 : Rebase and add unittest. #
Total comments: 18
Patch Set 3 : Address comments of engedy@ and add more counter checks. #
Total comments: 30
Patch Set 4 : Address comments from engedy@ and add more tests. #
Total comments: 14
Patch Set 5 : Address more comments from engedy@ #Messages
Total messages: 23 (10 generated)
Description was changed from ========== Implement deactivating SubresourceFilter for document. BUG=609747 ========== to ========== Implement deactivating SubresourceFilter for document. BUG=609747 ==========
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org
Balazs, PTAL. Unittests for this are coming.
https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:87: document_origin_ = url::Origin(ancestor_document_urls.front()); nit: Maybe group this line with the loop below. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: if (i != size) This will never evaluate true. Also, can we make this loop prettier somehow, potentially a range-based loop or an iterator-based loop? https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:99: // TODO(pkalinnikov): Implement GENERICBLOCK activation type as well. How much work is this? Can we implement it already in this CL? https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:59: IndexedRulesetMatcher matcher_; nit: s//ruleset_matcher_/ for increased clarity. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:363: if (!IsMatch(url, UrlPattern(*rule), begin, end)) nit: Would also suggest a more specific name for this, there is an IsMatch in the namespace scope and also in the class scope. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:395: bool IndexedRulesetMatcher::IsDeactivated( Can you please think a bit about making method names more specific across this class? Two things in particular: -- Now that we have different things being evaluated, we should name the noun that the verb acts on in each method names. -- We should find a way to distinguish between page-level activation coming from Safe Browsing, and document-level activation dictated by the subresource filtering rules.
https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: if (i != size) On 2016/07/27 13:23:14, engedy wrote: > This will never evaluate true. Also, can we make this loop prettier somehow, > potentially a range-based loop or an iterator-based loop? Never evaluate *false*.
https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:87: document_origin_ = url::Origin(ancestor_document_urls.front()); On 2016/07/27 13:23:14, engedy wrote: > nit: Maybe group this line with the loop below. Done. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: if (i != size) On 2016/07/27 13:24:40, engedy wrote: > On 2016/07/27 13:23:14, engedy wrote: > > This will never evaluate true. Also, can we make this loop prettier somehow, > > potentially a range-based loop or an iterator-based loop? > > Never evaluate *false*. This was a typo. Now the condition does not hold exactly once (on the last iteration). I think that range-base loop is not an option, since anyway I need some arithmetic to access 2 successive items simultaneously. How about this loop with iterators? https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:99: // TODO(pkalinnikov): Implement GENERICBLOCK activation type as well. On 2016/07/27 13:23:14, engedy wrote: > How much work is this? Can we implement it already in this CL? 1 more IsDeactivated call + passing this bit to all IsAllowed requests all the way down to evaluating against single rules. For each single rule we need to know a bit whether the rule is generic, and this is currently not cached in the data structures. It's expensive to recalculate it every time (scan domain list for positive domains). I propose to do it in a separate CL, because it can be format-changing. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:59: IndexedRulesetMatcher matcher_; On 2016/07/27 13:23:14, engedy wrote: > nit: s//ruleset_matcher_/ for increased clarity. Done. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:363: if (!IsMatch(url, UrlPattern(*rule), begin, end)) On 2016/07/27 13:23:15, engedy wrote: > nit: Would also suggest a more specific name for this, there is an IsMatch in > the namespace scope and also in the class scope. Done. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:395: bool IndexedRulesetMatcher::IsDeactivated( On 2016/07/27 13:23:15, engedy wrote: > Can you please think a bit about making method names more specific across this > class? > > Two things in particular: > -- Now that we have different things being evaluated, we should name the noun > that the verb acts on in each method names. > -- We should find a way to distinguish between page-level activation coming > from Safe Browsing, and document-level activation dictated by the subresource > filtering rules. Done.
Looking good % nits + one important question regarding blacklist vs. whitelist and the desire for moar tests. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: if (i != size) On 2016/07/27 17:12:15, pkalinnikov wrote: > On 2016/07/27 13:24:40, engedy wrote: > > On 2016/07/27 13:23:14, engedy wrote: > > > This will never evaluate true. Also, can we make this loop prettier somehow, > > > potentially a range-based loop or an iterator-based loop? > > > > Never evaluate *false*. > > This was a typo. Now the condition does not hold exactly once (on the last > iteration). > > I think that range-base loop is not an option, since anyway I need some > arithmetic to access 2 successive items simultaneously. How about this loop with > iterators? How about this (you should feel free to redefine |ancestor_document_urls| to have reverse ordering so that you can make this range-based): url::Origin ancestor_document_parent_origin; for (auto iter = ancestor_document_urls.rbegin(); iter != ancestor_document_urls.rend(); ++iter) { const GURL& ancestor_document_url(*iter); if (ruleset_matcher_.ShouldDisableFilteringForDocument( ancestor_document_url, ancestor_document_parent_origin, proto::ACTIVATION_TYPE_DOCUMENT)) { disable_filtering_for_document_ = true; return; } ancestor_document_parent_origin = url::Origin(ancestor_document_url); } https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:99: // TODO(pkalinnikov): Implement GENERICBLOCK activation type as well. On 2016/07/27 17:12:15, pkalinnikov wrote: > On 2016/07/27 13:23:14, engedy wrote: > > How much work is this? Can we implement it already in this CL? > > 1 more IsDeactivated call + passing this bit to all IsAllowed requests all the > way down to evaluating against single rules. For each single rule we need to > know a bit whether the rule is generic, and this is currently not cached in the > data structures. It's expensive to recalculate it every time (scan domain list > for positive domains). > > I propose to do it in a separate CL, because it can be format-changing. Acknowledged. Keep in mind that in https://codereview.chromium.org/2182493009/ I have added RulesetIndexer::kIndexedFormatVersion that will need to be incremented every time there is a change to the Flatbuffer data format. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:101: deactivated_ = ruleset_matcher_.IsFilteringDeactivated( My pet naming peeve: ShouldDisableFilteringForDocument (or Deactivate) https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:121: if (ruleset_matcher_.IsLoadDisallowed(GURL(resourceUrl), document_origin_, Sorry for being so pedantic, but let's be even more specific here: ShouldDisallowResourceLoad() https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:51: size_t num_loads_requested() const { return num_loads_requested_; } nit: How about simply s/requested/total/? It's not immediately obvious to the reader what requested might mean. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:67: bool deactivated_ = false; I think it is important to emphasize how this differs from |activation_state| above. How about the following: // Even when subresource filtering is activated at the page level by the |activation_state| passed into the constructor, the current document or ancestors thereof may still match special filtering rules that specifically disable the application of other types of rules on these documents. See proto::ActivationType for details. Regarding naming, we should be more expressive, e.g., |filtering_disabled_for_document_|, and in the future |generic_rules_disabled_for_document_| and |domain_specific_rules_disabled_for_document_|, or use a bitmask |disabled_rule_types_for_document_|. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/content/renderer/subresource_filter_agent.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/subresource_filter_agent.cc:69: "SubresourceFilter.DocumentLoad.NumSubresourceLoads.Requested", You will need to add a description of this into histograms.xml, see https://codereview.chromium.org/2161963002/ for an example. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:405: return IsMatch(root_->blacklist_index(), document_url, parent_document_origin, Shouldn't |whitelist_index| and |blacklist_index| be reversed here? https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:112: // Returns whether subresource filtering should be deactivated according to Phrasing nit: How about: ... whether subresource filtering rules according to |activation_type| should be disabled for ... https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:116: // Unlike page-level activation, this can be used for fine-grained filtering Phrasing nit: ..., such rules can be used to have fine-grained control over the activation of filtering ... https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset_unittest.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:50: bool ShouldDeactivate(const char* url, nit: s/url/document_url/ https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:96: proto::UrlRule rule = CreateRule(url_pattern, kAnyParty, false); Wouldn't these be whitelist rules most of the time? https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:499: const char* url; nit: document_url https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:503: {"example.com", kDocument, "http://example.com", kDocument, true}, We should have at least one test for invalid URL and unspecified activation type. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:505: {"||xample.com", kDocument, "http://example.com", kDocument, false}, Do you intend kSubdomain or just a simple mismatch here? If the latter, let's use non-matching, but alphabetic characters to avoid a confusion. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:525: ShouldDeactivate(test_case.url, nullptr /* initiator */, Please add tests for non-null initiators. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:562: TEST_F(IndexedRulesetTest, BlacklistWhitelist) { Please add a test analogous to this, but for activation.
PTAL. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:101: deactivated_ = ruleset_matcher_.IsFilteringDeactivated( On 2016/07/28 16:46:15, engedy wrote: > My pet naming peeve: ShouldDisableFilteringForDocument (or Deactivate) Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:121: if (ruleset_matcher_.IsLoadDisallowed(GURL(resourceUrl), document_origin_, On 2016/07/28 16:46:15, engedy wrote: > Sorry for being so pedantic, but let's be even more specific here: > ShouldDisallowResourceLoad() Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:51: size_t num_loads_requested() const { return num_loads_requested_; } On 2016/07/28 16:46:15, engedy wrote: > nit: How about simply s/requested/total/? It's not immediately obvious to the > reader what requested might mean. Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:67: bool deactivated_ = false; On 2016/07/28 16:46:15, engedy wrote: > I think it is important to emphasize how this differs from |activation_state| > above. How about the following: > > // Even when subresource filtering is activated at the page level by the > |activation_state| passed into the constructor, the current document or > ancestors thereof may still match special filtering rules that specifically > disable the application of other types of rules on these documents. See > proto::ActivationType for details. > > Regarding naming, we should be more expressive, e.g., > |filtering_disabled_for_document_|, and in the future > |generic_rules_disabled_for_document_| and > |domain_specific_rules_disabled_for_document_|, or use a bitmask > |disabled_rule_types_for_document_|. Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/content/renderer/subresource_filter_agent.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/content/renderer/subresource_filter_agent.cc:69: "SubresourceFilter.DocumentLoad.NumSubresourceLoads.Requested", On 2016/07/28 16:46:15, engedy wrote: > You will need to add a description of this into histograms.xml, see > https://codereview.chromium.org/2161963002/ for an example. Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:405: return IsMatch(root_->blacklist_index(), document_url, parent_document_origin, On 2016/07/28 16:46:16, engedy wrote: > Shouldn't |whitelist_index| and |blacklist_index| be reversed here? Also, we only need whitelist here. Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:112: // Returns whether subresource filtering should be deactivated according to On 2016/07/28 16:46:16, engedy wrote: > Phrasing nit: How about: ... whether subresource filtering rules according to > |activation_type| should be disabled for ... Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:116: // Unlike page-level activation, this can be used for fine-grained filtering On 2016/07/28 16:46:16, engedy wrote: > Phrasing nit: ..., such rules can be used to have fine-grained control over the > activation of filtering ... Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset_unittest.cc (right): https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:50: bool ShouldDeactivate(const char* url, On 2016/07/28 16:46:16, engedy wrote: > nit: s/url/document_url/ Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:96: proto::UrlRule rule = CreateRule(url_pattern, kAnyParty, false); On 2016/07/28 16:46:16, engedy wrote: > Wouldn't these be whitelist rules most of the time? They should. Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:499: const char* url; On 2016/07/28 16:46:16, engedy wrote: > nit: document_url Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:503: {"example.com", kDocument, "http://example.com", kDocument, true}, On 2016/07/28 16:46:16, engedy wrote: > We should have at least one test for invalid URL and unspecified activation > type. Added test for invalid URL. Regarding the unspecified activation type, there are couple of tests above already. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:505: {"||xample.com", kDocument, "http://example.com", kDocument, false}, On 2016/07/28 16:46:16, engedy wrote: > Do you intend kSubdomain or just a simple mismatch here? If the latter, let's > use non-matching, but alphabetic characters to avoid a confusion. Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:525: ShouldDeactivate(test_case.url, nullptr /* initiator */, On 2016/07/28 16:46:16, engedy wrote: > Please add tests for non-null initiators. Done. https://codereview.chromium.org/2183883002/diff/40001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:562: TEST_F(IndexedRulesetTest, BlacklistWhitelist) { On 2016/07/28 16:46:16, engedy wrote: > Please add a test analogous to this, but for activation. Since blacklist activation rules are not possible, I add a mix of a blacklist regular rule and whitelist activation.
LGTM % nits. You will need to add one of the /tools/metrics/OWNERS for histograms.xml, though. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: if (i != size) On 2016/07/28 16:46:15, engedy wrote: > On 2016/07/27 17:12:15, pkalinnikov wrote: > > On 2016/07/27 13:24:40, engedy wrote: > > > On 2016/07/27 13:23:14, engedy wrote: > > > > This will never evaluate true. Also, can we make this loop prettier > somehow, > > > > potentially a range-based loop or an iterator-based loop? > > > > > > Never evaluate *false*. > > > > This was a typo. Now the condition does not hold exactly once (on the last > > iteration). > > > > I think that range-base loop is not an option, since anyway I need some > > arithmetic to access 2 successive items simultaneously. How about this loop > with > > iterators? > > How about this (you should feel free to redefine |ancestor_document_urls| to > have reverse ordering so that you can make this range-based): > > url::Origin ancestor_document_parent_origin; > for (auto iter = ancestor_document_urls.rbegin(); > iter != ancestor_document_urls.rend(); ++iter) { > const GURL& ancestor_document_url(*iter); > if (ruleset_matcher_.ShouldDisableFilteringForDocument( > ancestor_document_url, ancestor_document_parent_origin, > proto::ACTIVATION_TYPE_DOCUMENT)) { > disable_filtering_for_document_ = true; > return; > } > ancestor_document_parent_origin = url::Origin(ancestor_document_url); > } So what do you think about this? https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:48: // The number of subresource loads went through the allowLoad method. typo: ... loads that went ... https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:49: size_t num_loads_total() const { return num_loads_requested_; } nit: Rename variable to num_loads_requested_ for consistency. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:74: // Indicates whether the DOCUMENT activation type takes place. How about using the same phrasing you used for histograms.xml: // Indicates whether the document is subject to a whitelist rule with DOCUMENT activation type. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:115: // Returns whether subresource filtering rules according to |activation_type| nit: ... the subset of subresource filtering rules specified by ... https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset_unittest.cc (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:55: DCHECK_NE(matcher_.get(), nullptr); nit: You can simply use DCHECK(matcher_); https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:99: rule.set_element_types(proto::ELEMENT_TYPE_UNSPECIFIED); nit: This line is not really needed, right? I can see the value in explicitly specifying this for readability, but then we should do it in all functions around here for consistency.
pkalinnikov@chromium.org changed reviewers: + rkaplow@chromium.org
Adding rkaplow@ for reviewing the histograms.xml. https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: if (i != size) On 2016/08/01 17:01:10, engedy wrote: > On 2016/07/28 16:46:15, engedy wrote: > > On 2016/07/27 17:12:15, pkalinnikov wrote: > > > On 2016/07/27 13:24:40, engedy wrote: > > > > On 2016/07/27 13:23:14, engedy wrote: > > > > > This will never evaluate true. Also, can we make this loop prettier > > somehow, > > > > > potentially a range-based loop or an iterator-based loop? > > > > > > > > Never evaluate *false*. > > > > > > This was a typo. Now the condition does not hold exactly once (on the last > > > iteration). > > > > > > I think that range-base loop is not an option, since anyway I need some > > > arithmetic to access 2 successive items simultaneously. How about this loop > > with > > > iterators? > > > > How about this (you should feel free to redefine |ancestor_document_urls| to > > have reverse ordering so that you can make this range-based): > > > > url::Origin ancestor_document_parent_origin; > > for (auto iter = ancestor_document_urls.rbegin(); > > iter != ancestor_document_urls.rend(); ++iter) { > > const GURL& ancestor_document_url(*iter); > > if (ruleset_matcher_.ShouldDisableFilteringForDocument( > > ancestor_document_url, ancestor_document_parent_origin, > > proto::ACTIVATION_TYPE_DOCUMENT)) { > > disable_filtering_for_document_ = true; > > return; > > } > > ancestor_document_parent_origin = url::Origin(ancestor_document_url); > > } > > So what do you think about this? Oh, I forgot about this comment. Looks nice, I refactored this piece as you proposed (but with shorter variable names). https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:48: // The number of subresource loads went through the allowLoad method. On 2016/08/01 17:01:10, engedy wrote: > typo: ... loads that went ... Done. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:49: size_t num_loads_total() const { return num_loads_requested_; } On 2016/08/01 17:01:10, engedy wrote: > nit: Rename variable to num_loads_requested_ for consistency. I believe you meant "num_loads_total_". Renamed. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:74: // Indicates whether the DOCUMENT activation type takes place. On 2016/08/01 17:01:10, engedy wrote: > How about using the same phrasing you used for histograms.xml: > > // Indicates whether the document is subject to a whitelist rule with DOCUMENT > activation type. Done. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:115: // Returns whether subresource filtering rules according to |activation_type| On 2016/08/01 17:01:10, engedy wrote: > nit: ... the subset of subresource filtering rules specified by ... Done. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset_unittest.cc (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:55: DCHECK_NE(matcher_.get(), nullptr); On 2016/08/01 17:01:10, engedy wrote: > nit: You can simply use DCHECK(matcher_); Done. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:99: rule.set_element_types(proto::ELEMENT_TYPE_UNSPECIFIED); On 2016/08/01 17:01:10, engedy wrote: > nit: This line is not really needed, right? I can see the value in explicitly > specifying this for readability, but then we should do it in all functions > around here for consistency. This line is actually needed, because the CreateRule function initializes |element_types| to ELEMENT_TYPE_ALL, and it's not what I need in tests for activation types.
LGTM. Thank you! https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.cc (right): https://codereview.chromium.org/2183883002/diff/20001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.cc:91: if (i != size) On 2016/08/02 12:16:35, pkalinnikov wrote: > On 2016/08/01 17:01:10, engedy wrote: > > On 2016/07/28 16:46:15, engedy wrote: > > > On 2016/07/27 17:12:15, pkalinnikov wrote: > > > > On 2016/07/27 13:24:40, engedy wrote: > > > > > On 2016/07/27 13:23:14, engedy wrote: > > > > > > This will never evaluate true. Also, can we make this loop prettier > > > somehow, > > > > > > potentially a range-based loop or an iterator-based loop? > > > > > > > > > > Never evaluate *false*. > > > > > > > > This was a typo. Now the condition does not hold exactly once (on the last > > > > iteration). > > > > > > > > I think that range-base loop is not an option, since anyway I need some > > > > arithmetic to access 2 successive items simultaneously. How about this > loop > > > with > > > > iterators? > > > > > > How about this (you should feel free to redefine |ancestor_document_urls| to > > > have reverse ordering so that you can make this range-based): > > > > > > url::Origin ancestor_document_parent_origin; > > > for (auto iter = ancestor_document_urls.rbegin(); > > > iter != ancestor_document_urls.rend(); ++iter) { > > > const GURL& ancestor_document_url(*iter); > > > if (ruleset_matcher_.ShouldDisableFilteringForDocument( > > > ancestor_document_url, ancestor_document_parent_origin, > > > proto::ACTIVATION_TYPE_DOCUMENT)) { > > > disable_filtering_for_document_ = true; > > > return; > > > } > > > ancestor_document_parent_origin = url::Origin(ancestor_document_url); > > > } > > > > So what do you think about this? > > Oh, I forgot about this comment. Looks nice, I refactored this piece as you > proposed (but with shorter variable names). Acknowledged. https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/content/renderer/document_subresource_filter.h (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/content/renderer/document_subresource_filter.h:49: size_t num_loads_total() const { return num_loads_requested_; } On 2016/08/02 12:16:35, pkalinnikov wrote: > On 2016/08/01 17:01:10, engedy wrote: > > nit: Rename variable to num_loads_requested_ for consistency. > > I believe you meant "num_loads_total_". Renamed. Oops, yeah. Thanks for not just marking this no-op as "Done". :-) https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset_unittest.cc (right): https://codereview.chromium.org/2183883002/diff/60001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset_unittest.cc:99: rule.set_element_types(proto::ELEMENT_TYPE_UNSPECIFIED); On 2016/08/02 12:16:35, pkalinnikov wrote: > On 2016/08/01 17:01:10, engedy wrote: > > nit: This line is not really needed, right? I can see the value in explicitly > > specifying this for readability, but then we should do it in all functions > > around here for consistency. > > This line is actually needed, because the CreateRule function initializes > |element_types| to ELEMENT_TYPE_ALL, and it's not what I need in tests for > activation types. Ah, yes, I missed that. SGTM.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm histograms lg
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement deactivating SubresourceFilter for document. BUG=609747 ========== to ========== Implement deactivating SubresourceFilter for document. BUG=609747 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implement deactivating SubresourceFilter for document. BUG=609747 ========== to ========== Implement deactivating SubresourceFilter for document. BUG=609747 Committed: https://crrev.com/fb1e4c1329b610261b46cbfaa5027ffdcc3e2915 Cr-Commit-Position: refs/heads/master@{#409476} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fb1e4c1329b610261b46cbfaa5027ffdcc3e2915 Cr-Commit-Position: refs/heads/master@{#409476} |
