[webkit-reviews] review granted: [Bug 238608] IPC::Connection should support diverting all messages to a message queue in other thread : [Attachment 456354] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 238608: IPC::Connection should support diverting all messages to a message
queue in other thread
https://bugs.webkit.org/show_bug.cgi?id=238608

Attachment 456354: Patch

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




--- Comment #9 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 456354
  --> https://bugs.webkit.org/attachment.cgi?id=456354
Patch

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

>>> Source/WebKit/Platform/IPC/Connection.cpp:374
>>> +	 ReceiverMatcher receiverMatcher =
ReceiverMatcher::createForLegacyAPI(receiverName, destinationID);
>> 
>> How is this legacy?
> 
> Passing zero id to mean any id is legacy and new interfaces should use
receiver matcher to explicitly say if the match should apply for zero only or
any id.

Maybe instead of "legacy API", which is opaque unless you know the history of
this code, use something like createAllowingCatchAllDestination()

> Source/WebKit/Platform/IPC/Connection.h:247
> +    void removeWorkQueueMessageReceiver(ReceiverName, uint64_t destinationID
= 0);

Why don't we use std::optional<destinationID> here to match ReceiverMatcher
behavior, making the comment unnecessary.

> Source/WebKit/Platform/IPC/Connection.h:253
> +    void removeThreadMessageReceiver(ReceiverName, uint64_t destinationID =
0);

Ditto

> Source/WebKit/Platform/IPC/MessageReceiveQueueMap.cpp:40
> +    if (!matcher.destinationID) {

I find !has_value() more readable when dealing with optionals with bool or
sometimes-zero values.

> Source/WebKit/Platform/IPC/MessageReceiveQueueMap.h:56
> +    using AnyIDQueueMap = HashMap<uint8_t, StoreType>;

Would be nice to add a comment to say that the key is a ReceiverName.

> Source/WebKit/Platform/IPC/ReceiverMatcher.h:41
> +    // Note: destinationID == 0 matches only 0 ids.

Is a 0 ID valid? It would be clearer to be able to ASSERT(destinationID) here.

> Source/WebKit/Platform/IPC/ReceiverMatcher.h:62
> +    std::optional<ReceiverName> receiverName;
> +    std::optional<uint64_t> destinationID;

This struct is going to end up as 128 bytes (or larger?) so maybe you should
pass it around by reference.