[webkit-reviews] review granted: [Bug 233011] [WebAuthn] Stop serializing BufferSource and Vector<uint8_t> duplicates of identifiers : [Attachment 444005] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 233011: [WebAuthn] Stop serializing BufferSource and Vector<uint8_t>
duplicates of identifiers
https://bugs.webkit.org/show_bug.cgi?id=233011

Attachment 444005: Patch

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




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 444005
  --> https://bugs.webkit.org/attachment.cgi?id=444005
Patch

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

r=me, very nice!

> Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:88
> +    return constructU2fSignCommand(applicationParameter, challengeParameter,
keyVector, checkOnly);

Doesn't need to be done in this patch but constructU2fSignCommand() should
probably take in a WTF::Span<const uint8_t> instead of a Vector<uint8_t>.

> Source/WebCore/bindings/js/BufferSource.h:99
> +    auto dataSize = CheckedSize { *size } * sizeof(uint8_t);

nit: The `* sizeof(uint8_t)` doesn't look terribly useful :)

> Source/WebCore/bindings/js/BufferSource.h:106
> +    return BufferSource(JSC::ArrayBuffer::tryCreate(reinterpret_cast<const
void*>(data), dataSize.value()));

nit: Would a static_cast<const void*> work?

> Source/WebCore/bindings/js/BufferSource.h:117
> +    return BufferSource(JSC::ArrayBuffer::tryCreate(reinterpret_cast<const
uint8_t*>(data.bytes), data.length));

static_cast<> should suffice since bytes returns a const void*

> Source/WebCore/bindings/js/BufferSource.h:128
> +using WebCore::toBufferSource;

I am not opposed to it but I didn't realize we were doing this outside of WTF.