[webkit-reviews] review granted: [Bug 170270] [GCrypt] Implement PBKDF2 support : [Attachment 305947] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 170270: [GCrypt] Implement PBKDF2 support
https://bugs.webkit.org/show_bug.cgi?id=170270

Attachment 305947: Patch

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




--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 305947
  --> https://bugs.webkit.org/attachment.cgi?id=305947
Patch

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

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:37
> +#include <pal/crypto/gcrypt/Utilities.h>

Can you not #include <pal/Utilities.h>? Do we not have any forwarding headers
for PAL yet?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:76
> +    context.ref();

It'd be cleaner to use Ref<ScriptExecutionContext> for this, so you don't have
to remember to deref it in two difference places below, but now I see that
ScriptExecutionContext does not inherit from RefCounted even though it has ref
and deref methods. Huh. So not great, but I guess this is best....

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:85
> +		       [callback = WTFMove(callback), exceptionCallback =
WTFMove(exceptionCallback)](ScriptExecutionContext& context) {

What, why are you capturing callback here, when you don't use it? There's no
particular need to move the value. Just ignore it.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:93
> +		   [output = WTFMove(*output), callback = WTFMove(callback),
exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext&
context) {

And here there is no need to capture exceptionCallback.