Bug 188978

Created attachment 348128 [details]
patch

Comment on attachment 348128 [details]
patch

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

LGTM and adding tests is awesome.

I'm just not sure we need all the #ifndef's hanging out.

> Source/WebCore/PAL/pal/crypto/openssl/CryptoDigestOpenSSL.cpp:38
> +#ifndef OPENSSL_NO_SHA1

What are the cases where this and the other macros would be defined?

> Tools/TestWebKitAPI/PlatformWin.cmake:129
> +if (PAL_LIBRARY_TYPE MATCHES STATIC)

Could this go into the root CMakelists.txt?

Comment on attachment 348128 [details]
patch

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

>> Source/WebCore/PAL/pal/crypto/openssl/CryptoDigestOpenSSL.cpp:38
>> +#ifndef OPENSSL_NO_SHA1
> 
> What are the cases where this and the other macros would be defined?

Do we want to support those cases?  I think probably not, especially not without a fallback.

> Source/WebCore/PAL/pal/crypto/openssl/CryptoDigestOpenSSL.cpp:71
> +    : m_context(new CryptoDigestContext)

This is what std::make_unique is for.

Myles and Alex after this would we like to create a TestPAL project? There's a legit test now in the repository.

Thanks for the reviews. I'm revising my patch.

(In reply to Don Olmstead from comment #2)
> > Tools/TestWebKitAPI/PlatformWin.cmake:129
> > +if (PAL_LIBRARY_TYPE MATCHES STATIC)
> 
> Could this go into the root CMakelists.txt?

I don't think so since somehow TestWebCoreLib is defined in each Platform*.cmake, perhaps due to JSCOnly.

(In reply to Yoshiaki Jitsukawa from comment #6)
> (In reply to Don Olmstead from comment #2)
> > > Tools/TestWebKitAPI/PlatformWin.cmake:129
> > > +if (PAL_LIBRARY_TYPE MATCHES STATIC)
> > 
> > Could this go into the root CMakelists.txt?
> 
> I don't think so since somehow TestWebCoreLib is defined in each
> Platform*.cmake, perhaps due to JSCOnly.

K I'll maybe take a look at the CMake stuff for the tests in the future.

Created attachment 348235 [details]
Patch take2

Removing #ifndefs and switching to make_unique().

Comment on attachment 348235 [details]
Patch take2

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

> Source/WebCore/PAL/pal/crypto/openssl/CryptoDigestOpenSSL.cpp:81
> +    case CryptoDigest::Algorithm::SHA_1:
> +        delete toSHA1Context(m_context.get());
> +        return;
> +    case CryptoDigest::Algorithm::SHA_224:
> +        delete toSHA224Context(m_context.get());
> +        return;

This is what a virtual destructor is for.

> Source/WebCore/PAL/pal/crypto/openssl/CryptoDigestOpenSSL.cpp:96
> +    std::unique_ptr<CryptoDigest> digest(new CryptoDigest);

This is also what make_unique is for.

Created attachment 348399 [details]
Patch take3

Using template to generate CryptoDigestContextImpl variants of SHA family.

(In reply to Alex Christensen from comment #9)
> > Source/WebCore/PAL/pal/crypto/openssl/CryptoDigestOpenSSL.cpp:96
> > +    std::unique_ptr<CryptoDigest> digest(new CryptoDigest);
> 
> This is also what make_unique is for.

Since the constructor of CryptoDigest is private, unfortunately make_unique cannot invoke it.

Why don't we make the constructor public instead of having a static create method that returns a unique_ptr?  And why doesn't this replace wtf/SHA1.h?

(In reply to Alex Christensen from comment #12)
> And why doesn't this replace wtf/SHA1.h?

I think that's because WTF::Persistence::Encoder and JSC::CodeBlockHash use it.

(In reply to Alex Christensen from comment #12)
> Why don't we make the constructor public instead of having a static create
> method that returns a unique_ptr?

I suppose we don't want to return a CryptoDigest instance with unsupported algorithm. As far as I see CryptoDigestWin lacks SHA_224.

Comment on attachment 348399 [details]
Patch take3

Clearing flags on attachment: 348399

Committed r235587: <https://trac.webkit.org/changeset/235587>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/44041807>