[webkit-reviews] review denied: [Bug 233295] More webpushd architecture work : [Attachment 444641] EWS v5

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 18 11:51:33 PST 2021


Alex Christensen <achristensen at apple.com> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 233295: More webpushd architecture work
https://bugs.webkit.org/show_bug.cgi?id=233295

Attachment 444641: EWS v5

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




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 444641
  --> https://bugs.webkit.org/attachment.cgi?id=444641
EWS v5

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

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:45
> +	   auto cfData =
m_networkSession.networkProcess().sourceApplicationAuditData();

You can probably do this from sourceApplicationAuditToken to be slightly more
efficient.

> Source/WebKit/Shared/Cocoa/CodeSigning.mm:78
> +    auto pair = codeSigningIdentifierAndPlatformBinaryStatus(token);

This doesn't need to call SecTaskGetCodeSignStatus.

> Source/WebKit/webpushd/PushClientConnection.mm:42
> +void ClientConnection::setAuditTokenData(const Vector<uint8_t>& tokenData)

This is a little strange.  Isn't xpc_connection_get_audit_token enough?

> Source/WebKit/webpushd/PushClientConnection.mm:72
> +	   m_hostHasPushEntitlement = WTF::hasEntitlement(*m_hostAppAuditToken,
"com.apple.private.webkit.webpush");

This is typically done at the time a connection is being received, then
terminating the connection if it doesn't have an entitlement.  That way you
avoid bugs where you forgot to check the entitlement.  If you have a
connection, then it is entitled.

> Source/WebKit/webpushd/WebPushDaemon.h:56
> +    void echoTwice(ClientConnection*, const String&,
CompletionHandler<void(const String&)>&& replySender);

We can probably remove this at some point.

> Source/WebKit/webpushd/WebPushDaemon.h:77
> +    HashMap<xpc_connection_t, std::unique_ptr<ClientConnection>>
m_connectionMap;

It's a little strange that the connection is both in the key and the value.


More information about the webkit-reviews mailing list