[webkit-reviews] review denied: [Bug 182032] [WebAuthN] Implement PublicKeyCredential’s [[DiscoverFromExternalSource]] with a dummy authenticator : [Attachment 332126] 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 182032: [WebAuthN] Implement PublicKeyCredential’s
[[DiscoverFromExternalSource]] with a dummy authenticator
https://bugs.webkit.org/show_bug.cgi?id=182032

Attachment 332126: Patch

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




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

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

All of this looks good, except the 'getAssertion' implementation. I am very
uneasy by the "do bogus work to get the tests to run properly". If you run on
different hardware, your loop of 10000 may not be enough. Let's try to do this
properly. For that reason, r- of this patch (just for the 'getAssertion'
chunk).  Let's talk when you have a minute.

> Source/WebCore/Modules/webauthn/Authenticator.cpp:71
> +    }

This is pretty sketchy. What if you just used a 'sleep' here? I know you added
this due to test flakiness, but I don't understand why the timing here matters.
What's wrong with an instantaneous 'getAssertion' happening?

In general, we want to support asynchronous actions, and need to use promises
or some other mechanism to allow for variation in response time from these
methods. Artificially slowing this code path down to satisfy the test system
seems like we aren't addressing the real problem.

Do we need to use some kind of async coordination primitive (semaphore,
completion handler, etc.) to address this properly? Maybe you should just do
that now when things are as simple as they can be.

> Source/WebCore/Modules/webauthn/Authenticator.h:38
> +// https://bugs.webkit.org/show_bug.cgi?id=181946

We usually just write: "FIXME(181946):" for this kind of thing.

> Source/WebCore/Modules/webauthn/PublicKeyCredential.cpp:146
> +    // FIXME: Got some crazy compile error when I was trying to return RHS
directly.

Can you show the error to JF? He might be able to help figure out what's going
on.