BSK support for FB CTL update #760
Conversation
ladamski
force-pushed
the
la/clicktoload-redux
branch
from
0402bab
to
a4ec422
Compare
ladamski
changed the title
| @@ -221,7 +221,7 @@ fileprivate extension KnownTracker.Rule { | |||
| private extension KnownTracker.ActionType { | |||
|
|
|||
| func toTrackerResolverRuleAction() -> TrackerResolver.RuleAction { | |||
| self == .block ? .blockRequest : .allowRequest | |||
| self == .block || self == .blockCtlFB ? .blockRequest : .allowRequest | |||
There was a problem hiding this comment.
What about line 209 above? We might also need to address the custom action here?
There was a problem hiding this comment.
I'm not sure what is this comment about, perhaps this is some old conversation you had with Bartek?
There was a problem hiding this comment.
btw there is inconsistency in naming, you often use CTL and in this case there is Ctl, please make sure we use it capitalized everywhere (unless the var starts with ctl, in such case use all lowercased)
There was a problem hiding this comment.
Yeah please disregard that comment, that's for a previous discussion/rev of the PR.
For the naming issue, this was just landed in duckduckgo/TrackerRadarKit#19, not sure the effort is worth the squeeze here?
If we do want to address it, maybe it makes more sense to follow the more conventional approach of camelCasing acronyms for all instances of CTL (i.e any acronym of 3+ chars). That would better match the rest of our code I think? Or I can also update the TRK PR, just LMK which way you prefer.
There was a problem hiding this comment.
We got a rule (that is obviously not followed everywhere) https://github.com/duckduckgo/swift?tab=readme-ov-file#capitalize-acronyms - so it would be awesome to fix it
There was a problem hiding this comment.
@jaceklyp cool, fixed! FWIW I don't have access to that repo (/swift), any reason it can't be public? Seems like something that would benefit from broad visibility.
There was a problem hiding this comment.
Related TRK PR: duckduckgo/TrackerRadarKit#22
ladamski
force-pushed
the
la/clicktoload-redux
branch
from
20e59fa
to
717db16
Compare
|
This PR has been inactive for more than 7 days and will be automatically closed 7 days from now. |
|
This PR has been closed after 14 days of inactivity. Feel free to reopen it if you plan to continue working on it or have further discussions. |
|
This PR has been inactive for more than 7 days and will be automatically closed 7 days from now. |
| @@ -221,7 +221,7 @@ fileprivate extension KnownTracker.Rule { | |||
| private extension KnownTracker.ActionType { | |||
|
|
|||
| func toTrackerResolverRuleAction() -> TrackerResolver.RuleAction { | |||
| self == .block ? .blockRequest : .allowRequest | |||
| self == .block || self == .blockCtlFB ? .blockRequest : .allowRequest | |||
There was a problem hiding this comment.
I'm not sure what is this comment about, perhaps this is some old conversation you had with Bartek?
| @@ -221,7 +221,7 @@ fileprivate extension KnownTracker.Rule { | |||
| private extension KnownTracker.ActionType { | |||
|
|
|||
| func toTrackerResolverRuleAction() -> TrackerResolver.RuleAction { | |||
| self == .block ? .blockRequest : .allowRequest | |||
| self == .block || self == .blockCtlFB ? .blockRequest : .allowRequest | |||
There was a problem hiding this comment.
btw there is inconsistency in naming, you often use CTL and in this case there is Ctl, please make sure we use it capitalized everywhere (unless the var starts with ctl, in such case use all lowercased)
Tests/BrowserServicesKitTests/ContentBlocker/WebViewTestHelper.swift
Outdated
Show resolved
Hide resolved
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/72649045549333/1205105078450227/f
iOS PR: TBD (hopefully N/A)
macOS PR: https://github.com/duckduckgo/macos-browser/pull/2162
What kind of version bump will this require?: Major/Minor/Patch
Optional:
Tech Design URL: https://app.asana.com/0/1201720254973470/1206422390411022/f
CC:
Description:
Updates BSK to support separate clickToLoad rule list, with associated surrogates changes. CTL is disabled for iOS
Steps to test this PR:
1.
1.
OS Testing:
Internal references:
Software Engineering Expectations
Technical Design Template