-
Notifications
You must be signed in to change notification settings - Fork 28
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
Centralised Messaging for User Scripts #299
Conversation
c69cdbc
to
2685c2a
Compare
fb6c092
to
7965613
Compare
5f16e6a
to
f4ffc54
Compare
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
@ayoy @Bunn @jaceklyp this was just my way of making the new standardised messaging platform work with minimal changes to BSK (it just adds a new protocol, it doesn't alter any existing User Scripts) Let me know if it's the wrong the direction, or if you can suggest any parts to re-write :p - I'm really just interested in supporting the idea of 'multiple features per User Script' and ensuring it follows what we have documented in C-S-S. |
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.
Added some comments there, I think it looks good, but I haven't used UserScript before in a level that I feel comfortable knowing some of the impacts here for existing features. @jaceklyp do you mind taking a second look? And @ayoy as well because of the Duck Player messaging.
From what I'm seeing and using it for DBP, LGTM.
Edit: To be clear, I approved the PR but it doesn't mean it can be merged 😅 just approving the direction
Sources/BrowserServicesKit/ContentScopeScript/SpecialPagesUserScript.swift
Outdated
Show resolved
Hide resolved
Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift
Outdated
Show resolved
Hide resolved
Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift
Outdated
Show resolved
Hide resolved
Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift
Outdated
Show resolved
Hide resolved
Sources/BrowserServicesKit/ContentScopeScript/SpecialPagesUserScript.swift
Outdated
Show resolved
Hide resolved
Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift
Outdated
Show resolved
Hide resolved
|
||
public func toJSON() -> String? { | ||
|
||
// I added this because I couldn't figure out the generic/recursive Encodable |
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.
let's discuss it over MM
Hey Shane, thank you for this! 🙏 Added several comments - if you have any questions let's discuss them. I would like to give another round of review after your fixes. |
34c9240
to
b80a0a6
Compare
@jaceklyp - thanks for the feedback, please feel free to check this again. Because this work deliberately DOES NOT alter how existing User Scripts work, it should be fairly low-risk to merge and then iterate if we need to following further feedback of course :) |
@jaceklyp please have another look after @shakyShane has addressed your comments. |
Sources/BrowserServicesKit/ContentScopeScript/ContentScopeUserScript.swift
Outdated
Show resolved
Hide resolved
public let isIsolated: Bool | ||
public var messageNames: [String] = [] | ||
|
||
public init(_ privacyConfigManager: PrivacyConfigurationManaging, |
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 believe it shoud be init(privacyConfigManager, let's not hide the external label in init
self.broker = UserScriptMessageBroker(context: contextName) | ||
|
||
// dont register any handlers at all if we're not in the isolated context | ||
self.messageNames = self.isIsolated ? [contextName] : [] | ||
|
||
self.source = ContentScopeUserScript.generateSource( | ||
privacyConfigManager, | ||
properties: properties, | ||
isolated: self.isIsolated, | ||
config: self.broker.messagingConfig() |
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.
pretty much all selfs are not needed here
public func registerSubFeature(delegate: Subfeature) { | ||
delegate.with(broker: broker) | ||
broker.registerSubFeature(delegate: delegate) |
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.
Subfeature :) here and above
Great work, sir! |
@jaceklyp updated! thanks :) |
Task/Issue URL: https://app.asana.com/0/0/1204657577764249/f Tech Design URL: CC: **Description**: Update macOS to use latest BSK https://app.asana.com/0/0/1204544767438474/f Those changes to BSK are all additions - there are no functional changes to macOS in this PR, it's just keeping it up to date to allow ClickToLoad, DataBrokerProtection and DuckPlayer to use them. BSK PR: duckduckgo/BrowserServicesKit#299 **Steps to test this PR**: 1. No functional changes should be present - so just verifying everything still works as it did before <!-- Tagging instructions If this PR isn't ready to be merged for whatever reason it should be marked with the `DO NOT MERGE` label (particularly if it's a draft) If it's pending Product Review/PFR, please add the `Pending Product Review` label. If at any point it isn't actively being worked on/ready for review/otherwise moving forward (besides the above PR/PFR exception) strongly consider closing it (or not opening it in the first place). If you decide not to close it, make sure it's labelled to make it clear the PRs state and comment with more information. --> --- ###### Internal references: [Pull Request Review Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f) [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) [Pull Request Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f) --------- Co-authored-by: Shane Osbourne <sosbourne@duckduckgo.com>
* main: (45 commits) SecureVault Keychain updates (#382) Reattach orphaned bookmarks to root folder when reordering (#383) Allow for asynchronous sync initialization (#381) Drop regex support in param stripping (#367) mac promo exp2 - add support for share link message (#375) Revert accidentally pushed change Remove test that was dependant on the keychain Add item to keychain in the background (#380) macOS in-context signup updates (#359) Fixes the format in a log call (#379) Add EventMapping to DDGSync dependencies and call it from handleUnauthenticated (#376) Autofill hash migration updates + Bitwarden hotfix (#372) Autofill password generation support for iOS (#361) Update autofill to 7.1.0 (#370) Sync Engine with support for syncing bookmarks (#355) Centralised Messaging for User Scripts (#299) Update TLD accounts query to be more specific (#368) Bump Tests/BrowserServicesKitTests/Resources/privacy-reference-tests (#366) Adds a different hash Account per each macOS build kind (#363) adjust os_log parameter name using disfavoredOverload (#349) ...
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL:
iOS PR:
macOS PR:
What kind of version bump will this require?: Major/Minor/Patch
Optional:
TODO
tl;dr - multiple features will soon be messaging from single UserScripts
Description:
Content Scope Scripts is adding features that require messaging - there will be multiple such features in the future.
Therefor, on the JS side we've created a standardised system for messaging to and from WebViews that include enough information for the native platforms to distribute the messages as they see fit.
CC @jaceklyp
We have Specified a number of types that native platforms must implement and understand, such as
NotificationMessage
(a fire-and-forget message)RequestMessage
(a notification that requires a response)MessageResponse
(a response to aRequestMessage
)SubscriptionEvent
(ability to push data to WebViews)Messaging https://duckduckgo.github.io/content-scope-scripts/modules/Messaging.html
MessagingSchema https://duckduckgo.github.io/content-scope-scripts/modules/Messaging_Schema.html
Steps to test this PR:
1.
1.
OS Testing:
Internal references:
Software Engineering Expectations
Technical Design Template