Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086213003:
Data structures for Safe Browsing subresource filtering rules. (Closed)
|
Created:
4 years, 6 months ago by pkalinnikov Modified:
4 years, 5 months ago Reviewers:
engedy CC:
battre Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionData structures for Safe Browsing subresource filtering rules.
The rules are defined in two formats:
1) Protocol Buffers, which is used as the wire format for transferring the
rules, and is intended to remain relatively stable.
2) FlatBuffers, which is used internally as a compact representation of the
rules, and which can be memory mapped into renderer processes directly.
This CL also fixes GYP files for flatc for iOS.
BUG=609747
Committed: https://crrev.com/2f6afa6d80d6c2addb3b95626140327570151e14
Cr-Commit-Position: refs/heads/master@{#401563}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Rephrased comments. #
Total comments: 6
Patch Set 3 : More rephrasings. #Patch Set 4 : Fix flatc build on iOS. #Patch Set 5 : Data structures for Subresource Filter. #Messages
Total messages: 41 (25 generated)
Description was changed from ========== Added definitions of ruleset data structures. BUG=609747 ========== to ========== Added definitions of ruleset data structures. The ruleset has 2 formats. One is protobuf, which is external, used for transferring the rules, and is supposed to be relatively stable. Another format is FlatBuffers, which is used internally for representing the ruleset as a flat data structure. BUG=609747 ==========
pkalinnikov@chromium.org changed reviewers: + battre@chromium.org, engedy@chromium.org
PTAL.
Looks great! I have done a final round of wordsmithing, plus a couple of minor comments, otherwise LGTM. Thank you! https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/flat/rules.fbs (right): https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:17: // Bitmask flags corresponding to various URL rule options. Phrasing nit: How about: URL rule matching options. These correspond to multiple fields of subresource_filter::proto::UrlRule, but here, they are represented as flags of the same bitmask to allow for compact storage. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:25: // A flat representation of a single URL rule. Could you please add a comment to the effect that a more detailed description of the fields is available in the definition for the protobuf? https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:27: // Rule matching options, a bitmask consisting of OptionFlag's. nit: OptionFlags. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:30: // A bitmask of element types (bits 0..11, as per proto/rules.proto). Please mention the proto enum type by name. Same below. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:51: // A URL pattern in the format, defined by |url_pattern_type|. nit: -, https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:54: // A compound failure function of the |url_pattern|. Used for SUBSTRING and Comment nit: // The compound Knuth-Morris-Pratt failure function corresponding to |url_pattern|. Also, could you please phrase the details below in a way so that it does not use personal pronouns, i.e. `we`? https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:69: // are stored in the lowest non-zero bytes. nit: String consisting ... (drop the reference to URL pattern, it's clear from the field below) nit: Could you please indicate endianness? https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:72: // The list of URL rules containing |ngram| as a URL pattern's substring. Phrasing nit: ... as a substring of their URL patterns. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:84: // The slot that is pointed to by all empty slots of |ngram_index| hash table. nit: Mention that this is a workaround needed because null offsets are not allowed as elements of FlatBuffer arrays. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:96: // A data structure used to store URL rules. Nit: The top-level data ... https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/proto/rules.proto (right): https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:50: // The type of subresource request that a URL rule should apply to. nit: types, requests https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:104: // The type of relation between the requested subresource and the source. This comment doesn't add up. How about: .. between the source of the requested subresource and that of the document? https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:118: // Defines whether the domain is excluded from the filter's affected domains. nit: s/filter's affected/set of/ https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:131: // Stores ElementType's the rule applies to as a bitwise OR of the Phrasing nit: I know that I recommended this, but on second thought this looks really weird. How about: // Stores the ElementTypes that the rule ... Same below. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:139: // The list of domains to be included/excluded from the filter's affected set. Phrasing nit: How about deleting this line, and rephrasing the next one as: The rule applies only to subresources of documents ... https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:141: // subdomains thereof). If the list is empty, the rule applies to documents nit: s/applies to/is applied on/ https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:160: // The rule applies only to URLs that match |url_pattern| in a case-sensitive When set, the ... https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:190: // A container for a number of rules subresource filter cares about. nit: // A container for lists of non-comment rules collated by RuleType.
Description was changed from ========== Added definitions of ruleset data structures. The ruleset has 2 formats. One is protobuf, which is external, used for transferring the rules, and is supposed to be relatively stable. Another format is FlatBuffers, which is used internally for representing the ruleset as a flat data structure. BUG=609747 ========== to ========== Added definitions of ruleset data structures. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. BUG=609747 ==========
Description was changed from ========== Added definitions of ruleset data structures. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. BUG=609747 ========== to ========== Added definitions of ruleset data structures. Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. BUG=609747 ==========
https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/flat/rules.fbs (right): https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:17: // Bitmask flags corresponding to various URL rule options. On 2016/06/22 11:47:55, engedy wrote: > Phrasing nit: How about: > > URL rule matching options. These correspond to multiple fields of > subresource_filter::proto::UrlRule, but here, they are represented as flags of > the same bitmask to allow for compact storage. Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:25: // A flat representation of a single URL rule. On 2016/06/22 11:47:54, engedy wrote: > Could you please add a comment to the effect that a more detailed description of > the fields is available in the definition for the protobuf? Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:27: // Rule matching options, a bitmask consisting of OptionFlag's. On 2016/06/22 11:47:55, engedy wrote: > nit: OptionFlags. Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:30: // A bitmask of element types (bits 0..11, as per proto/rules.proto). On 2016/06/22 11:47:55, engedy wrote: > Please mention the proto enum type by name. Same below. Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:51: // A URL pattern in the format, defined by |url_pattern_type|. On 2016/06/22 11:47:55, engedy wrote: > nit: -, Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:54: // A compound failure function of the |url_pattern|. Used for SUBSTRING and On 2016/06/22 11:47:55, engedy wrote: > Comment nit: > > // The compound Knuth-Morris-Pratt failure function corresponding to > |url_pattern|. > > Also, could you please phrase the details below in a way so that it does not use > personal pronouns, i.e. `we`? Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:69: // are stored in the lowest non-zero bytes. On 2016/06/22 11:47:54, engedy wrote: > nit: String consisting ... (drop the reference to URL pattern, it's clear from > the field below) > nit: Could you please indicate endianness? Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:72: // The list of URL rules containing |ngram| as a URL pattern's substring. On 2016/06/22 11:47:55, engedy wrote: > Phrasing nit: ... as a substring of their URL patterns. Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:84: // The slot that is pointed to by all empty slots of |ngram_index| hash table. On 2016/06/22 11:47:55, engedy wrote: > nit: Mention that this is a workaround needed because null offsets are not > allowed as elements of FlatBuffer arrays. Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/flat/rules.fbs:96: // A data structure used to store URL rules. On 2016/06/22 11:47:54, engedy wrote: > Nit: The top-level data ... Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... File components/subresource_filter/core/common/proto/rules.proto (right): https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:50: // The type of subresource request that a URL rule should apply to. On 2016/06/22 11:47:55, engedy wrote: > nit: types, requests Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:104: // The type of relation between the requested subresource and the source. On 2016/06/22 11:47:55, engedy wrote: > This comment doesn't add up. How about: > > .. between the source of the requested subresource and that of the document? Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:118: // Defines whether the domain is excluded from the filter's affected domains. On 2016/06/22 11:47:55, engedy wrote: > nit: s/filter's affected/set of/ Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:131: // Stores ElementType's the rule applies to as a bitwise OR of the On 2016/06/22 11:47:55, engedy wrote: > Phrasing nit: I know that I recommended this, but on second thought this looks > really weird. How about: > > // Stores the ElementTypes that the rule ... > > Same below. Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:139: // The list of domains to be included/excluded from the filter's affected set. On 2016/06/22 11:47:55, engedy wrote: > Phrasing nit: How about deleting this line, and rephrasing the next one as: > > The rule applies only to subresources of documents ... Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:141: // subdomains thereof). If the list is empty, the rule applies to documents On 2016/06/22 11:47:55, engedy wrote: > nit: s/applies to/is applied on/ Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:160: // The rule applies only to URLs that match |url_pattern| in a case-sensitive On 2016/06/22 11:47:55, engedy wrote: > When set, the ... Done. https://codereview.chromium.org/2086213003/diff/1/components/subresource_filt... components/subresource_filter/core/common/proto/rules.proto:190: // A container for a number of rules subresource filter cares about. On 2016/06/22 11:47:55, engedy wrote: > nit: > > // A container for lists of non-comment rules collated by RuleType. Done.
Description was changed from ========== Added definitions of ruleset data structures. Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. BUG=609747 ========== to ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. BUG=609747 ==========
Still LGTM % just some nits in one paragraph. https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... File components/subresource_filter/core/common/flat/rules.fbs (right): https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... components/subresource_filter/core/common/flat/rules.fbs:62: // (but, if some subpattern contains at least one '^' placeholder, all the nit: /(but,/, with the caveat that/ https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... components/subresource_filter/core/common/flat/rules.fbs:66: // Failure functions of all the subpattern are stored sequentially in the nit: // The failure functions of subpatterns are ... https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... components/subresource_filter/core/common/flat/rules.fbs:67: // |failure_function| array. Some of the subpatterns can be excempted from nit: Some subpatterns, however, will not have a corresponding failure function, e.g., ...
pkalinnikov@chromium.org changed reviewers: - battre@chromium.org
https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... File components/subresource_filter/core/common/flat/rules.fbs (right): https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... components/subresource_filter/core/common/flat/rules.fbs:62: // (but, if some subpattern contains at least one '^' placeholder, all the On 2016/06/22 13:06:05, engedy wrote: > nit: /(but,/, with the caveat that/ Done. https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... components/subresource_filter/core/common/flat/rules.fbs:66: // Failure functions of all the subpattern are stored sequentially in the On 2016/06/22 13:06:05, engedy wrote: > nit: // The failure functions of subpatterns are ... Done. https://codereview.chromium.org/2086213003/diff/20001/components/subresource_... components/subresource_filter/core/common/flat/rules.fbs:67: // |failure_function| array. Some of the subpatterns can be excempted from On 2016/06/22 13:06:05, engedy wrote: > nit: Some subpatterns, however, will not have a corresponding failure function, > e.g., ... Done.
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2086213003/#ps40001 (title: "More rephrasings.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086213003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Description was changed from ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. BUG=609747 ========== to ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. Fixed flatc compilation on iOS. BUG=609747 ==========
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2086213003/#ps60001 (title: "Changed flatc build config.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086213003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2086213003/#ps80001 (title: "Changed flatc build config, attempt 2.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086213003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Description was changed from ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. Fixed flatc compilation on iOS. BUG=609747 ========== to ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. This CL also fixes GYP files for flatc for iOS. BUG=609747 ==========
Patchset #4 (id:140001) has been deleted
Thanks for fixing the GYP files too, still LGTM. Try job failure seems to be a flake, it will likely pass for the next time.
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086213003/160001
Message was sent while issue was closed.
Description was changed from ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. This CL also fixes GYP files for flatc for iOS. BUG=609747 ========== to ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. This CL also fixes GYP files for flatc for iOS. BUG=609747 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. This CL also fixes GYP files for flatc for iOS. BUG=609747 ========== to ========== Data structures for Safe Browsing subresource filtering rules. The rules are defined in two formats: 1) Protocol Buffers, which is used as the wire format for transferring the rules, and is intended to remain relatively stable. 2) FlatBuffers, which is used internally as a compact representation of the rules, and which can be memory mapped into renderer processes directly. This CL also fixes GYP files for flatc for iOS. BUG=609747 Committed: https://crrev.com/2f6afa6d80d6c2addb3b95626140327570151e14 Cr-Commit-Position: refs/heads/master@{#401563} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2f6afa6d80d6c2addb3b95626140327570151e14 Cr-Commit-Position: refs/heads/master@{#401563}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:160001) has been created in https://codereview.chromium.org/2098453002/ by hbos@chromium.org. The reason for reverting is: I think this is causing compile errors on https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux.... [5683/25038] STAMP obj/components/subresource_filter/core/common/flat/flatbuffer_gen.inputdeps.stamp [5684/25038] ACTION //components/subresource_filter/core/common/flat:flatbuffer_gen(//build/toolchain/linux:clang_x64) FAILED: gen/components/subresource_filter/core/common/flat/rules_generated.h python ../../build/gn_run_binary.py flatc -c -o gen/components/subresource_filter/core/common/flat ../../components/subresource_filter/core/common/flat/rules.fbs. |
