Bug 123582

Created attachment 215662 [details] proposed patch

Attachment 215662 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/TestExpectations', u'LayoutTests/security/crypto-subtle-arguments-expected.txt', u'LayoutTests/security/crypto-subtle-arguments.html', u'LayoutTests/security/crypto-subtle-sha1-expected.txt', u'LayoutTests/security/crypto-subtle-sha1.html', u'LayoutTests/security/resources/common.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp', u'Source/WebCore/crypto/SubtleCrypto.idl', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.h', u'Source/WebCore/crypto/mac/CryptoAlgorithmRegistryMac.cpp', u'Source/WebCore/crypto/mac/CryptoAlgorithmSHA1Mac.cpp']" exit_code: 1 LayoutTests/TestExpectations:79: Path does not exist. [test/expectations] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 215664 [details] with a better skip list

Comment on attachment 215664 [details] with a better skip list View in context: https://bugs.webkit.org/attachment.cgi?id=215664&action=review > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:49 > + return 0; nullptr. > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:55 > + return std::move(result); No need for std::move here. > Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.h:38 > + static const char* const m_name; > + static const CryptoAlgorithmIdentifier m_identifier = CryptoAlgorithmIdentifier::SHA_1; We usually don’t use m_ for public member variables, at least I don’t think we do.

Committed <http://trac.webkit.org/r158387>.

Comment on attachment 215664 [details] with a better skip list View in context: https://bugs.webkit.org/attachment.cgi?id=215664&action=review > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:52 > + std::unique_ptr<CryptoAlgorithm> result(CryptoAlgorithmRegistry::shared().create(algorithmIdentifier)); I would write: auto result = CryptoAlgorithmRegistry::shared().create(algorithmIdentifier); > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:86 > + std::unique_ptr<CryptoAlgorithm> algorithm = createAlgorithmFromJSValue(exec, exec->uncheckedArgument(0)); I like auto better than std::unique_ptr here. > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:92 > + std::unique_ptr<CryptoAlgorithmParameters> parameters = JSCryptoAlgorithmDictionary::createParametersForDigest(exec, algorithm->identifier(), exec->uncheckedArgument(0)); I like auto better than std::unique_ptr here. >> Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.h:38 >> + static const CryptoAlgorithmIdentifier m_identifier = CryptoAlgorithmIdentifier::SHA_1; > > We usually don’t use m_ for public member variables, at least I don’t think we do. We definitely don’t use m_ for static data members. We normally avoid public member variables entirely, except when we use a struct instead of a class and we don’t use prefixes at all. I think it would be best to use a public static function instead of a public static data member. Inline if it needs to be. > Source/WebCore/crypto/mac/CryptoAlgorithmSHA1Mac.cpp:36 > +void CryptoAlgorithmSHA1::digest(const CryptoAlgorithmParameters&, const Vector<CryptoOperationData>& data, std::unique_ptr<PromiseWrapper> promise, ExceptionCode&) Seems like this should take PromiseWrapper& instead of unique_ptr<PromiseWrapper>.

> I like auto better than std::unique_ptr here. Can try how this works. I'm worried that with smart pointers replaced with auto, it will be harder to confirm that there are no memory leaks. > Seems like this should take PromiseWrapper& instead of unique_ptr<PromiseWrapper>. The interface is designed to allow for async execution, so I don't think that this would work.