[webkit-reviews] review granted: [Bug 233030] Distinguish contiguous SharedBuffer from non-contiguous one and guarantee immutability : [Attachment 445993] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jean-Yves Avenard [:jya]
<jean-yves.avenard at apple.com>'s request for review:
Bug 233030: Distinguish contiguous SharedBuffer from non-contiguous one and
guarantee immutability
https://bugs.webkit.org/show_bug.cgi?id=233030

Attachment 445993: Patch

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




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

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

> Source/WebCore/ChangeLog:32
> +	   There's a potential increase of temporary memory usage with the
> +	   ScriptBufferSourceProvider class; bug 233511 is tracking it.

Presumably this is a small enough amount of additional memory that it’s OK and
we don’t have "unshippable" WebKit until it’s done.

> Source/WebCore/platform/SharedBuffer.cpp:102
> +    if (m_segments.size() == 1)
> +	   return
ContiguousSharedBuffer::create(m_segments[0].segment.copyRef());

Isn’t this less efficient than just returning a pointer to the first segment in
this case? How common is this likely to be?

There’s a new failure mode in the new design where the same shared buffer
repeatedly is converted into a ContiguousSharedBuffer. What guarantees that
won’t happen.

> Source/WebCore/platform/SharedBuffer.cpp:172
> +size_t SharedBuffer::size() const
> +{
> +    return m_size;
> +}
> +
> +bool SharedBuffer::isEmpty() const
> +{
> +    return !size();
> +}
> +
> +bool SharedBuffer::isContiguous() const
> +{
> +    return m_contiguous;
> +}

Should any of these go in the header instead as inlines?

> Source/WebCore/platform/SharedBuffer.cpp:226
> +void SharedBuffer::append(const char* data, size_t length)
> +{
> +    append(reinterpret_cast<const uint8_t*>(data), length);
> +}

Should this go in the header instead as an inline?

> Source/WebCore/platform/SharedBuffer.cpp:349
>  bool SharedBuffer::hasOneSegment() const
>  {
> -    auto it = begin();
> -    return it != end() && ++it == end();
> +    return m_segments.size() == 1;
>  }

Should this go in the header instead as an inline?

> Source/WebCore/platform/SharedBuffer.cpp:412
> +const char* ContiguousSharedBuffer::dataAsCharPtr() const
> +{
> +    return reinterpret_cast<const char*>(data());
> +}

Should this go in the header instead as an inline?

> Source/WebCore/platform/SharedBuffer.cpp:544
> +const char* SharedBufferDataView::dataAsCharPtr() const
> +{
> +    return reinterpret_cast<const char*>(data());
> +}

Should this go in the header instead as an inline?

> Source/WebCore/platform/SharedBuffer.h:79
> +    static Ref<DataSegment> create(Vector<uint8_t>&& data)
> +    {
> +	   data.shrinkToFit();
> +	   return adoptRef(*new DataSegment(WTFMove(data)));
> +    }

For future reference/refactoring:

Functions like these probably should not be inlined in the header. It makes the
code bigger at each call site if there is more than one, and the constructors
themselves can often get inlined in these create functions if they are moved
out of the header file, even if the constructors aren’t explicitly marked
inline.

Same for the other create functions.

> Source/WebCore/platform/SharedBuffer.h:140
> +#if !USE(FOUNDATION)
> +    friend class ContiguousSharedBuffer; // For createCFData
> +#endif

Do we really need the #if here?

> Source/WebCore/platform/SharedBuffer.h:281
> +    // Ensure that you can't call append on a ContiguousSharedBuffer
directly.
> +    // When called from the base class, it will assert.
> +    template <typename... Args>
> +    void append(Args&&...) = delete;

Is there an alternate version of this design that doesn’t have the Liskov
Substitutability Principle problem?

> Source/WebCore/platform/SharedBuffer.h:283
> +    WEBCORE_EXPORT const uint8_t *data() const;

Misplaced "*" here.

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:77
> +	   data = API::Data::createWithoutCopying((const unsigned
char*)contiguousBuffer->data(), contiguousBuffer->size(), releaseSharedBuffer,
contiguousBuffer.ptr());

This typecast is not needed, that’s already the correct type.

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:284
> +    RefPtr<ContiguousSharedBuffer> buffer = handle.tryWrapInSharedBuffer();

I suggest auto here.

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:569
> +    RefPtr<ContiguousSharedBuffer> buffer =
frame.editor().dataSelectionForPasteboard(pasteboardType);

I suggest auto here.

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:-235
> -#if HAVE(UIKIT_WEBKIT_INTERNALS)
> -    return mode == HTMLMediaElementEnums::VideoFullscreenModeStandard;
> -#else
>      return true;
> -#endif

This seems like an unrelated change that was included in this patch by
accident?

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:242
> -#if PLATFORM(IOS_FAMILY) && !HAVE(UIKIT_WEBKIT_INTERNALS)
> +#if PLATFORM(IOS_FAMILY)

Ditto.

> Source/WebKitLegacy/mac/WebView/WebHTMLRepresentation.mm:233
> +	   auto *parsedArchiveData = [_private->dataSource
_documentLoader]->parsedArchiveData();

This should be just auto or auto*, not "auto *".

> Source/WebKitLegacy/mac/WebView/WebResource.mm:167
> -    NSData *data = nil;
> +
> +    RetainPtr<NSData> data;

Good fix! I was prepping for createNSData and spotted this error, and was about
to ask you to fix it, but looks like you spotted it too.

> Tools/TestWebKitAPI/Tests/WebCore/SharedBuffer.cpp:46
> +    RefPtr<SharedBuffer> buffer =
ContiguousSharedBuffer::createWithContentsOfFile(String("not_existing_file"));

Would more auto make these better?