[webkit-reviews] review granted: [Bug 193150] [WebAuthN] Import U2F command/response converters from Chromium : [Attachment 358399] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 193150: [WebAuthN] Import U2F command/response converters from Chromium
https://bugs.webkit.org/show_bug.cgi?id=193150

Attachment 358399: Patch

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




--- Comment #6 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 358399
  --> https://bugs.webkit.org/attachment.cgi?id=358399
Patch

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

R=me

> Source/WebCore/ChangeLog:20
> +	   Besides importing stuffs from Chroimum, it also gathers a bunch of
constants and helper functions into WebAuthenticationConstants.h

Besides importing stuff from Chromium ...

> Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:90
> +	   if (parameters.alg == COSE::ES256)

This really seems like something a std::find would work for.  But it’s fine
as-is.

> Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:114
> +    return constructU2fSignCommand(produceRpIdHash(request.rp.id),
clientDataHash, keyHandle.idVector, true /* checkOnly */);

The need for this comment indicates this should be an enumeration! But since
this is imported code, land as-is and then post a patch to clean up the
interface.

> Source/WebCore/Modules/webauthn/fido/U2fResponseConverter.cpp:71
> +    x.append(u2fData.data() + pos, ES256FieldElementLength);

Can the vector be constructed directly with this data? Or at least sized
properly, rather than having a separate allocation event, then a resize as we
append.

This is true for all such cases.

> Source/WebCore/Modules/webgpu/WebGPUCommandBuffer.cpp:34
> +#include "GPURenderPipeline.h"

Is this needed as part of this patch? Was it causing a build error for you?