[webkit-reviews] review granted: [Bug 173647] [GCrypt] Implement CryptoKeyEC PKCS#8 imports : [Attachment 316884] Patch

[bugzilla-daemon at webkit.org]

Jiewen Tan <jiewen_tan at apple.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 173647: [GCrypt] Implement CryptoKeyEC PKCS#8 imports
https://bugs.webkit.org/show_bug.cgi?id=173647

Attachment 316884: Patch

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




--- Comment #21 from Jiewen Tan <jiewen_tan at apple.com> ---
Comment on attachment 316884
  --> https://bugs.webkit.org/attachment.cgi?id=316884
Patch

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

Looks good to me. r=me.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:468
>>> +	     // Retrieve the `q` point. Since only `d` is provided, libgcrypt
will compute it on-the-fly.
>> 
>> Isn't the public key provided in the previous step if any? Why is
computation still needed for that case?
> 
> Pubic key is optional. If it's present and of valid form, it's set above,
using the gcry_mpi_t object. Here it's retrieved into a gcry_mpi_point_t
object, which is the type used to represent EC points, but it has essentially
the same value as publicKeyMPI if that was provided, or libgcrypt will have to
compute it from scratch for the specific EC curve.
> 
> I can improve the comment upon landing, or upon next upload if there's
anything more substantial to polish.

Sure, please improve the comments.