[webkit-reviews] review denied: [Bug 188624] [WebAuthn] Support NFC authenticators for iOS : [Attachment 376823] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 188624: [WebAuthn] Support NFC authenticators for iOS
https://bugs.webkit.org/show_bug.cgi?id=188624

Attachment 376823: Patch

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




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

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

>> Source/WTF/wtf/Platform.h:1556
>> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 0
> 
> Oops. Somehow forgot to checkout this file.

LOl. r- to fix! :-)

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:74
> +	   addResult = result.add(AuthenticatorTransport::Nfc);

Do we support NFC on Macs?

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:44
> +// FIXME(200934)

"// FIXME(200934): Support NFCCTAP_GETRESPONSE"

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:84
> +    RunLoop::main().dispatch([callback = WTFMove(callback), response =
WTFMove(response)] () mutable {

Please double-check that ResponseCallback (WTF::Function?) can be safely moved
across threads without needing a cross-thread copy. I think if it's a
WTF::Function and is constructed/destroyed on the same thread it's fine.

>>
LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.ht
ml:12
>> +	function checkResult(credential)
> 
> Should probably move this to util and share it between NFC and HID.

Good idea!