[webkit-reviews] review granted: [Bug 226688] Use `const uint8_t*` type more consistently to store bytes in WebKit : [Attachment 430671] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 226688: Use `const uint8_t*` type more consistently to store bytes in
WebKit
https://bugs.webkit.org/show_bug.cgi?id=226688

Attachment 430671: Patch

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




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

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

Looks great as is. Some suggestions for making it even better follow.

Side note: If we do decide to move to std::byte for some or all of this at some
point, reducing the casts to a minimum as you are doing here and using auto
instead of repeating types, as I suggest in various places below, can make such
a transition even easier. Note that if we ever *do* switch to std::byte,
reading from such things will be done with std::to_integer<uint8_t>, not
reinterpret_cast, so I think it will be a not-super-hard job on top of this
one. std::byte is quite close to a subset of uint8_t in semantics.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:661
> +	       auto highByte = frame.payload[0];
> +	       auto lowByte = frame.payload[1];
>	       m_closeEventCode = highByte << 8 | lowByte;

With the types handled correctly don’t need the local variables any more, so
this might be good as a one-liner.

> Source/WebCore/Modules/websockets/WebSocketDeflater.cpp:174
> +    static const uint8_t strippedFields[] = "\0\0\xff\xff";
>      static const size_t strippedLength = 4;

When touching code like this it’s nice to make it use constexpr instead of
const. Also not sure string syntax is better here than array syntax. I would
use something like this:

    constexpr uint8_t strippedFields[] = { 0, 0, 0xFF, 0xFF };
    constexpr auto strippedLength = std::size(strippedFields);

> Source/WebCore/Modules/websockets/WebSocketFrame.cpp:42
> -const unsigned char finalBit = 0x80;
> -const unsigned char compressBit = 0x40;
> -const unsigned char reserved2Bit = 0x20;
> -const unsigned char reserved3Bit = 0x10;
> -const unsigned char opCodeMask = 0xF;
> -const unsigned char maskBit = 0x80;
> -const unsigned char payloadLengthMask = 0x7F;
> +const uint8_t finalBit = 0x80;
> +const uint8_t compressBit = 0x40;
> +const uint8_t reserved2Bit = 0x20;
> +const uint8_t reserved3Bit = 0x10;
> +const uint8_t opCodeMask = 0xF;
> +const uint8_t maskBit = 0x80;
> +const uint8_t payloadLengthMask = 0x7F;
>  const size_t maxPayloadLengthWithoutExtendedLengthField = 125;
>  const size_t payloadLengthWithTwoByteExtendedLengthField = 126;
>  const size_t payloadLengthWithEightByteExtendedLengthField = 127;

Same constexpr thought.

> Source/WebCore/Modules/websockets/WebSocketFrame.cpp:52
> +    uint8_t* p = data;

I’d use more auto in this function rather than writing uint8_t so many times.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:253
> +    const uint8_t* p = readHTTPHeaders(header + lineLength, header + len);

auto

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:400
> +    const uint8_t* end = p + 1;

auto

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:446
> +    const uint8_t* p = start;

auto

> Source/WebCore/contentextensions/SerializedNFA.cpp:39
> +    const uint8_t* bytes = reinterpret_cast<const
uint8_t*>(container.data());

auto

> Source/WebCore/contentextensions/SerializedNFA.cpp:41
> +    const uint8_t* end = bytes + bytesLength;

auto

> Source/WebCore/html/track/WebVTTParser.cpp:228
> +    const uint8_t endLines[] = "\n\n";

constexpr

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:107
> +    const uint8_t* data = static_cast<const uint8_t*>(srcData);

auto

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:118
> +	   uint8_t* dst = temporaryData.data();

auto

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:121
> +	   const uint8_t* bits = static_cast<const uint8_t*>(srcData);
> +	   const uint8_t* src = bits + sourceOffset.y() * bytesPerLine +
sourceOffset.x() * bytesPerPixel;

auto

> Source/WebCore/platform/network/HTTPParsers.cpp:668
> +    const uint8_t* p = start;
> +    const uint8_t* end = start + length;

auto

> Source/WebKitLegacy/win/WebDownloadCFNet.cpp:170
> +    RetainPtr<CFDataRef> resumeData = adoptCF(CFDataCreate(0, buffer.data(),
buffer.size()));

auto

> Tools/DumpRenderTree/TestRunner.cpp:315
> +    const uint8_t* buffer = static_cast<const
uint8_t*>(bufferView->baseAddress());

auto

> Tools/DumpRenderTree/TestRunner.cpp:316
> +    std::vector<uint8_t> audioData(buffer, buffer +
bufferView->byteLength());

Can probably just use std::vector, and rely on deduction to supply <uint8_t>.

> Tools/DumpRenderTree/TestRunner.cpp:2371
> +    const uint8_t* buffer = static_cast<const
uint8_t*>(bufferView->baseAddress());
> +    std::vector<uint8_t> mediaIconData(buffer, buffer +
bufferView->byteLength());

Ditto.