Created attachment 215694 [details] proposed patch

Attachment 215694 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/crypto/subtle/hmac-sign-verify-expected.txt', u'LayoutTests/crypto/subtle/hmac-sign-verify.html', 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/CryptoAlgorithmHMAC.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.h', u'Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp', u'Source/WebCore/crypto/keys/CryptoKeyHMAC.h', u'Source/WebCore/crypto/mac/CryptoAlgorithmHMACMac.cpp', u'Source/WebCore/crypto/mac/CryptoAlgorithmRegistryMac.cpp']" exit_code: 1 Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.h:45: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/crypto/keys/CryptoKeyHMAC.h:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 215694 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=215694&action=review > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:235 > + std::unique_ptr<PromiseWrapper> promiseWrapper = PromiseWrapper::create(globalObject(), promise); I’d use auto for the variable declaration here. > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:238 > + algorithm->verify(*parameters, *key.get(), signature, data, std::move(promiseWrapper), ec); Can’t you just do *key here? > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:337 > + std::unique_ptr<PromiseWrapper> promiseWrapper = PromiseWrapper::create(globalObject(), promise); Auto? > Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:52 > + std::unique_ptr<CryptoAlgorithmDescriptionBuilder> hashDescriptionBuilder = builder.createEmptyClone(); Auto?

Created attachment 215720 [details] patch for landing

Comment on attachment 215720 [details] patch for landing Clearing flags on attachment: 215720 Committed r158427: <http://trac.webkit.org/changeset/158427>

All reviewed patches have been landed. Closing bug.

Comment on attachment 215720 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=215720&action=review > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:144 > + if (usageString == "encrypt") > + result |= CryptoKeyUsageEncrypt; > + else if (usageString == "decrypt") > + result |= CryptoKeyUsageDecrypt; > + else if (usageString == "sign") > + result |= CryptoKeyUsageSign; > + else if (usageString == "verify") > + result |= CryptoKeyUsageVerify; > + else if (usageString == "deriveKey") > + result |= CryptoKeyUsageDeriveKey; > + else if (usageString == "deriveBits") > + result |= CryptoKeyUsageDeriveBits; > + else if (usageString == "wrapKey") > + result |= CryptoKeyUsageWrapKey; > + else if (usageString == "unwrapKey") > + result |= CryptoKeyUsageUnwrapKey; Are repeated strings allowed? Are unknown strings supposed to be silently ignored?

> Are repeated strings allowed? WebCrypto would have to specify that, which it doesn't (neither does it specify pretty much anything about "usages" yet). > Are unknown strings supposed to be silently ignored? Good point, WebIDL says that a typeError should be raised when a JS value is not one of enum values. I'm not sure if that makes sense for WebCrypto, because unknown values have no effect on the behavior. If we don't implement a WebCrypto operation, it doesn't matter if it's allowed. The only way it's observable is via inspecting key.usages attribute, which is probably not useful outside of debugging. It can also become observable for keys that are stored permanently, if we add support for new operations. Our generated bindings code currently doesn't raise exceptions when setting an attribute to an unknown enum value. It silently ignores setting an attribute to an unknown enum value. Ugh.