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

Unify regional and default list formats #103

Merged
merged 3 commits into from
Apr 13, 2023

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Apr 6, 2023

This will be a breaking change, although it should help us to remove duplicated logic across several repos, and will allow us to bundle multiple list sources in each regional component going forwards.

To summarize the changes:

  • title, url, and support_url are moved into individual objects within a sources list for each component
  • The default list now has its component information defined in the same file, rather than hardcoded in other repos which depend on it
  • sources is a list so it can now support multiple list sources bundled into the same component
  • default.json and regional.json now share the exact same structure

Code that depends on these files:

adblock-resources

Tests in this repo were updated in this PR.

brave-core-crx-packager

Needs to be updated to pull potentially multiple URLs into each list. There are multiple codepaths dedicated to building the default component and the regional components, which can be deduplicated.

TODO

brave-core

regional_catalog.json, which is shipped directly from filter_lists/regional.json in a component, gets parsed into a vector of FilterListCatalogEntry structs by Chromium's JSONValueConverter.

According to the comment documentation:

// Convert() returns false when it fails.  Here "fail" means that the value is
// structurally different from expected, such like a string value appears
// for an int field.  Do not report failures for missing fields.
// Also note that Convert() will modify the passed |message| even when it
// fails for performance reason.

...it appears that the removal of url and support_url from the top-level filter list object should not cause any issues here, as per Do not report failures for missing fields. Those fields are defined in the parsed struct, but not used in the browser.

TODO

  • verify that brave-core works with the new regional.json format

If for some reason it doesn't work, we can have brave-core-crx-packager repackage it with the problematic fields added back, then update brave-core to migrate to a new component with the newer format.

brave-ios

brave-ios uses the same code as brave-core to download the regional catalog component and parse the fields. It also doesn't use the url or support_url fields. If brave-core works, then brave-ios should work too.

slim-list-lambda

The assemble.js script references the regional catalog from this repo and needs to be updated to handle potentially multiple URLs.

TODO

adblock-rust

tests/live.rs uses regional.json. It's lower priority, but this should also be updated to work with the newer format.

TODO

Copy link
Collaborator

@pes10k pes10k left a comment

Choose a reason for hiding this comment

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

terrific!

@ShivanKaul
Copy link
Collaborator

This is really helpful!

@antonok-edm antonok-edm merged commit 712e0b6 into master Apr 13, 2023
@antonok-edm antonok-edm deleted the unify-regional-default-format branch April 13, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants