[webkit-reviews] review granted: [Bug 175657] [WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with CCECCryptorExportKey for exporting ECC JWKs : [Attachment 318401] 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 175657: [WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with
CCECCryptorExportKey for exporting ECC JWKs
https://bugs.webkit.org/show_bug.cgi?id=175657

Attachment 318401: Patch

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




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

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

I think this looks good, but I would prefer you revise the return code as I
outlined above. r=me assuming you make the changes I suggest.

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:149
> +    return Exception { OperationError };

I think this would be clearer if you did the early return for the failure case,
rather than success.

I also think this would be clearer (use 'isEmpty()' rather than a zero-check
for size())

if (result.isEmpty())
    return Exception { OperationError };

return WTFMove(result);

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:168
> +    return Exception { OperationError };

Ditto the early-return case.

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:179
> +    return Exception { OperationError };

Ditto.

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:190
> +    return Exception { OperationError };

Ditto.