| Summary: | [WebCrypto] Add SHA-1 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||
| Component: | WebCore Misc. | Assignee: | Alexey Proskuryakov <ap> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, commit-queue, darin, esprehn+autocc, kondapallykalyan, sam | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 122679 | ||||||||
| Attachments: |
|
||||||||
|
Description
Alexey Proskuryakov
2013-10-31 12:06:29 PDT
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. |