[webkit-reviews] review granted: [Bug 233988] Add ability to inject messages into webpushd : [Attachment 446405] Patch v3

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 233988: Add ability to inject messages into webpushd
https://bugs.webkit.org/show_bug.cgi?id=233988

Attachment 446405: Patch v3

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




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

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

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:94
> +    CompletionHandler<void(Vector<WebPushMessage>&&)> replyHandler =
[completionHandler = WTFMove(completionHandler)] (Vector<WebPushMessage>
messages) mutable {

This looks like an unneeded Vector copy.  Can we just pass a Vector&& and avoid
this wrapper too?

> Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:2
> + * Copyright (C) 2010-2020 Apple Inc. All rights reserved.

2021

> Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:47
> +	   Vector<uint8_t> { (const uint8_t*)pushData.bytes, pushData.length },

static_cast

> Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:48
> +	   URL { (NSURL *)url }

unnecessary cast

> Source/WebKit/webpushd/PushClientConnection.mm:126
> +	   messageIdentifier = makeString("[", signingIdentifer, " (0x",
hex(reinterpret_cast<uint64_t>(m_xpcConnection.get()),
WTF::HexConversionMode::Lowercase), ")] ");

nit: I think "[" can be '['

> Source/WebKit/webpushd/WebPushDaemon.mm:369
> +	       resultMessages.append(WebKit::WebPushMessage { Vector<uint8_t> {
reinterpret_cast<const uint8_t*>(data.data()), data.length() },
message.registrationURL });

nit: I wonder if you could use a Span instead of a Vector here because all you
are doing is sending it.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:235
> +    >(utf8.length() >> 24);

line break is probably a typo