It's nice to have a CryptoDigest implementation with OpenSSL/LibreSSL, which could potentially be shared with other ports.
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>