[webkit-reviews] review granted: [Bug 208090] [GPUP] Implement Modern EME API in the GPU Process : [Attachment 391989] Part 2: WebKit + SharedBuffer

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 208090: [GPUP] Implement Modern EME API in the GPU Process
https://bugs.webkit.org/show_bug.cgi?id=208090

Attachment 391989: Part 2: WebKit + SharedBuffer

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




--- Comment #35 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 391989
  --> https://bugs.webkit.org/attachment.cgi?id=391989
Part 2: WebKit + SharedBuffer

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

> Source/WebKit/ChangeLog:13
> +	   by the SharedBuffer object at creation time, leading to zero-copy
decoding, but is not done in this patch.

Wouldn't this still be one-copy because we need to copy it into shared memory
when encoding?
decodeSharedBuffer is in desperate need of at least a FIXME comment saying that
the SharedBuffer should just adopt SharedMemory::Handle as a new type of
DataSegment, which would require moving SharedMemory to WebCore.  I think we
should to that before this to prevent performance regressions, but if you
promise to do that in the next week or two I'd be ok with a FIXME comment.

> Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39
> +	   : m_buffer(WTFMove(buffer))
> +    {
> +    }

We're starting to put these on the line before.
: m_buffer(WTFMove(buffer)) { }

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:192
> +    auto fileWrapper = adoptNS([pageClient().allocFileWrapperInstance()
initRegularFileWithContents:dataReference.buffer()->createNSData().autorelease(
)]);

This used to be a Ref<SharedBuffer> but now it is a RefPtr<SharedBuffer> which
would need a null check.

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:198
> +    if
(UNLIKELY(m_interceptController.isIntercepting(m_coreLoader->identifier()))) {

I'm not a huge fan of all this duplicate code.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:132
> +   
m_connection->send(Messages::ServiceWorkerFetchTask::DidReceiveSharedBuffer { {
SharedBuffer::create(data, size) }, static_cast<int64_t>(size) },
m_fetchIdentifier);

This looks like an unnecessary copy from {data, size} into a SharedBuffer which
will then copy it into shared memory to serialize.  This at least needs a FIXME
comment saying to improve it.