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 default.json and regional.json into list_catalog.json #130

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Aug 30, 2023

Required for brave/brave-core#19946
Should be merged along with brave/brave-core-crx-packager#687

Note the 4 new optional fields:

  • hidden: boolean should the component be displayed in brave://settings/shields/filters as an additional list option?
  • default_enabled: boolean should the component be enabled by default?
  • first_party_protections: boolean should first-party heuristics be applied to rules from the list?
  • permission_mask: number what scriptlet permissions should be granted to rules from the list?

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! What changes are needed to support this in Android and iOS then too (so that those platforms know to respect, hidden, default_enabled and first_party_protections?

@antonok-edm
Copy link
Collaborator Author

Android should "just work"; the UI is using a method that retrieves only the visible items form the component catalog and everything else shares the same mechanics with Desktop.

iOS will continue using the older regional_catalog.json file (backwards-compatibly generated in brave-core-crx-packager) until it is updated to match the brave-core implementation.

@ryanbr
Copy link
Collaborator

ryanbr commented Aug 31, 2023

Is added custom lists and/or brave://adblock rules affected by this?

@antonok-edm
Copy link
Collaborator Author

@ryanbr nope, hidden: true ensures the default lists are not visible in brave://adblock and custom rules or subscriptions are unaffected

- `hidden` (`boolean`) should the component be displayed in brave://settings/shields/filters as an additional list option?
- `default_enabled` (`boolean`) should the component be enabled by default?
- `first_party_protections` (`boolean`) should first-party heuristics be applied to rules from the list?
- `permission_mask` (`number`) what scriptlet permissions should be granted to rules from the list?
Copy link

@bridiver bridiver Sep 18, 2023

Choose a reason for hiding this comment

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

I don't understand what a numeric permission value means in this context. It seems like this should be an enum? What are the possible values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a mask, as in "bitmask". There's 8 independent bits that can be set or unset. They don't necessarily have any fixed meaning at the moment, other than that we're using bit 0 for uBlock Origin's "trusted" resources and bit 1 for any Brave-specific sensitive resources.

This would indeed be good to document here.

Choose a reason for hiding this comment

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

well, on the cpp side we generally do that with an enum like

enum : uint8_t {
  kIsInactiveUser = 0,
  kIsDailyUser = (1 << 0),
  kIsWeeklyUser = (1 << 1),
  kIsMonthlyUser = (1 << 2),
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

neither brave-core nor adblock-rust actually attempt to prescribe any meaning to any of the bits; they just apply bitwise operations and allow or deny permission accordingly. The permissions are both required and granted from within this repository.

Choose a reason for hiding this comment

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

well, in any case this needs some explanation about what is going on (like what we discussed in DM) because right now it's really not clear at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added context to the README

@antonok-edm antonok-edm merged commit 5c44e08 into master Sep 19, 2023
4 checks passed
@antonok-edm antonok-edm deleted the unified-catalog branch September 19, 2023 19:35
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.

4 participants