[webkit-reviews] review granted: [Bug 226623] Use Vector<uint8_t> instead of Vector<char> to store bytes in SharedBuffer : [Attachment 430577] Patch

Fri Jun 4 11:04:44 PDT 2021

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 226623: Use Vector<uint8_t> instead of Vector<char> to store bytes in
SharedBuffer
https://bugs.webkit.org/show_bug.cgi?id=226623

Attachment 430577: Patch

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

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

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

We may want to work harder to make sure continue to adopt vectors in cases
where this patch changes us to copy instead.

We may also want to use const void* and void* more on functions that take and
process bytes, rather than using a specific byte pointer types.

> Source/WTF/wtf/FileSystem.h:156
> +WTF_EXPORT_PRIVATE int writeToFile(PlatformFileHandle, const char* data, int
length); // FIXME: Should use `const uint8_t*`.

Maybe it should take const void* like the POSIX write() function.

> Source/WTF/wtf/FileSystem.h:158
> +WTF_EXPORT_PRIVATE int readFromFile(PlatformFileHandle, char* data, int
length); // FIXME: Should use `uint8_t*`.

Maybe it should take void* like the POSIX read() function.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:197
> +	   const char* nextBoundary = static_cast<const
char*>(memmem(currentBoundary + boundaryLength, length - (currentBoundary +
boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(),
boundaryLength));

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:203
> +	       nextBoundary = static_cast<const char*>(memmem(nextBoundary +
boundaryLength, length - (nextBoundary + boundaryLength -
reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));

Ditto.

> Source/WebCore/Modules/highlight/AppHighlight.h:55
> +    encoder.encodeFixedLengthData(reinterpret_cast<const
uint8_t*>(highlight->data()), highlight->size(), 1);

Why is a cast needed here? Doesn’t data() return a uint8_t*?

> Source/WebCore/Modules/indexeddb/IDBGetResult.cpp:37
> +    m_value = ThreadSafeDataBuffer::create({ buffer.data(), buffer.size()
});

No cleaner idiom? Maybe a std::span or Vector?

>
Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:17
2
> +	   const uint8_t* data = reinterpret_cast<const
uint8_t*>(buffer->data.data<uint8_t>());

auto would be better here.

Why is reinterpret_cast needed here? What type does data<uint8_t>() return? Why
not uint8_t*?

> Source/WebCore/css/CSSFontFaceSource.cpp:168
> -		       m_generatedOTFBuffer =
SharedBuffer::create(WTFMove(otfFont.value()));
> +		       m_generatedOTFBuffer =
SharedBuffer::create(otfFont->data(), otfFont->size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/fileapi/Blob.cpp:117
> +    blobParts.append(Vector { buffer.data(), buffer.size() });

This seems to call for std::span. Or doesn’t SharedBuffer have a function that
returns a Vector?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:583
> +	   NetworkResourcesData::ResourceData const* resourceData =
m_resourcesData->maybeAddResourceData(requestId, reinterpret_cast<const
uint8_t*>(data), dataLength);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:159
> +	   m_client->didReceiveData(this, buffer ? buffer->data() :
reinterpret_cast<const uint8_t*>(data), buffer ? buffer->size() : length);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:157
> -	   content->append(WTFMove(part));
> +	   content->append(part.data(), part.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:198
> +	   auto decodedData = base64Decode(content->data()   ,
content->size());

Should probably get rid of the extra spaces here.

> Source/WebCore/loader/cache/CachedSVGFont.cpp:92
> -	       m_convertedFont =
SharedBuffer::create(WTFMove(convertedFont.value()));
> +	       m_convertedFont = SharedBuffer::create(convertedFont->data(),
convertedFont->size());

Is this changing from adopting data to copying it, or did the old version
already copy the data?

> Source/WebCore/platform/SharedBuffer.cpp:142
> -    auto arrayBuffer =
ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()),
sizeof(char));
> +    auto arrayBuffer =
ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()),
sizeof(uint8_t));

Not new: What guarantees size() fits in unsigned?

Both of these sizeof expression are silly ways to write the number 1. I think
we could just write 1.

> Source/WebCore/platform/SharedBuffer.cpp:232
> +	   [](const RetainPtr<CFDataRef>& data) { return reinterpret_cast<const
uint8_t*>(CFDataGetBytePtr(data.get())); },

No typecast needed any more: CFDataGetBytePtr returns const uint8_t*.

> Source/WebCore/platform/SharedBuffer.cpp:235
> +	   [](const GRefPtr<GBytes>& data) { return reinterpret_cast<const
uint8_t*>(g_bytes_get_data(data.get(), nullptr)); },

This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

> Source/WebCore/platform/SharedBuffer.cpp:238
> +	   [](const RefPtr<GstMappedOwnedBuffer>& data) { return
reinterpret_cast<const uint8_t*>(data->data()); },

No typecast needed any more: GstMappedOwnedBuffer::data returns const uint8_t*.

> Source/WebCore/platform/SharedBuffer.cpp:240
> +	   [](const FileSystem::MappedFileData& data) { return
reinterpret_cast<const uint8_t*>(data.data()); }

This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

> Source/WebCore/platform/SharedBuffer.cpp:357
> +    char* p = reinterpret_cast<char*>(buffer.data());

Always a sad moment when we have to add a reinterpret_cast; any way to avoid
this one?

> Source/WebCore/platform/SharedBuffer.cpp:370
> +    buffer.shrink(p - reinterpret_cast<char*>(buffer.data()));

Always a sad moment when we have to add a reinterpret_cast; any way to avoid
this one?

> Source/WebCore/platform/SharedBufferChunkReader.cpp:103
> +		   chunk.append(reinterpret_cast<const
uint8_t*>(m_separator.data()), m_separatorIndex);

Always a sad moment when we have to add a reinterpret_cast; any way to avoid
this one?

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:297
> -	   buffer = SharedBuffer::create(WTFMove(convertedFont));
> +	   buffer = SharedBuffer::create(convertedFont.data(),
convertedFont.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:239
> +    const uint8_t* source = sharedBuffer->data() + position;

auto

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:89
> +	   size_t offset = reinterpret_cast<const uint8_t*>(position) -
buffer.data();

This can be static_cast instead of reinterpret_cast since it’s from a void*.

> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:318
> +    return strncmp(&m_data->dataAsCharPtr()[imageOffset], "\x89PNG", 4) ?
BMP : PNG;

This should use memcmp instead of strncmp--it was never useful to use strncmp
here, no reason to handle null characters specially--and then we will not need
dataAsCharPtr here.

> Source/WebCore/platform/network/FormData.cpp:417
> +    auto bytes = flatten();
> +    return SharedBuffer::create(bytes.data(), bytes.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/platform/win/SharedBufferWin.cpp:55
> +	   if (ReadFile(fileHandle, (LPVOID)buffer.data(), bytesToRead,
&bytesRead, 0) && bytesToRead == bytesRead)

Why was this new typecast needed?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:362
> -    OffsetBuffer(Vector<char> buffer)
> +    OffsetBuffer(Vector<uint8_t>&& buffer)

Nice optimization.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:364
>	   , m_currentOffset(0)

We should move this initialization down to where the data member is defined
below.

> Source/WebKit/WebProcess/Plugins/Netscape/NetscapePluginStream.cpp:120
> +    deliverData(reinterpret_cast<const uint8_t*>(resultCString.data()),
resultCString.length());

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1172
> +    m_pluginView->manualLoadDidReceiveData(reinterpret_cast<const
uint8_t*>(data), length);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?