-
Notifications
You must be signed in to change notification settings - Fork 868
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
antonok-edm
force-pushed
the
adblock-custom-subscriptions
branch
from
March 4, 2021 17:41
ae01800
to
bb1fce6
Compare
components/brave_shields/browser/ad_block_subscription_service_manager.cc
Outdated
Show resolved
Hide resolved
components/brave_shields/browser/ad_block_subscription_service_manager.cc
Outdated
Show resolved
Hide resolved
components/brave_shields/browser/ad_block_subscription_service_manager.h
Outdated
Show resolved
Hide resolved
antonok-edm
force-pushed
the
adblock-custom-subscriptions
branch
from
March 8, 2021 19:24
bb1fce6
to
a6f4897
Compare
antonok-edm
force-pushed
the
adblock-custom-subscriptions
branch
3 times, most recently
from
March 24, 2021 18:43
c080618
to
d52e83d
Compare
antonok-edm
force-pushed
the
adblock-custom-subscriptions
branch
2 times, most recently
from
March 31, 2021 16:25
51fd40b
to
446d554
Compare
antonok-edm
force-pushed
the
adblock-custom-subscriptions
branch
7 times, most recently
from
April 7, 2021 03:00
93e9029
to
afebdfb
Compare
antonok-edm
force-pushed
the
adblock-custom-subscriptions
branch
from
April 7, 2021 17:16
afebdfb
to
9790400
Compare
antonok-edm
force-pushed
the
adblock-custom-subscriptions
branch
3 times, most recently
from
April 7, 2021 17:49
18075d3
to
1af3073
Compare
unrelated intermittent test failure |
24 tasks
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
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 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 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves brave/brave-browser#8107
To reduce the complexity associated with this change, I am leaving some potential improvements for followup PRs:
Title
,Homepage
,Expires
,Version
) and display theTitle
rather than URL if presentHomepage
Expires
field to support update intervals other than 7 daysabp:subscribe
protocol for adding new subscriptionsSubmitter 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
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
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!
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.brave://adblock
tab instances, and also across browser restarts.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.[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 thantext/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 theSyntaxes
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 is604800000000
microseconds, so subtracting604800000000
from both numbers will make the browser think the last download for that subscription occurred 1 week before it actually did.