Bug 226688 - Use `const uint8_t*` type more consistently to store bytes in WebKit
Summary: Use `const uint8_t*` type more consistently to store bytes in WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-05 16:08 PDT by Chris Dumez
Modified: 2021-06-05 22:26 PDT (History)
36 users (show)

See Also:


Attachments
Patch (104.81 KB, patch)
2021-06-05 16:09 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (120.24 KB, patch)
2021-06-05 16:31 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (120.75 KB, patch)
2021-06-05 16:35 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (122.34 KB, patch)
2021-06-05 16:43 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (123.46 KB, patch)
2021-06-05 17:31 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (123.71 KB, patch)
2021-06-05 17:42 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (124.22 KB, patch)
2021-06-05 20:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (125.06 KB, patch)
2021-06-05 21:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-06-05 16:08:24 PDT
Use `const uint8_t*` type more consistently to store bytes in WebKit (instead of `unsigned char*`).
Comment 1 Chris Dumez 2021-06-05 16:09:48 PDT
Created attachment 430661 [details]
Patch
Comment 2 EWS Watchlist 2021-06-05 16:10:41 PDT
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
Comment 3 Chris Dumez 2021-06-05 16:31:24 PDT
Created attachment 430662 [details]
Patch
Comment 4 Chris Dumez 2021-06-05 16:35:52 PDT
Created attachment 430663 [details]
Patch
Comment 5 Chris Dumez 2021-06-05 16:43:22 PDT
Created attachment 430664 [details]
Patch
Comment 6 Chris Dumez 2021-06-05 17:31:07 PDT
Created attachment 430666 [details]
Patch
Comment 7 Chris Dumez 2021-06-05 17:42:18 PDT
Created attachment 430667 [details]
Patch
Comment 8 Chris Dumez 2021-06-05 20:32:32 PDT
Created attachment 430671 [details]
Patch
Comment 9 Darin Adler 2021-06-05 21:01:42 PDT
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.
Comment 10 Chris Dumez 2021-06-05 21:12:15 PDT
Created attachment 430673 [details]
Patch
Comment 11 EWS 2021-06-05 22:25:50 PDT
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].
Comment 12 Radar WebKit Bug Importer 2021-06-05 22:26:18 PDT
<rdar://problem/78915526>