[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 bugzilla-daemon at webkit.org
Tue Oct 5 07:21:52 PDT 2021

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

Attachment 439999: Patch


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

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.

> +void
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
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

nit: extra space before data.

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

duplicate statement.

More information about the webkit-reviews mailing list