Summary: | Add helper classes and messaging infrastructure to launch webpushd and round trip a message to it | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | Brady Eidson <beidson> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Brady Eidson
2021-10-25 13:21:46 PDT
Created attachment 442439 [details]
Patch v1
Created attachment 442442 [details]
Patch v2
Created attachment 442446 [details]
Patch v3
Comment on attachment 442446 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=442446&action=review > Source/WebKit/NetworkProcess/Notifications/Cocoa/WebPushDaemonConnectionCocoa.mm:39 > +namespace WebKit { > +namespace WebPushD { namespace WebKit::WebPushD Isn't C++17 great? > Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:41 > + printf("Created new NetworkNotificationManager with webPushMachServiceName: %s\n", webPushMachServiceName.utf8().data()); Please omit > Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:373 > +;; FIXME: Settle on real name > +(allow mach-lookup (with telemetry) (global-name "org.webkit.webpushtestdaemon.service")) This name is already in the plist: com.apple.webkit.webpushd.service Created attachment 442448 [details]
Patch v4
Created attachment 442451 [details]
Patch v5
Created attachment 442453 [details]
Patch v6
Comment on attachment 442453 [details] Patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=442453&action=review > Source/WebKit/NetworkProcess/Notifications/Cocoa/WebPushDaemonConnectionCocoa.mm:38 > +using Daemon::EncodedMessage; > + > +namespace WebKit::WebPushD { put using inside the namespace scope to avoid future source unification problems > Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:108 > +template<WebPushD::MessageType messageType, typename... Args, typename... ReplyArgs> > +void NetworkNotificationManager::sendMessageWithReply(CompletionHandler<void(ReplyArgs...)>&& completionHandler, Args&&... args) const You will probably want this in the header so you won't need to explicitly instantiate each template instance. > Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:373 > +(allow mach-lookup (with telemetry) (global-name "org.webkit.webpushtestdaemon.service")) We should eventually use a sandbox extension handle instead of putting this in the production sandbox, but this follows an existing pattern. Maybe add a FIXME comment to remove all of them. > Source/WebKit/Shared/WebPushDaemonConstants.h:42 > +inline bool messageTypeSendsReply(MessageType messageType) This doesn't need to be inline but can. > Source/WebKit/webpushd/WebPushDaemon.mm:98 > + // FIXME: Verify the connection is known I don't think this is necessary. It's impossible to receive from a connection you haven't been informed of. But you will need to keep track of connections, as noted below. > Tools/MiniBrowser/mac/AppDelegate.m:104 > + // [configuration setWebPushMachServiceName:@"org.webkit.webpushtestdaemon.service"]; I don't think we'll want this code here. You can use it locally for your testing, but not in everyone's MiniBrowser. > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:219 > + NSLog(@"Granting notifications permission to origin %@", securityOrigin); > + decisionHandler(YES); Are you sure you don't want a dialog? (In reply to Alex Christensen from comment #8) > Comment on attachment 442453 [details] > Patch v6 > > > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:219 > > + NSLog(@"Granting notifications permission to origin %@", securityOrigin); > > + decisionHandler(YES); > > Are you sure you don't want a dialog? Would require adding a (at least in-memory) permissions manager to support that. Added a FIXME Created attachment 442513 [details]
PFL
Committed r284887 (243564@main): <https://commits.webkit.org/243564@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 442513 [details]. |