[webkit-reviews] review granted: [Bug 232262] Add helper classes and messaging infrastructure to launch webpushd and round trip a message to it : [Attachment 442453] Patch v6

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 232262: Add helper classes and messaging infrastructure to launch webpushd
and round trip a message to it
https://bugs.webkit.org/show_bug.cgi?id=232262

Attachment 442453: Patch v6

https://bugs.webkit.org/attachment.cgi?id=442453&action=review




--- Comment #8 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 442453
  --> https://bugs.webkit.org/attachment.cgi?id=442453
Patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=442453&action=review

>
Source/WebKit/NetworkProcess/Notifications/Cocoa/WebPushDaemonConnectionCocoa.m
m: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(ReplyAr
gs...)>&& 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?