[webkit-reviews] review granted: [Bug 231970] RemoteRenderingBackend::CreateImageBuffer should be an async IPC stream message : [Attachment 441804] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 231970: RemoteRenderingBackend::CreateImageBuffer should be an async IPC
stream message
https://bugs.webkit.org/show_bug.cgi?id=231970

Attachment 441804: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 441804
  --> https://bugs.webkit.org/attachment.cgi?id=441804
Patch

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

> Source/WebKit/Platform/IPC/Connection.h:236
> +    void removeStreamMessageReceiver(ReceiverName receiverName, uint64_t
destinationID = 0)
> +    {
> +	   ASSERT(isStreamMessageReceiver(receiverName));
> +	   removeMessageReceiveQueue(receiverName, destinationID);
> +    }

Maybe just a personal preference of mine, but I would like it better if a
multi-line inline function was defined outside the class definition, making it
a little easier to see an overview of what the class provides rather than
having that interspersed with implementation. So I would put the implementation
below.

If you do that, I would also suggest we add the keyword "inline" to start of
the declaration; not something we did in the past, but a good practice to start
going forward. Doing that will let us move the inlines to a separate header in
the future if we wish to, and give us a compile time error if we don’t include
that header. So it’s a good practice to start with any function that has an
inline implementation outside the class definiton. We could wait and add that
keyword only if we decide to move the function body to a separate header, but I
think that specifying inline in the declarations in such cases may be a good
habit to cultivate for future maintainability.

> Source/WebKit/Platform/IPC/Connection.h:424
>      Deque<std::unique_ptr<Decoder>> m_incomingMessages
WTF_GUARDED_BY_LOCK(m_incomingMessagesLock);
> +    Deque<std::unique_ptr<Decoder>>
m_incomingStreamMessagesWithMissingReceiverQueues
WTF_GUARDED_BY_LOCK(m_incomingMessagesLock);

I’m not sure I understand why we use a Deque here instead of a Vector. As I
understand it, the primary feature a Deque offers that a Vector does not is
efficient removal at the front. I don’t see code here that removes from the
front, but maybe this is done on the main m_incomingMessages somewhere I did
not spot and somehow indirectly that’s the same for the new new duque.

I’m sure that I’m just missing the place where this occurs, but wanted to call
it to your intention in case it’s not so.