[webkit-reviews] review granted: [Bug 233442] Add SharedBufferBuilder class : [Attachment 446542] Patch for review

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 9 13:47:12 PST 2021


youenn fablet <youennf at gmail.com> has granted Jean-Yves Avenard [:jya]
<jean-yves.avenard at apple.com>'s request for review:
Bug 233442: Add SharedBufferBuilder class
https://bugs.webkit.org/show_bug.cgi?id=233442

Attachment 446542: Patch for review

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




--- Comment #54 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 446542
  --> https://bugs.webkit.org/attachment.cgi?id=446542
Patch for review

LGTM.

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

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:335
> +    m_buffer.append(WTFMove(data));

m_buffer = WTFMove(data) might be better?

> Source/WebCore/platform/SharedBuffer.cpp:544
> +    ensureBuffer();

No need to call reset here.
m_buffer = SharedBuffer::create might be enough.

> Source/WebCore/platform/SharedBuffer.cpp:550
> +    return m_buffer.releaseNonNull();

Could be rewritten to:
return m_buffer ? m_buffer.releaseNonNull() : SharedBuffer::create();

> Source/WebCore/platform/SharedBuffer.h:314
> +	   : SharedBufferBuilder(RefPtr<SharedBuffer> { WTFMove(buffer) })

Here we are doing a if(!buffer) unnecessary check.
If we want to share code, we could simply add a:
initialize(Ref<SharedBuffer>&&) that we would call here and in the above
constructor (initialize(buffer.releaseNonNull()).

> Source/WebCore/platform/SharedBuffer.h:337
> +    RefPtr<SharedBuffer> get() const { return m_buffer; }

Looking a more places, I would be tempted to rename it as buffer().
Given the size of the patch, this could be done in a follow-up if you think
that is valuable enough.

> Source/WebCore/platform/cf/SharedBufferCF.cpp:38
> +    ASSERT(!m_contiguous);

Is it needed? I guess this is initialised in SharedBuffer m_contiguous
declaration.

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:61
> +	       if (auto parsedPayload =
extractCencIfNeeded(payload->makeContiguous()))

I am missing some other patches defining makeContiguous.
InitData is taking a RefPtr&& which does no longer seem needed, except if doing
WTFMove(payload)->makeContiguous() would make it more efficient.

> Source/WebCore/workers/ScriptBuffer.h:51
> +    const SharedBuffer* buffer() const { return m_buffer.get().get(); }

.get().get() is a bit weird.
I guess the first .get() could be renamed to .buffer().
But then we would have to rename m_buffer to m_bufferBuilder.
Oh well, might be good enough for now.

> Source/WebCore/workers/WorkerFontLoadRequest.cpp:85
> +	       m_data.append(*contiguousData);

As I read it, we called takeAsContiguous so m_data is null.
It seems like we could be able to optimise if we were able to call m_data
constructor instead of append.

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.cpp:70
> +   
webPage->didFinishLoadForQuickLookDocumentInMainFrame(m_buffer->take().get());

Probably s/->/./

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.cpp:75
> +    m_buffer->reset();

s/->/./

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.h:33
> +#include <wtf/UniqueRef.h>

No longer need for UniqueRef.h


More information about the webkit-reviews mailing list