[webkit-reviews] review granted: [Bug 237730] Allow modification of content of StreamClientConnection.streamBuffer in IPCTestingAPI : [Attachment 454413] Patch

[bugzilla-daemon at webkit.org]

Kimmo Kinnunen <kkinnunen at apple.com> has granted Simon Lewis
<simon.lewis at apple.com>'s request for review:
Bug 237730: Allow modification of content of
StreamClientConnection.streamBuffer in IPCTestingAPI
https://bugs.webkit.org/show_bug.cgi?id=237730

Attachment 454413: Patch

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




--- Comment #3 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 454413
  --> https://bugs.webkit.org/attachment.cgi?id=454413
Patch

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

looks great, some maintenance improvement aspects to consider

> Source/WebKit/Platform/IPC/StreamClientConnection.h:81
> +    friend class WebKit::IPCTestingAPI::JSIPCStreamConnectionBuffer;

could you instead add:
StreamConnectionBuffer& bufferForTesting()

for StreamConnectionBuffer, add function:
Span<uint8_t> headerForTesting()

This way if StreamConnectionBuffer changes a bit, it doesn't neccarily need
knowing that the ipc testing .cpp needs change.
This way the access chain is not so long and

> Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:909
> +    uint8_t* data =
static_cast<uint8_t*>(jsStreamBuffer->m_streamConnection->m_streamConnection.m_
buffer.m_sharedMemory->data());

so this brittle access chain to multiple internal structures we want to break
with controlled ...ForTesting() functions.
(this enables the developers to understand that private variables are accessed
with ForTesting methods. Understanding	and remembering that via friend
declarations is a bit harder)

> LayoutTests/ipc/stream-buffer-read-write.html:17
> +    let initialData = new Uint8Array(streamBuffer.readBytes(0, 32));

I suggest add two modalities:
modify the header
modify the data

this way if the header changes position or is detached from the data area, all
the tests related to corrupting the header will still be valid. it's also
easier to understand what the test does.