Bug 226688 – Use `const uint8_t*` type more consistently to store bytes in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Use `const uint8_t*` type more consistently to store bytes in WebKit (instead of `unsigned char*`).

[Chris Dumez]

Created attachment 430661 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Chris Dumez]

Created attachment 430662 [details]
Patch

[Chris Dumez]

Created attachment 430663 [details]
Patch

[Chris Dumez]

Created attachment 430664 [details]
Patch

[Chris Dumez]

Created attachment 430666 [details]
Patch

[Chris Dumez]

Created attachment 430667 [details]
Patch

[Chris Dumez]

Created attachment 430671 [details]
Patch

[Darin Adler]

Comment on attachment 430671 [details]
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.

[Chris Dumez]

Created attachment 430673 [details]
Patch

[EWS]

Committed r278532 (238530@main): <https://commits.webkit.org/238530@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430673 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78915526>