[webkit-reviews] review granted: [Bug 231070] Implement https://w3c.github.io/push-api/#receiving-a-push-message : [Attachment 439999] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 231070: Implement https://w3c.github.io/push-api/#receiving-a-push-message
https://bugs.webkit.org/show_bug.cgi?id=231070

Attachment 439999: Patch

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




--- Comment #5 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 439999
  --> https://bugs.webkit.org/attachment.cgi?id=439999
Patch

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

> Source/WebCore/workers/service/server/SWServer.h:225
> +    enum class ShouldSkipEvent { No, Yes };

: bool

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2539
> +	   data = Vector<uint8_t> { ipcData->data(), ipcData->size()  };

nit: extra space before }

> Source/WebKit/NetworkProcess/NetworkProcess.h:400
> +    void processPushMessage(PAL::SessionID, const
std::optional<IPC::DataReference>&, URL&&, CompletionHandler<void(bool)>&&);

Would be good to not omit the bool parameter name as it is not obvious.

>
Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:1
10
> +void
WebSWServerToContextConnection::firePushEvent(WebCore::ServiceWorkerIdentifier
serviceWorkerIdentifier, const std::optional<Vector<uint8_t>>& data,
CompletionHandler<void(bool)>&& callback)

Would be good to not omit the bool parameter name as it is not obvious.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:756
> +	   data = Span<const uint8_t> { reinterpret_cast<const
uint8_t*>(message.bytes), message.length };

It doesn't look safe to wrap the provided NSData into a Span and do stuff
asynchronously. The lifetime of the NSData* is controller by the caller
(client) and it may be destroyed right after this call, leaving your Span
invalid.
We should be using a SharedBuffer, not a Span IMO. A SharedBuffer can wrap a
NSData and does not actually copy the data either.

The current code might be safe depending on how long you hold on to the Span
but it is fragile at best.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:108
> +-(void)_processPushMessage:(NSData*) data registration:(NSURL *)registration
completionHandler:(void(^)(bool))completionHandler
WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

nit: extra space before data.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PushAPI.mm:126
> +    done = false;

duplicate statement.