[webkit-reviews] review granted: [Bug 190784] [WebAuthN] Import CTAP device request/response converters from Chromium : [Attachment 353527] 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 190784: [WebAuthN] Import CTAP device request/response converters from
Chromium
https://bugs.webkit.org/show_bug.cgi?id=190784

Attachment 353527: Patch

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




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

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

Looks good. I had a minor suggestion for the decode logic (labelling), but not
necessary to land the code.

> Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:98
> +    auto it = decodedMap.find(CBOR(1));

It seems like these ordinal values for the decodeMap might be more meaningful
with constant definitions. Is the only meaning of this map entry that it is the
first of 'n' values? Or is the first value always a specific piece of
information? Just like the request map is accessed through keys, it would be
nice to access the response with keys, too (for legibility).

Still, since this is Google code, maybe we want to keep it as pristine as
possible to aid in porting over changes.