Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parser for publisher v4 prefix list #5713

Merged
merged 1 commit into from Jun 5, 2020
Merged

Conversation

@zenparsing
Copy link
Collaborator

zenparsing commented Jun 2, 2020

Resolves brave/brave-browser#10056

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.
@zenparsing zenparsing changed the title Add publisher list v4 reader class Add parser for publisher v4 prefix list Jun 2, 2020
@zenparsing zenparsing force-pushed the ksmith-publisher-list-reader branch 5 times, most recently from 0ee1397 to f019def Jun 2, 2020
@zenparsing zenparsing marked this pull request as ready for review Jun 2, 2020
@zenparsing zenparsing requested a review from NejcZdovc as a code owner Jun 2, 2020
@zenparsing zenparsing requested a review from brave/rewards Jun 2, 2020
@emerick
emerick approved these changes Jun 2, 2020
Copy link
Contributor

emerick left a comment

LGTM, a couple of comments.

@NejcZdovc NejcZdovc requested a review from bridiver Jun 2, 2020

namespace {

class BrotliStreamDecoder {

This comment has been minimized.

Copy link
@bridiver

bridiver Jun 2, 2020

Collaborator

there is already existing code for handling Brotli in chromium BrotliSourceStream

This comment has been minimized.

Copy link
@bridiver

bridiver Jun 2, 2020

Collaborator

in fact most of this code appears to be copied from chromium and there's no reason to do that. We can just use it directly

This comment has been minimized.

Copy link
@zenparsing

zenparsing Jun 3, 2020

Author Collaborator

For future reference, reusing BrotliSourceStream from net/filter would require creating a SourceStream class from a string (example here).

@zenparsing zenparsing force-pushed the ksmith-publisher-list-reader branch 2 times, most recently from 6b83cd3 to ea33a94 Jun 2, 2020
@zenparsing zenparsing self-assigned this Jun 3, 2020
uint32 prefix_size = 1;

// The type of compression applied to the hash prefix list.
CompressionType compression_type = 2;

This comment has been minimized.

Copy link
@zenparsing

zenparsing Jun 3, 2020

Author Collaborator

This approach of compressing the data inside of the protobuf message is inspired by Safe Browsing (see here), which embeds Rice-encoded prefix delta values into the message when the prefix lengths are 4 bytes. This message format will give us the option of moving in that direction in the future if we choose.

@zenparsing zenparsing force-pushed the ksmith-publisher-list-reader branch from ea33a94 to aac822b Jun 4, 2020
@zenparsing zenparsing force-pushed the ksmith-publisher-list-reader branch from aac822b to af703e5 Jun 5, 2020
@NejcZdovc NejcZdovc added this to the 1.12.x - Nightly milestone Jun 5, 2020
@zenparsing zenparsing merged commit d0e86a2 into master Jun 5, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/jenkins/pr-head This commit is being built
Details
SonarCloud Code Analysis Quality Gate passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zenparsing zenparsing deleted the ksmith-publisher-list-reader branch Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.