-
Notifications
You must be signed in to change notification settings - Fork 895
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
[Android] default filter list settings #19670
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
<message name="IDS_DEFAULT_FILTER_LISTS" desc="Title for Default filter lists."> | ||
Default filter lists | ||
</message> | ||
<message name="IDS_DEFAULT_FILTER_LISTS_SUMMARY" desc="Summary text for Default filter lists."> | ||
Additional popular community lists. Note that enabling too many filters will degrade browsing speeds. | ||
</message> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about calling these "default" filter lists, since they are generally off by default. We already reserve that naming for the on-by-default lists (although we don't show it in the UI anywhere).
These lists come from the "regional list catalog"†. Ever since brave://adblock
was replaced by brave://settings/shields/filters
on Desktop, they're just labeled "Filter lists".
My recommendation is to follow the naming on Desktop (Filter lists
). The title of the overall settings page can also be renamed to match accordingly (Content Filters
). I'd also update the code to refer to them as "regional lists" (e.g. FilterListService::GetRegionalFilterLists
).
† which is no longer only for "regional" lists, to make things even more confusing 😂
this needs some refactoring, but it'll be easier to do it all at once if the naming is consistent in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timchilds @deeppandya what do you think? Should we follow the desktop or ios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following desktop makes sense to me. @anthonypkeane what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should remove the default and just keep the filter list as a copy.
bc05616
to
c69ab93
Compare
android/java/org/chromium/chrome/browser/shields/ContentFilteringAdapter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
f9d6368
to
e6863eb
Compare
Verification PASSED on
Using the STR/Cases outlined via #19670 (comment) and the information outlined via brave/brave-browser#26587, ensured the following:
|
Uplift of #19670 (squashed) to beta
Resolves brave/brave-browser#26587
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: