Update Content Blocking to use BSK stack #354
Conversation
| userScriptsUpdatedCancellable = scriptsSource.sourceUpdatedPublisher.receive(on: RunLoop.main).sink { [weak self] _ in | ||
| guard let self = self, self.delegate != nil else { return } | ||
|
|
||
| self.userScripts = UserScripts(with: self.scriptsSource) |
There was a problem hiding this comment.
We could check the diff of the rules here (e.g. for the presence of unprotected sites changes) and reload the tab here, instead of doing that from Dashboard (but it would require figuring out if current tab is the one for which change has been triggered, or if it matches the unprotected sites change).
There was a problem hiding this comment.
+1
Not sure if it helps, but active tab state can be easily determined with webview.superview != nil
There was a problem hiding this comment.
We still need to know if that tab is relevant to the change - hard to do that now, probably should be tackled elsewhere.
|
|
||
| self.reload(knownChanges: newRulesInfo.changes) | ||
| }) | ||
| contentBlockingRulesUpdatedCancellable = cancellable |
There was a problem hiding this comment.
I've refactored the event flow, as Scripts depend on the state of the Content Blocking & Config layers, so source provider now listens to changes and sends sigal once sources are updated. Tabs listen for this and reinstantiate scripts.
| @@ -26,7 +26,7 @@ struct PrivacySecurityPreferences { | |||
| @UserDefaultsWrapper(key: .gpcEnabled, defaultValue: true) | |||
| public var gpcEnabled: Bool { | |||
| didSet { | |||
| DefaultScriptSourceProvider.shared.reload() | |||
| DefaultScriptSourceProvider.shared.reload(knownChanges: nil) | |||
There was a problem hiding this comment.
This is just 2nd place where DefaultScriptSourceProvider.shared is used so we could make it regular instance instead of singleton if we were to refactor this code into some event-driven logic.
|
|
||
| let majorTrackerThresholdPrevalence = 25.0 | ||
| let parentEntity = ContentBlocking.trackerDataManager.trackerData.findEntity(forHost: host) | ||
| let isMajorTrackingNetwork = (parentEntity?.prevalence ?? 0.0) >= majorTrackerThresholdPrevalence |
There was a problem hiding this comment.
This business logic has been moved from TrackerRadarManager.
| self.sendPendingUpdates() | ||
|
|
||
| let activeTab = self.tabViewModel?.tab | ||
| activeTab?.reload() |
There was a problem hiding this comment.
There are some bugs related to fact that we allow user to change tabs after toggling protection status:
- Current Tab can be closed - then browser crashes.
- Current Tab can be changed - then
reload()is called for wrong tab.
These issues should be handled in another task.
|
|
||
| final class PrivacyDashboardViewController: NSViewController { | ||
|
|
||
| @IBOutlet var webView: WKWebView! | ||
| private let privacyDashboardScript = PrivacyDashboardUserScript() | ||
| private var cancellables = Set<AnyCancellable>() | ||
| @Published var pendingUpdates = Set<String>() | ||
| @Published var pendingUpdates = [String: String]() |
There was a problem hiding this comment.
It is not possible to have multiple pending updates per current logic (toggle is disabled when processing change), but I preserved that capability in the code to not make too many changes here.
bwaresiak
changed the title
bwaresiak
force-pushed
the
bartek/update-content-blocking
branch
from
dc211c0 to
9102a99
Compare
There was a problem hiding this comment.
@bwaresiak, I promised this to be reviewed today, but I need few more minutes to review this tomorrow. Code architecture looks very good, I can see you cared. Don't expect any major changes after the second review tomorrow
| @@ -429,29 +430,27 @@ extension URL { | |||
| } | |||
|
|
|||
| // MARK: - GPC | |||
|
|
|||
| static func gpcHeadersEnabled(config: PrivacyConfiguration) -> [String] { | |||
There was a problem hiding this comment.
To be honest, these two methods are weird. They are static on URL, but the main point is to look into the instance of PrivacyConfiguration. I would define them there. Definitely not scope of this PR, I added a task into my tech debt list.
There was a problem hiding this comment.
Definitely good idea, I didn't want to touch "too much" to not add to the scope creep.
| @@ -19,7 +19,7 @@ | |||
| import WebKit | |||
| import BrowserServicesKit | |||
|
|
|||
| final class ContentBlockerRulesUserScript: NSObject, UserScript { | |||
| final class OldContentBlockerRulesUserScript: NSObject, UserScript { | |||
There was a problem hiding this comment.
The reference to this file is removed from Xcode but the file is still in ContentBlocker folder.
There was a problem hiding this comment.
These are tests that I used to verify the functionality:
- Tracker blocking: https://privacy-test-pages.glitch.me/index.html
- Surrogates: https://efficient-zest-side.glitch.me/
- GPC: https://globalprivacycontrol.org/
- Unprotected sites and toggling protection: switching the protection on/off in the privacy dashboard
- Temporary list exceptions: Editing bundled macos-config.json and waiting until the downloaded version is applied
- Tracker Allowlist exceptions: Run tests in BSK using "swift test" (Failed to load a local file when tests were running from Xcode)
- TDS and PrivacyConfig updates: Combination of breakpoints, removal of the local configuration from ~/Library/Containers/com.duckduckgo.macos.browser.debug/Data/Library/Application Support/Configuration and looking whether tracker allowlist exceptions were applied
The only issue discovered is we are not passing information about temporary unprotected site to the user. It is not part of the scope of this PR
Bartek, this is an excelent work! 🎖️ 🏅 I'm very happy we have another pair of eyes looking at our codebase. Thanks for all suggestions, I took notes and will add follow up tasks into our macOS Development board.
LGTM! 💯
Task/Issue URL:
Tech Design URL:
CC:
Description:
Move Content Blocking stack from iOS to BSK and introduce it to Mac OS browser.
This is sibling PR to the one from BSK: duckduckgo/BrowserServicesKit#35
Steps to test this PR:
All privacy related features should be carefully tested (see checklist below) and checked against privacy test pages.
Testing checklist:
Privacy features:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM