search

brave / adblock-rust-ffi

An FFI crate to expose functionality from brave/adblock-rust
Mozilla Public License 2.0
46 stars 16 forks source link

Using embedded adblock-rust resolver #35

Open ernestask opened 3 years ago

ernestask commented 3 years ago

Hello,

I suppose this is more a question than a specific complaint, but what’s the recommendation wrt the embedded-domain-resolver feature? After psl was ripped out, it became a requirement to provide an external one (barring hacking the bindings to enable the feature), but I’m not quite sure if I should just start using psl myself.

antonok-edm commented 3 years ago

@ernestask are you using adblock-rust-ffi for another project? That would be good to know, because we are considering making this FFI more tightly coupled with Brave and just including it in the brave-core repo instead of being standalone.

Otherwise I'm happy to give advice on domain resolution implementations at the adblock-rust API level - if you're using this outside of a browser, it definitely makes more sense to just enable the feature (as is currently done for the JS bindings). The community-maintained python bindings also take the same approach, even though I believe the primary use-case is within qutebrowser which presumably still has its own PSL-like mechanism regardless.

The option to disable the embedded-domain-resolver feature was added as an optimization that made Brave build faster, more deterministically, and with a smaller binary size, and also ruled out any subtle bugs that may have occurred as a result of one implementation differing slightly from the other. These are all extremely important when running CI and serving new releases to millions of users on a tight schedule, but obviously the same benefits might be more or less apparent depending on the scale of whatever project you're using adblock-rust in.

ernestask commented 3 years ago

@antonok-edm, hey, yeah, I work on a Go codebase that uses adblock-rust through these bindings (don’t ask why, because I’m largely inheriting that mess). The submodule is currently at an older version with psl still in use. As I tried to update it (for a lifeguard-related crash), I noticed other panics due to an external resolver not being set, hence this issue.

So, if your plans are to just include this wholesale in adblock-rust, I suppose I could look at other bindings or maintain my own? I don’t think we’ll suffer much either way, because this is a fairly small endeavor, but it just would have been nice to be able to stay level with upstream.

PS could you point me to the JS bindings?

antonok-edm commented 3 years ago

It'd definitely be possible to conditionally enable the embedded-domain-resolver feature based on a Makefile configuration from this FFI if you're interested in staying up to date with this repo; I'd be happy to accept a PR for that. Again, I can't guarantee that this repo will continue to be maintained, but if and when that time comes you're always welcome to fork it.

The JS bindings are directly in the adblock-rust repo. There's a package.json at the top level, and the code resides in the native directory, which is an unfortunate naming scheme but required by Neon.