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

Replace handwritten adblock rust FFI with CXX based FFI #17368

Merged
merged 4 commits into from
Jul 28, 2023
Merged

Conversation

DJAndries
Copy link
Collaborator

Resolves brave/brave-browser#22985

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Standard regression testing.

@DJAndries DJAndries requested review from a team and iefremov as code owners February 24, 2023 22:41
@github-actions github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/run-network-audit Run network-audit labels Feb 24, 2023
Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @DJAndries!! Love to see a diff with over 700 1500! lines net removed 😁

I'm gonna submit a followup review for specific APIs I think we can simplify further by removing more of the UTF-8 checks in places they shouldn't be necessary, but this should be enough to work with for now.

components/brave_shields/adblock/rs/Cargo.toml Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/rustfmt.toml Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/lib.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/lib.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/result.rs Outdated Show resolved Hide resolved
components/brave_shields/common/BUILD.gn Outdated Show resolved Hide resolved
ios/browser/api/brave_shields/adblock_engine.mm Outdated Show resolved Hide resolved
ios/browser/api/brave_shields/adblock_engine.h Outdated Show resolved Hide resolved
@antonok-edm antonok-edm requested a review from rillian March 3, 2023 06:16
@DJAndries
Copy link
Collaborator Author

will squash before future merge

Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

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

Followed up commenting on the safety of all the APIs returning a result type - should be ok to clean up most of them!

components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
components/brave_shields/adblock/rs/src/engine.rs Outdated Show resolved Hide resolved
@DJAndries
Copy link
Collaborator Author

Rebased and squashed

@DJAndries DJAndries force-pushed the adblock-cxx branch 2 times, most recently from a9a2053 to cbdb136 Compare July 6, 2023 01:05
@antonok-edm
Copy link
Collaborator

looks like this is just about ready to go - @DJAndries do you mind rebasing? it looks like the only CI failure is from unrelated npm audits.

cc @iefremov, we'll need your +1 too

(ctx->method == "GET" || ctx->method == "HEAD" ||
ctx->method == "OPTIONS")) {
ctx->new_url_spec = rewritten_url;
ctx->new_url_spec = std::string(adblock_result.rewritten_url.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can put this string into a variable

Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

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

browser/net lgtm

# License, v. 2.0. If a copy of the MPL was not distributed with this file,
# You can obtain one at https://mozilla.org/MPL/2.0/.

group("rust_lib") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not the correct places for this directory. If we're adding support for things that will be used in ios then you should be using browser/core and browser/content to split things that can/cannot be used on ios

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/run-network-audit Run network-audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use cxx for adblock-rust FFI
7 participants