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

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted 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 376960: Patch

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




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

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

r=me with changes

> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:85
> +- (void) endSession;

Why the space before endSession? Same comment for several other below.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:682
> +	   WebKit::MockWebAuthenticationConfiguration::Nfc nfc;

Why is this local needed? Why not use configuration.nfc directly below?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:687
> +	   if (error == "no-tags")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:689
> +	   if (error == "wrong-tag-type")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:691
> +	   if (error == "no-connections")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:693
> +	   if (error == "malicious-payload")

else if

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:79
> +    // FIXME(200932)

Please put in an actual comment, not just a number.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:94
> +	   if (!m_service)

Why is this in the loop and not at the beginning of the method? Can m_service
change while iterating?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.h:50
> +    std::unique_ptr<CtapNfcDriver> m_driver;

We normally try to avoid protected data members. We put data members private
and have protected getters / setters for them if needed.

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
>>> +	     RunLoop::main().dispatch([weakThis, this, session = retainSession]
() mutable {
>> 
>> You could do session = retainPtr(session).
>> Also move weakThis into the lambda capture.
>> Just reducing a little ref churn.
> 
> Fixed the retainPtr(session).
> 
> What do you mean by moving weakThis into the lambda capture which it is
already in?

weakThis = WTFMove(weakThis)

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.h:31
> +#import <wtf/WeakPtr.h>

Does not appear to be needed in the header.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:98
> +using Mock = MockWebAuthenticationConfiguration::Nfc;

Mock seems too generic a name, especially now that we have unified builds.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:146
> +    return result;

Should this autorelease ?

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:71
> +	   // FIXME: Somehow eliminate the need for the copy?

You could add the following getter to ApduResponse:
Vector<uint8_t>& data() { return m_data; }

and then WTFMove(apduResponse->data()) below.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:56
> +	   weakThis->continueAfterGetInfo(ptr, WTFMove(response));

what guarantees that ptr is not stale here? Also please find a better name.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:62
> +void FidoService::continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&&
response)

ptr is not a good name.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.h:45
> +    void continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& info);

ptr should be omitted.