Bug 232262 – Add helper classes and messaging infrastructure to launch webpushd and round trip a message to it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Add helper classes and messaging infrastructure to launch webpushd and round trip a message to it

Helpful as a standalone patch before we start using it for things.

[Brady Eidson]

Created attachment 442439 [details]
Patch v1

[Brady Eidson]

Created attachment 442442 [details]
Patch v2

[Brady Eidson]

Created attachment 442446 [details]
Patch v3

[Alex Christensen]

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

[Brady Eidson]

Created attachment 442448 [details]
Patch v4

[Brady Eidson]

Created attachment 442451 [details]
Patch v5

[Brady Eidson]

Created attachment 442453 [details]
Patch v6

[Alex Christensen]

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?

[Brady Eidson]

(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

[Brady Eidson]

Created attachment 442513 [details]
PFL

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/84676890>