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

Use new Rust based ad-block library #2569

Merged
merged 9 commits into from
Jun 24, 2019
Merged

Use new Rust based ad-block library #2569

merged 9 commits into from
Jun 24, 2019

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented May 31, 2019

Fix brave/brave-browser#4793
Please let me merge if approved, I still need to verify CI.

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.

@bbondy bbondy self-assigned this May 31, 2019
@bbondy bbondy force-pushed the adblock-rust-ffi branch 6 times, most recently from 592c470 to 8c1d830 Compare June 7, 2019 02:52
@bbondy bbondy requested review from emerick and mkarolin June 11, 2019 02:15
@bbondy bbondy marked this pull request as ready for review June 11, 2019 02:16
@bbondy bbondy force-pushed the adblock-rust-ffi branch 3 times, most recently from cce1b54 to 555e37c Compare June 11, 2019 02:42
emerick
emerick previously approved these changes Jun 11, 2019
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, had a couple of minor typo nits

components/brave_shields/browser/ad_block_base_service.cc Outdated Show resolved Hide resolved
components/brave_shields/browser/ad_block_base_service.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No successful CI build yet (and I can't build locally on mac at the moment) so I am not sure where libadblock ends up and if the signing script needs to be patched to have it signed.

@bbondy
Copy link
Member Author

bbondy commented Jun 12, 2019

(and I can't build locally on mac at the moment)

Do you mean you can't build this PR in particular? Or you can't build at all?

if the signing script needs to be patched to have it signed.

There's a commit to do the signing of it already. I haven't verified on CI if that works yet though.

@mkarolin
Copy link
Collaborator

(and I can't build locally on mac at the moment)

Do you mean you can't build this PR in particular? Or you can't build at all?

Just meant I was on Windows and couldn't switch to Mac at that moment.

@bbondy bbondy force-pushed the adblock-rust-ffi branch 7 times, most recently from 292f9b5 to 5199963 Compare June 13, 2019 18:54
@bbondy bbondy mentioned this pull request Jun 20, 2019
32 tasks
@bbondy bbondy force-pushed the adblock-rust-ffi branch 2 times, most recently from d8514d2 to 2d6aecf Compare June 23, 2019 20:51
@bbondy bbondy added CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Jun 24, 2019
@simonhong
Copy link
Member

simonhong commented Jun 24, 2019

There's a commit to do the signing of it already. I haven't verified on CI if that works yet though.

@bbondy I think you missed one thing in that commit.
See https://github.com/brave/brave-core/blob/master/patches/chrome-installer-mac-signing-signing.py.patch#L26 cc:@mkarolin

@bbondy bbondy removed CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Jun 24, 2019
@bbondy bbondy added CI/skip Do not run CI builds (except noplatform) CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux and removed CI/skip Do not run CI builds (except noplatform) labels Jun 24, 2019
@bbondy bbondy merged commit 4ca1a58 into master Jun 24, 2019
@mihaiplesa mihaiplesa deleted the adblock-rust-ffi branch June 24, 2019 14:56
@mihaiplesa mihaiplesa added this to the 0.68.x - Nightly milestone Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use new adblock-rust library
5 participants