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

Support custom filter list subscriptions in brave://adblock #8124

Merged
merged 73 commits into from
Sep 1, 2021

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Mar 4, 2021

Resolves brave/brave-browser#8107

To reduce the complexity associated with this change, I am leaving some potential improvements for followup PRs:

  • Read list metadata (e.g. Title, Homepage, Expires, Version) and display the Title rather than URL if present
  • Add a button to visit the subscription's listed Homepage
  • Use Expires field to support update intervals other than 7 days
  • Compile the list into a DAT and load that at startup rather than recompiling every time
  • Only initialize the download manager if at least one subscription has been added
  • UI polish (likely along with the rest of brave://adblock)
  • Support abp:subscribe protocol for adding new subscriptions

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
  • 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, npm run lint, 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:

There's a lot of behavior here; some parts are easier to test than others. In some cases I've described general expected behavior rather than concrete steps. Please reach out if anything is unclear!

  1. Visit brave://adblock
  2. Verify that a "Subscribe to filter lists" section exists between the "Additonal Filters" and "Custom Filters" sections, including an "Add filter list via URL" button.
  3. Click on the button to create a new subscription. A textbox should appear, along with a disabled "Submit" button and a "Cancel" button.
  4. The "Submit" button should remain disabled as long as the text box does not include a valid URL. Pressing enter from the textbox should also have no effect whenever the button is disabled.
  5. Submitting a correctly formatted URL that points to an invalid filter list document[1] should display a table with labeled "Filter list" and "Last updated" columns, as well as a "triple dot" menu at the right side. The table should have a single row for the new subscription, with its URL. Once the download has failed or timed out, the "Last updated" field should show the text "Download failure" in red, with the last download attempt time available on mouse hover. The "triple dot" menu should be expandable and collapsible, presenting options for "Update now" and "Unsubscribe". Pressing "Update now" should cause the "Download failure" hover text to update accordingly, but should not change the entry otherwise. Pressing "Unsubscribe" should remove the entry from the table, removing the table altogether if it is the last entry.
  6. Submitting a correctly formatted URL that points to a valid filter list document[2] should create a table row with the list's URL, last updated time, a toggleable checkbox, and the triple dot menu. Once the download has succeeded, the last updated time should display "less than a minute ago", and the hover text should show the most recent update time. The triple dot menu should show options for "Update now", "View source", and "Unsubscribe" when expanded. Clicking "View source" should open a file:// URL in a new tab, showing a locally cached version of the content available from the original subscription URL. Pressing "Update now" should cause the "Last updated" column text and hover info to be updated accordingly. Pressing "Unsubscribe" should remove the entry from the table, removing the table altogether if it is the last entry.
  7. The information in the table should remain consistent across different brave://adblock tab instances, and also across browser restarts.
  8. It should not be possible to have more than one filter list entry with any given subscription URL.
  9. The contents of any list with a checkbox toggled ON should be applied by the adblock engine during browsing[3]. If that list's checkbox is toggled OFF, the list should not be applied during browsing.
  10. Pressing "Update now" for a list that previously succeeded but is no longer a valid list[1] should display Download failure since ... in red, showing the last successful update and last attempted update times on hover. A simple way to simulate this is by disconnecting from the network before attempting an update. In this state, the most recent successfully downloaded version should still be applied during browsing and visible using the "View source" option. A later successful update should bring it back out of this state.
  11. Any subscriptions that have not been toggled OFF should be subject to automatic updates. A list whose last update attempt was successful, whether manual or automatic, should be automatically updated once it is at least 7 days old[4]. A list whose last update attempt was not successful should be automatically retried hourly until success. Update checks for all lists always occur ~1 minute after launching the browser and subsequently every hour.

[1] Examples of URLs that will produce a failure case include: nonexistent domain, server that is not responding, HTTP response other than 200, mimetype other than text/html.
[2] Plenty of these can be found at https://filterlists.com. Any of those lists should download successfully, although it may be preferable to filter by Adblock Plus in the Syntaxes column to find "useful" lists.
[3] I've hosted a simple example: create a subscription to https://antonok.com/tmp/list.txt and observe that https://example.com is mostly red. That list includes a single rule: example.com,antonok.com##*:style(background: red).
[4] Better get started in advance! Just kidding... For testing purposes, you can modify the Local State file in your profile directory while the browser is closed. This file is a JSON document; if you search for a particular list URL you've entered you should be able to find something of the form "https://antonok.com/tmp/list.txt":{"enabled":true,"last_successful_update_attempt":"13274404478321439","last_update_attempt":"13274404478321439"}. The numbers are timestamps in microseconds, you can manually edit them to be in the past. For reference, 1 week is 604800000000 microseconds, so subtracting 604800000000 from both numbers will make the browser think the last download for that subscription occurred 1 week before it actually did.

@antonok-edm antonok-edm force-pushed the adblock-custom-subscriptions branch 3 times, most recently from c080618 to d52e83d Compare March 24, 2021 18:43
@antonok-edm antonok-edm force-pushed the adblock-custom-subscriptions branch 2 times, most recently from 51fd40b to 446d554 Compare March 31, 2021 16:25
@antonok-edm antonok-edm force-pushed the adblock-custom-subscriptions branch 7 times, most recently from 93e9029 to afebdfb Compare April 7, 2021 03:00
@antonok-edm antonok-edm added the CI/skip Do not run CI builds (except noplatform) label Apr 7, 2021
@antonok-edm antonok-edm force-pushed the adblock-custom-subscriptions branch 3 times, most recently from 18075d3 to 1af3073 Compare April 7, 2021 17:49
@bridiver
Copy link
Collaborator

bridiver commented Sep 1, 2021

unrelated intermittent test failure

@bridiver bridiver merged commit db5da23 into master Sep 1, 2021
@bridiver bridiver deleted the adblock-custom-subscriptions branch September 1, 2021 15:59
@antonok-edm antonok-edm added this to the 1.31.x - Nightly milestone Sep 1, 2021
mariospr added a commit that referenced this pull request Sep 3, 2021
This was initially needed as per some changes that were going to be
added as part of PR #8124[1] (in commit 8ba9ddb[2]), that used
util::WallClockTimer, but the merged PR ended up implementing things
differently and this dependency was left around in the BUILD.gn file.

Since //base/util/timer does not exist anymore in Chromium 94 (i.e it
has been moved to //base/timer), this is breaking the build now so
let's remove the stray dependency.

[1] #8124
[2] 8ba9ddb5
mariospr added a commit that referenced this pull request Sep 3, 2021
This was added as part of PR #8124[1] (in commit 1ab371c[2]) but then
removed lated in the same PR before merging (commit be1073b[3]),
leaving the include around without being needed, so let's drop it.

[1] #8124
[2] 1ab371c4
[3] be1073be
mkarolin pushed a commit that referenced this pull request Sep 7, 2021
This was initially needed as per some changes that were going to be
added as part of PR #8124[1] (in commit 8ba9ddb[2]), that used
util::WallClockTimer, but the merged PR ended up implementing things
differently and this dependency was left around in the BUILD.gn file.

Since //base/util/timer does not exist anymore in Chromium 94 (i.e it
has been moved to //base/timer), this is breaking the build now so
let's remove the stray dependency.

[1] #8124
[2] 8ba9ddb5
mkarolin pushed a commit that referenced this pull request Sep 7, 2021
This was added as part of PR #8124[1] (in commit 1ab371c[2]) but then
removed lated in the same PR before merging (commit be1073b[3]),
leaving the include around without being needed, so let's drop it.

[1] #8124
[2] 1ab371c4
[3] be1073be
mkarolin pushed a commit that referenced this pull request Sep 11, 2021
This was initially needed as per some changes that were going to be
added as part of PR #8124[1] (in commit 8ba9ddb[2]), that used
util::WallClockTimer, but the merged PR ended up implementing things
differently and this dependency was left around in the BUILD.gn file.

Since //base/util/timer does not exist anymore in Chromium 94 (i.e it
has been moved to //base/timer), this is breaking the build now so
let's remove the stray dependency.

[1] #8124
[2] 8ba9ddb5
mkarolin pushed a commit that referenced this pull request Sep 11, 2021
This was added as part of PR #8124[1] (in commit 1ab371c[2]) but then
removed lated in the same PR before merging (commit be1073b[3]),
leaving the include around without being needed, so let's drop it.

[1] #8124
[2] 1ab371c4
[3] be1073be
mkarolin pushed a commit that referenced this pull request Sep 13, 2021
This was initially needed as per some changes that were going to be
added as part of PR #8124[1] (in commit 8ba9ddb[2]), that used
util::WallClockTimer, but the merged PR ended up implementing things
differently and this dependency was left around in the BUILD.gn file.

Since //base/util/timer does not exist anymore in Chromium 94 (i.e it
has been moved to //base/timer), this is breaking the build now so
let's remove the stray dependency.

[1] #8124
[2] 8ba9ddb5
mkarolin pushed a commit that referenced this pull request Sep 13, 2021
This was added as part of PR #8124[1] (in commit 1ab371c[2]) but then
removed lated in the same PR before merging (commit be1073b[3]),
leaving the include around without being needed, so let's drop it.

[1] #8124
[2] 1ab371c4
[3] be1073be
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.

Support for custom filter lists in Shields
4 participants