Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844293003:
Factor out UrlPatternIndex from IndexedRuleset. (Closed)
|
Created:
3 years, 7 months ago by pkalinnikov Modified:
3 years, 7 months ago CC:
chromium-reviews, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor out UrlPatternIndex from IndexedRuleset.
This CL extracts the UrlPatternIndex class out of IndexedRuleset, and keeps the
latter's implementation use the former as a building block.
The new class can serve as a general-purpose tool for matching network requests
against a set of URL rules. For example, it can be used by Declarative
WebRequest API implementation.
The follow-up step would be to cover UrlPatternIndex with tests and move it out
from subresource_filter component to some generic place, e.g.,
components/url_matcher or components/url_pattern_index.
BUG=713774
Review-Url: https://codereview.chromium.org/2844293003
Cr-Commit-Position: refs/heads/master@{#469618}
Committed: https://chromium.googlesource.com/chromium/src/+/7a70ae843511b3e6d4f8a3924904fd3f7d1e0cba
Patch Set 1 #
Total comments: 31
Patch Set 2 : Address some comments from karandeepb@. #
Total comments: 47
Patch Set 3 : Address comments from engedy@. #Patch Set 4 : Clean up. #
Total comments: 6
Patch Set 5 : Address final nits. #Dependent Patchsets: Messages
Total messages: 41 (28 generated)
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...
Description was changed from ========== Factor out UrlPatternIndex from IndexedRuleset. BUG= ========== to ========== Factor out UrlPatternIndex from IndexedRuleset. BUG=713774 ==========
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org, karandeepb@chromium.org
Balazs, PTAL. Do you think it's necessary to add UrlPatternIndex tests in this CL? Generally it would be nice because there will be a direct client of this class. But maybe I can do it in a follow-up CL, because the current IndexedRuleset tests cover subresource_filter's usage. Karan, feel free to leave comments. Note that I will address DWR use-case later, this one is just the first step. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/document_subresource_filter_unittest.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/document_subresource_filter_unittest.cc:14: nit: Remove blank line.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for working on this Pavel. Looks good. Have left some nits and questions. I am guessing the next step would be to move url_pattern_index[.h/cc/fbs] to some more central directory or component? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:30: if (!offset.o) offset.o looks ugly and should be avoided. I think you did this because you are handling the not convertible case within SerializeUrlRule. Maybe return a unique_ptr/base::Optional? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.h:36: static const int kIndexedFormatVersion; nit: Might be worth clarifying that this should be updated whenever either of the IndexedRuleset or the UrlPatternIndex schemas are changed. Do you have presubmit tests for the same? If yes, they might need to be updated. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/url_pattern_index.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:260: : flat_builder_(flat_builder) {} DCHECK(flat_builder_)? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:265: if (!offset.o) .o? This seems to be relying on https://cs.chromium.org/chromium/src/third_party/flatbuffers/src/include/flat... but doesn't seem to be documented. If possible, avoid this. It's not clear what this is trying to do as it depends on FlatBuffers internals. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:268: const auto* rule = flatbuffers::GetTemporaryPointer(*flat_builder_, offset); For my own edification, what does GetTemporaryPointer do? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:281: return IndexUrlRule(SerializeUrlRule(rule, flat_builder_)); nit: you may inline this. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:557: return ::subresource_filter::FindMatch(*flat_index_, url, first_party_origin, Can't this just be FindMatch(..)? (Remove ::subresource_filter::) https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/url_pattern_index.h (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:53: // Creates a Builder which serializes a URL pattern index to |flat_builder|. nit: The class header comment should be enough. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:63: bool IndexUrlRule(const proto::UrlRule& rule); Is this used anywhere? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:88: flatbuffers::FlatBufferBuilder* flat_builder_; nit. Clarify we don't own this. Add comment: // Weak. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:95: class UrlPatternIndex { nit: Maybe call this UrlPatternIndexMatcher to clarify its purpose? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:98: UrlPatternIndex(const flat::UrlPatternIndex* flat_index); Nit: add explicit. The current comment on the constructor seems redundant, due to the class header comment. Also, might be worthwhile to specify that |flat_index| can be null. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:122: const flat::UrlPatternIndex* flat_index_; nit: Add comment // Weak.
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...
Thanks Karan. Addressed all your comments except 2 so far. Yes, the next step would be to find a generic place for url_pattern_index*, and then we would need to think how to support DWR, e.g., add priorities and adapt FindMatch. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/document_subresource_filter_unittest.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/document_subresource_filter_unittest.cc:14: On 2017/04/27 16:07:24, pkalinnikov wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/url_pattern_index.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:260: : flat_builder_(flat_builder) {} On 2017/04/27 19:56:25, karandeepb wrote: > DCHECK(flat_builder_)? Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:268: const auto* rule = flatbuffers::GetTemporaryPointer(*flat_builder_, offset); On 2017/04/27 19:56:25, karandeepb wrote: > For my own edification, what does GetTemporaryPointer do? It allows accessing objects in a FlatBuffer during its construction. Before this feature was introduced, it was only possible to read the buffer after finalizing the build phase. The returned pointer is called *Temporary* because it is safe to use only while the buffer stays unmodified, otherwise memory reallocations might screw it up (the same guarantee as vector::iterator has w.r.t. vector modifications). https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:281: return IndexUrlRule(SerializeUrlRule(rule, flat_builder_)); On 2017/04/27 19:56:25, karandeepb wrote: > nit: you may inline this. Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:557: return ::subresource_filter::FindMatch(*flat_index_, url, first_party_origin, On 2017/04/27 19:56:25, karandeepb wrote: > Can't this just be FindMatch(..)? (Remove ::subresource_filter::) Nope, my local compiler complains because it thinks it is UrlPatternIndex::FindMatch being called. It suggests to write the namespace explicitly. Alternatively, I could try not overloading the FindMatch name so heavily. WDYT? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/url_pattern_index.h (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:53: // Creates a Builder which serializes a URL pattern index to |flat_builder|. On 2017/04/27 19:56:26, karandeepb wrote: > nit: The class header comment should be enough. Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:63: bool IndexUrlRule(const proto::UrlRule& rule); On 2017/04/27 19:56:25, karandeepb wrote: > Is this used anywhere? No. I thought it would be nice to use (at least in tests), but let's just inline its body. Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:88: flatbuffers::FlatBufferBuilder* flat_builder_; On 2017/04/27 19:56:25, karandeepb wrote: > nit. Clarify we don't own this. Add comment: // Weak. Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:95: class UrlPatternIndex { On 2017/04/27 19:56:26, karandeepb wrote: > nit: Maybe call this UrlPatternIndexMatcher to clarify its purpose? Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:98: UrlPatternIndex(const flat::UrlPatternIndex* flat_index); On 2017/04/27 19:56:25, karandeepb wrote: > Nit: add explicit. The current comment on the constructor seems redundant, due > to the class header comment. Also, might be worthwhile to specify that > |flat_index| can be null. Added explicit. I don't think this comment is redundant, because the generic class comment has no details about FlatBuffers. Indicating nullability of |flat_index| makes sense, done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.h:122: const flat::UrlPatternIndex* flat_index_; On 2017/04/27 19:56:26, karandeepb wrote: > nit: Add comment // Weak. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.h:36: static const int kIndexedFormatVersion; On 2017/04/27 19:56:25, karandeepb wrote: > nit: Might be worth clarifying that this should be updated whenever either of > the IndexedRuleset or the UrlPatternIndex schemas are changed. Do you have > presubmit tests for the same? If yes, they might need to be updated. Good point. We don't have presubmit checks yet, but I will add them (and update commenting) in crrev/2850813003
LGTM. Thanks! https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:30: if (!offset.o) On 2017/04/27 19:56:25, karandeepb wrote: > offset.o looks ugly and should be avoided. I think you did this because you are > handling the not convertible case within SerializeUrlRule. Maybe return a > unique_ptr/base::Optional? If you decide to keep this, I think it would be nice to at least comment that you are checking for a null offset. |offset.o| doesn't describe much. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/url_pattern_index.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:557: return ::subresource_filter::FindMatch(*flat_index_, url, first_party_origin, On 2017/04/28 10:06:50, pkalinnikov wrote: > On 2017/04/27 19:56:25, karandeepb wrote: > > Can't this just be FindMatch(..)? (Remove ::subresource_filter::) > > Nope, my local compiler complains because it thinks it is > UrlPatternIndex::FindMatch being called. It suggests to write the namespace > explicitly. > > Alternatively, I could try not overloading the FindMatch name so heavily. WDYT? Weird since they have different signatures. Anyways, your call. This seems fine as well.
LGTM % minor comments and nits. Great refactoring. Also quite painless, which is a testament to the quality of this code! https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:7: #include <algorithm> nit: These three are no longer used. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:30: if (!offset.o) It would be nice to see the type here or add a comment to elucidate what ".o" stands for. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:36: const auto* flat_rule = flatbuffers::GetTemporaryPointer(builder_, offset); Yay, the practical-but-ugly lambda is no longer needed. \o/ https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:69: activation_(root_->activation_index()) {} nit: Have you considered calling this |deactivation_| or |whitelist_activation_| and the other |whitelist_element|? (provided that's correct) https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:8: #include <stdint.h> nit: Not sure if either includes the other, but the canonical include for size_t is stddef.h. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:12: #include "components/subresource_filter/core/common/flat/rules_generated.h" nit: Shouldn't this be indexed_ruleset_generated.h? https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/url_pattern_index.cc (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.cc:195: // Only the following activation types are supported, ignore the others. Nice cleanup! :) https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.cc:267: if (!offset.o) Can this be a DCHECK? https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.cc:463: const flat::UrlRule* FindMatch(const FlatUrlRuleList* rules, nit: How about FindMatchAmongCandidates(... candidates, ...)? https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/url_pattern_index.h (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:8: #include <stdint.h> nit: stddef.h for size_t https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:48: // The class used to construct an index built on the URL patterns of a set of nit: s/built on/over/ https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:49: // URL rules. The index is stored together with the rules in a FlatBuffer comment suggestion instead of last sentence: // The rules themselves need to be converted to Flatbuffers format by the client of this class, as well as persisted into the |flat_builder| that is supplied in the constructor. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:56: // Adds a UrlRule to the index. The rule should be stored in the same Phrasing nit: The caller should have already persisted the rule into the same |flat_builder| by a call to SerializeUrlRule, and should pass in the resulting |offset| here. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:84: // Weak. nit: The term `weak` is a bit ambiguous in my opinion, how about: "Must outlive this instance." (Same below.) https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:90: // Represents a read-only index built on the URL patterns of a set of URL rules. nit: s/Represents/Encapsulates/, s/on/over/ https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:91: // Provides fast matching of network requests against these rules. nit: ... rules, and provides ... https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:94: // Uses |flat_index| FlatBuffer object to read the index from. If |flat_index| nit: Creates an instance to access the given |flat_index|. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:99: // Returns an arbitrary UrlRule in the index matching the request, or nullptr phrasing nit: If the index contains one or more UrlRules that match the request, returns one of them (it is undefined which one). Otherwise, returns nullptr. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:102: // A rule matches the request iff all of the following applies: nit: A rule is deemed to match the ... https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:105: // - Either |element_type| or |activation_type| is present in the list of the nit: I think it would make this more readable to introduce another paragraph after the first 2 lines, to describe the meaning of these two arguments, and what happens when they are unspecified. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:109: // - The rule is not generic if |disable_generic_rules|. nit: ... is true. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:111: // Note: |is_third_party| should reflect relation between |first_party_origin| nit: This could also go into the above-mentioned second paragraph. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:111: // Note: |is_third_party| should reflect relation between |first_party_origin| nit: should be pre-computed by the caller, e.g. ..., to reflect the relation ...
Addressed all the comments (hopefully). Do you guys want to take another look? https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/indexed_ruleset.cc:30: if (!offset.o) On 2017/04/28 17:48:21, karandeepb wrote: > On 2017/04/27 19:56:25, karandeepb wrote: > > offset.o looks ugly and should be avoided. I think you did this because you > are > > handling the not convertible case within SerializeUrlRule. Maybe return a > > unique_ptr/base::Optional? > > If you decide to keep this, I think it would be nice to at least comment that > you are checking for a null offset. |offset.o| doesn't describe much. Done. https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/url_pattern_index.cc (right): https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:265: if (!offset.o) On 2017/04/27 19:56:25, karandeepb wrote: > .o? This seems to be relying on > https://cs.chromium.org/chromium/src/third_party/flatbuffers/src/include/flat... > but doesn't seem to be documented. If possible, avoid this. It's not clear what > this is trying to do as it depends on FlatBuffers internals. Double-checked with the FlatBuffers author that checking the "o" field is reliable. FlatBufferBuilder.Create* methods and Create* functions never return zero offset for serialized objects. I am inclined to keep this as is, because other approaches seem more ugly (unique_ptr, base::Optional). https://codereview.chromium.org/2844293003/diff/1/components/subresource_filt... components/subresource_filter/core/common/url_pattern_index.cc:557: return ::subresource_filter::FindMatch(*flat_index_, url, first_party_origin, On 2017/04/28 17:48:21, karandeepb wrote: > On 2017/04/28 10:06:50, pkalinnikov wrote: > > On 2017/04/27 19:56:25, karandeepb wrote: > > > Can't this just be FindMatch(..)? (Remove ::subresource_filter::) > > > > Nope, my local compiler complains because it thinks it is > > UrlPatternIndex::FindMatch being called. It suggests to write the namespace > > explicitly. > > > > Alternatively, I could try not overloading the FindMatch name so heavily. > WDYT? > > Weird since they have different signatures. Anyways, your call. This seems fine > as well. Got rid of name overloading. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.cc (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:7: #include <algorithm> On 2017/04/28 20:13:43, engedy wrote: > nit: These three are no longer used. Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:30: if (!offset.o) On 2017/04/28 20:13:43, engedy wrote: > It would be nice to see the type here or add a comment to elucidate what ".o" > stands for. Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:36: const auto* flat_rule = flatbuffers::GetTemporaryPointer(builder_, offset); On 2017/04/28 20:13:43, engedy wrote: > Yay, the practical-but-ugly lambda is no longer needed. \o/ Acknowledged. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.cc:69: activation_(root_->activation_index()) {} On 2017/04/28 20:13:43, engedy wrote: > nit: Have you considered calling this |deactivation_| or |whitelist_activation_| > and the other |whitelist_element|? (provided that's correct) Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/indexed_ruleset.h (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:8: #include <stdint.h> On 2017/04/28 20:13:43, engedy wrote: > nit: Not sure if either includes the other, but the canonical include for size_t > is stddef.h. Done. Although, this file needs uint8_t as well, so let's have both includes. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/indexed_ruleset.h:12: #include "components/subresource_filter/core/common/flat/rules_generated.h" On 2017/04/28 20:13:43, engedy wrote: > nit: Shouldn't this be indexed_ruleset_generated.h? Wow, how come it have passed the trybots? Fixed here and in other places. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/url_pattern_index.cc (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.cc:195: // Only the following activation types are supported, ignore the others. On 2017/04/28 20:13:43, engedy wrote: > Nice cleanup! :) Acknowledged. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.cc:267: if (!offset.o) On 2017/04/28 20:13:43, engedy wrote: > Can this be a DCHECK? Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.cc:463: const flat::UrlRule* FindMatch(const FlatUrlRuleList* rules, On 2017/04/28 20:13:43, engedy wrote: > nit: How about FindMatchAmongCandidates(... candidates, ...)? Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/url_pattern_index.h (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:8: #include <stdint.h> On 2017/04/28 20:13:44, engedy wrote: > nit: stddef.h for size_t Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:48: // The class used to construct an index built on the URL patterns of a set of On 2017/04/28 20:13:44, engedy wrote: > nit: s/built on/over/ Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:49: // URL rules. The index is stored together with the rules in a FlatBuffer On 2017/04/28 20:13:44, engedy wrote: > comment suggestion instead of last sentence: > > // The rules themselves need to be converted to Flatbuffers format by the client > of this class, as well as persisted into the |flat_builder| that is supplied in > the constructor. Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:56: // Adds a UrlRule to the index. The rule should be stored in the same On 2017/04/28 20:13:44, engedy wrote: > Phrasing nit: The caller should have already persisted the rule into the same > |flat_builder| by a call to SerializeUrlRule, and should pass in the resulting > |offset| here. Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:84: // Weak. On 2017/04/28 20:13:44, engedy wrote: > nit: The term `weak` is a bit ambiguous in my opinion, how about: > > "Must outlive this instance." > > (Same below.) Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:90: // Represents a read-only index built on the URL patterns of a set of URL rules. On 2017/04/28 20:13:44, engedy wrote: > nit: s/Represents/Encapsulates/, s/on/over/ Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:91: // Provides fast matching of network requests against these rules. On 2017/04/28 20:13:44, engedy wrote: > nit: ... rules, and provides ... Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:94: // Uses |flat_index| FlatBuffer object to read the index from. If |flat_index| On 2017/04/28 20:13:44, engedy wrote: > nit: Creates an instance to access the given |flat_index|. Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:99: // Returns an arbitrary UrlRule in the index matching the request, or nullptr On 2017/04/28 20:13:44, engedy wrote: > phrasing nit: If the index contains one or more UrlRules that match the request, > returns one of them (it is undefined which one). Otherwise, returns nullptr. Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:102: // A rule matches the request iff all of the following applies: On 2017/04/28 20:13:44, engedy wrote: > nit: A rule is deemed to match the ... Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:105: // - Either |element_type| or |activation_type| is present in the list of the On 2017/04/28 20:13:44, engedy wrote: > nit: I think it would make this more readable to introduce another paragraph > after the first 2 lines, to describe the meaning of these two arguments, and > what happens when they are unspecified. How about this? https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:109: // - The rule is not generic if |disable_generic_rules|. On 2017/04/28 20:13:44, engedy wrote: > nit: ... is true. Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:111: // Note: |is_third_party| should reflect relation between |first_party_origin| On 2017/04/28 20:13:44, engedy wrote: > nit: should be pre-computed by the caller, e.g. ..., to reflect the relation ... Done. https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:111: // Note: |is_third_party| should reflect relation between |first_party_origin| On 2017/04/28 20:13:44, engedy wrote: > nit: This could also go into the above-mentioned second paragraph. Done.
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...
Description was changed from ========== Factor out UrlPatternIndex from IndexedRuleset. BUG=713774 ========== to ========== Factor out UrlPatternIndex from IndexedRuleset. This CL extracts a UrlPatternIndex class out of IndexedRuleset, and keeps the latter's implementation use the former as a building block. The new class can serve as a general-purpose tool for matching network requests against a set of URL rules. For example, it can be used by Declarative WebRequest API implementation. The follow-up step would be to cover UrlPatternIndex with tests and move it out from subresource_filter component to some generic place, e.g., components/url_matcher or components/url_pattern_index. BUG=713774 ==========
Description was changed from ========== Factor out UrlPatternIndex from IndexedRuleset. This CL extracts a UrlPatternIndex class out of IndexedRuleset, and keeps the latter's implementation use the former as a building block. The new class can serve as a general-purpose tool for matching network requests against a set of URL rules. For example, it can be used by Declarative WebRequest API implementation. The follow-up step would be to cover UrlPatternIndex with tests and move it out from subresource_filter component to some generic place, e.g., components/url_matcher or components/url_pattern_index. BUG=713774 ========== to ========== Factor out UrlPatternIndex from IndexedRuleset. This CL extracts the UrlPatternIndex class out of IndexedRuleset, and keeps the latter's implementation use the former as a building block. The new class can serve as a general-purpose tool for matching network requests against a set of URL rules. For example, it can be used by Declarative WebRequest API implementation. The follow-up step would be to cover UrlPatternIndex with tests and move it out from subresource_filter component to some generic place, e.g., components/url_matcher or components/url_pattern_index. BUG=713774 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM.
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...
Thanks! Balazs, I'm waiting for a final sign-off from you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Still LGTM, couldn't resist some nits. :) https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... File components/subresource_filter/core/common/url_pattern_index.h (right): https://codereview.chromium.org/2844293003/diff/20001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:105: // - Either |element_type| or |activation_type| is present in the list of the On 2017/05/02 14:04:36, pkalinnikov wrote: > On 2017/04/28 20:13:44, engedy wrote: > > nit: I think it would make this more readable to introduce another paragraph > > after the first 2 lines, to describe the meaning of these two arguments, and > > what happens when they are unspecified. > > How about this? This now reads very well, thanks a lot for the polishing! https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... File components/subresource_filter/core/common/url_pattern_index.h (right): https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:44: // it the resulting buffer. Returns null offset iff the |rule| can not be nit: it in the ... could not be ... or it is otherwise invalid. https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:79: // N-gram. For a single rule the N-gram used as an index key is picked nit: For each given rule, the ... https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:116: // - The |is_third_party| bit matches the rule's requirement on the request's nit: requested
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...
Done, thanks! Will submit as soon as the bots become happy. https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... File components/subresource_filter/core/common/url_pattern_index.h (right): https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:44: // it the resulting buffer. Returns null offset iff the |rule| can not be On 2017/05/05 08:00:16, engedy wrote: > nit: it in the ... could not be ... or it is otherwise invalid. Done. https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:79: // N-gram. For a single rule the N-gram used as an index key is picked On 2017/05/05 08:00:16, engedy wrote: > nit: For each given rule, the ... Done. https://codereview.chromium.org/2844293003/diff/60001/components/subresource_... components/subresource_filter/core/common/url_pattern_index.h:116: // - The |is_third_party| bit matches the rule's requirement on the request's On 2017/05/05 08:00:16, engedy wrote: > nit: requested Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from karandeepb@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2844293003/#ps80001 (title: "Address final nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493975581837050,
"parent_rev": "abecaf4066bdffab7af3e26102d2e2644835d8a9", "commit_rev":
"7a70ae843511b3e6d4f8a3924904fd3f7d1e0cba"}
Message was sent while issue was closed.
Description was changed from ========== Factor out UrlPatternIndex from IndexedRuleset. This CL extracts the UrlPatternIndex class out of IndexedRuleset, and keeps the latter's implementation use the former as a building block. The new class can serve as a general-purpose tool for matching network requests against a set of URL rules. For example, it can be used by Declarative WebRequest API implementation. The follow-up step would be to cover UrlPatternIndex with tests and move it out from subresource_filter component to some generic place, e.g., components/url_matcher or components/url_pattern_index. BUG=713774 ========== to ========== Factor out UrlPatternIndex from IndexedRuleset. This CL extracts the UrlPatternIndex class out of IndexedRuleset, and keeps the latter's implementation use the former as a building block. The new class can serve as a general-purpose tool for matching network requests against a set of URL rules. For example, it can be used by Declarative WebRequest API implementation. The follow-up step would be to cover UrlPatternIndex with tests and move it out from subresource_filter component to some generic place, e.g., components/url_matcher or components/url_pattern_index. BUG=713774 Review-Url: https://codereview.chromium.org/2844293003 Cr-Commit-Position: refs/heads/master@{#469618} Committed: https://chromium.googlesource.com/chromium/src/+/7a70ae843511b3e6d4f8a3924904... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7a70ae843511b3e6d4f8a3924904... |
