Update SubtleCrypto::generateKey to match the latest spec

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Update SubtleCrypto::generateKey to match the latest spec:
https://www.w3.org/TR/WebCryptoAPI/#SubtleCrypto-method-generateKey

[Radar WebKit Bug Importer]

<rdar://problem/28864380>

[Jiewen Tan]

Created attachment 292161 [details]
Patch

[Jiewen Tan]

(In reply to comment #2)
> Created attachment 292161 [details]
> Patch

It becomes gigantic after rebasing w3c test suite. Sorry for that.

[youenn fablet]

Comment on attachment 292161 [details]
Patch

Some quick thoughts about the code.

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

> Source/WebCore/ChangeLog:11
> +           https://www.w3.org/TR/WebCryptoAPI/#SubtleCrypto-method-generateKey.

This spec is 2 years old.
It might be good to point to https://w3c.github.io/webcrypto/Overview.html#SubtleCrypto-method-generateKey which is probably more up to date, although hopefully very close to the W3C spec.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:53
> +static std::unique_ptr<CryptoAlgorithmParameters> nomalizeCryptoAlgorithmParameters(ExecState*, JSValue, Operations);

Would be good to pass an ExecState& for all these helper functions.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:159
> +    throwTypeError(state, scope, ASCIILiteral("Invalid AlgorithmIdentifier"));

Ideally we would like the exception messages to be like other non-custom methods, reusing throwArgumentTypeError.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:287
> +    algorithm->generateKey(*params, extractable, keyUsages, WTFMove(callback), WTFMove(exceptionCallback), scriptExecutionContextFromExecState(&state));

Promise handling moved from a callback-pair version to passing a DeferredWrapper&&, or better a DOMPromise<KeyPair>&&.
This allows rejecting/resolving promises more clearly.
For instance, rejectWithException helper function would probably no longer be needed here and you can do fine-grained messages directly in the code.

This also allows to keep only one DeferredWrapper ref while we keep two with callbacks (plus some unneeded count churning).

This may have quite some impact on the current implementation.
Maybe this could be done as a preliminary patch?

> Source/WebCore/crypto/CryptoAlgorithmParameters.h:45
> +public:

Use a struct if everything is public?

> Source/WebCore/crypto/CryptoAlgorithmParameters.h:50
> +    CryptoAlgorithmParameters() { }

Is it needed?

> Source/WebCore/crypto/SubtleCrypto.idl:32
> +    [Custom] Promise generateKey(AlgorithmIdentifier algorithm, boolean extractable, sequence<KeyUsage> keyUsages);

This is not really related to this patch, but we make the function custom just because of parsing some parameters.
Maybe we should find a keyword like CustomArguments that would allow to only define these custom arguments parsing routines.
This would solve the issue about argument error messages.
Would need some improvement at the binding generator so out-of-scope of this patch probably.

[Jiewen Tan]

Comment on attachment 292161 [details]
Patch

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

First of all, thanks Youenn for reviewing my patch.

>> Source/WebCore/ChangeLog:11
>> +           https://www.w3.org/TR/WebCryptoAPI/#SubtleCrypto-method-generateKey.
> 
> This spec is 2 years old.
> It might be good to point to https://w3c.github.io/webcrypto/Overview.html#SubtleCrypto-method-generateKey which is probably more up to date, although hopefully very close to the W3C spec.

I agree with you that the spec is too old and might soon changes. However, pointing to the GitHub version is not a reliable reference as well because it changes so fast and has not been formalized yet. Yet it is worth to point out the GitHub version. I actually refer to both the CR version and the GitHub version while doing this implementation.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:53
>> +static std::unique_ptr<CryptoAlgorithmParameters> nomalizeCryptoAlgorithmParameters(ExecState*, JSValue, Operations);
> 
> Would be good to pass an ExecState& for all these helper functions.

This was my first attempt actually. However, soon I realize mostly all other JS helper methods I call only accept ExecState*. Therefore, I adopt my codes to them.
Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:159
>> +    throwTypeError(state, scope, ASCIILiteral("Invalid AlgorithmIdentifier"));
> 
> Ideally we would like the exception messages to be like other non-custom methods, reusing throwArgumentTypeError.

Tried to fix this. It seems not quite applicable to my codes. throwArgumentTypeError needs the method name. However, this helper functions will be called by all multiple different methods. I tried to customized it with an Operations type. However, as you may notice, nomalizeCryptoAlgorithmParameters(..., Operations::GenerateKey) will call nomalizeCryptoAlgorithmParameters(..., Operations::Digest). Therefore, I have to remember the first caller's op instead of the current one. It makes the code more complicated which overweights the benefit it introduces. Therefore, I will probably not fix it.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:287
>> +    algorithm->generateKey(*params, extractable, keyUsages, WTFMove(callback), WTFMove(exceptionCallback), scriptExecutionContextFromExecState(&state));
> 
> Promise handling moved from a callback-pair version to passing a DeferredWrapper&&, or better a DOMPromise<KeyPair>&&.
> This allows rejecting/resolving promises more clearly.
> For instance, rejectWithException helper function would probably no longer be needed here and you can do fine-grained messages directly in the code.
> 
> This also allows to keep only one DeferredWrapper ref while we keep two with callbacks (plus some unneeded count churning).
> 
> This may have quite some impact on the current implementation.
> Maybe this could be done as a preliminary patch?

It was my initial attempt as well. However, I later notice that all the concrete algorithms need to perform the same things before resolve the CryptoKeyPair/CryptoKey. Therefore, I have to use a callback.

>> Source/WebCore/crypto/CryptoAlgorithmParameters.h:45
>> +public:
> 
> Use a struct if everything is public?

I agreed. However some of my previous patches' review process give me the impression that Structs in our code base tend to be simple and only store variables. This one doesn't meet the requirements. I made everything public is because our current binding generator can only assign Dictionary member by direct access instead of custom setters. Otherwise, I would like to use custom setters and make members private.

>> Source/WebCore/crypto/CryptoAlgorithmParameters.h:50
>> +    CryptoAlgorithmParameters() { }
> 
> Is it needed?

Removed.

>> Source/WebCore/crypto/SubtleCrypto.idl:32
>> +    [Custom] Promise generateKey(AlgorithmIdentifier algorithm, boolean extractable, sequence<KeyUsage> keyUsages);
> 
> This is not really related to this patch, but we make the function custom just because of parsing some parameters.
> Maybe we should find a keyword like CustomArguments that would allow to only define these custom arguments parsing routines.
> This would solve the issue about argument error messages.
> Would need some improvement at the binding generator so out-of-scope of this patch probably.

+1

[Jiewen Tan]

Created attachment 292255 [details]
Patch

[WebKit Commit Bot]

Attachment 292255 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 149 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Created attachment 292289 [details]
Patch

[WebKit Commit Bot]

Attachment 292289 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 149 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

EFL Error:

Source/WebCore/CMakeFiles/WebCore.dir/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp.o -c ../../Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp
../../Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:98:6: error: prototype for 'void WebCore::CryptoKeyRSA::generatePair(WebCore::CryptoAlgorithmIdentifier, WebCore::CryptoAlgorithmIdentifier, bool, unsigned int, const WTF::Vector<unsigned char>&, bool, WebCore::CryptoKeyUsage, WebCore::CryptoKeyRSA::KeyPairCallback, WebCore::CryptoKeyRSA::VoidCallback)' does not match any in class 'WebCore::CryptoKeyRSA'
 void CryptoKeyRSA::generatePair(CryptoAlgorithmIdentifier algorithm, CryptoAlgorithmIdentifier hash, bool hasHash, unsigned modulusLength, const Vector<uint8_t>& publicExponent, bool extractable, CryptoKeyUsage usage, KeyPairCallback callback, VoidCallback failureCallback)
      ^
In file included from ../../Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:27:0:
../../Source/WebCore/crypto/keys/CryptoKeyRSA.h:101:17: error: candidate is: static void WebCore::CryptoKeyRSA::generatePair(WebCore::CryptoAlgorithmIdentifier, WebCore::CryptoAlgorithmIdentifier, bool, unsigned int, const WTF::Vector<unsigned char>&, bool, WebCore::CryptoKeyUsage, WebCore::CryptoKeyRSA::KeyPairCallback, WebCore::CryptoKeyRSA::VoidCallback, WebCore::ScriptExecutionContext*)
     static void generatePair(CryptoAlgorithmIdentifier, CryptoAlgorithmIdentifier hash, bool hasHash, unsigned modulusLength, const Vector<uint8_t>& publicExponent, bool extractable, CryptoKeyUsage, KeyPairCallback, VoidCallback failureCallback, ScriptExecutionContext*);

[Jiewen Tan]

Created attachment 292294 [details]
Patch

[Jiewen Tan]

(In reply to comment #10)
> EFL Error:
> 
> Source/WebCore/CMakeFiles/WebCore.dir/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp.o
> -c ../../Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp
> ../../Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:98:6: error:
> prototype for 'void
> WebCore::CryptoKeyRSA::generatePair(WebCore::CryptoAlgorithmIdentifier,
> WebCore::CryptoAlgorithmIdentifier, bool, unsigned int, const
> WTF::Vector<unsigned char>&, bool, WebCore::CryptoKeyUsage,
> WebCore::CryptoKeyRSA::KeyPairCallback,
> WebCore::CryptoKeyRSA::VoidCallback)' does not match any in class
> 'WebCore::CryptoKeyRSA'
>  void CryptoKeyRSA::generatePair(CryptoAlgorithmIdentifier algorithm,
> CryptoAlgorithmIdentifier hash, bool hasHash, unsigned modulusLength, const
> Vector<uint8_t>& publicExponent, bool extractable, CryptoKeyUsage usage,
> KeyPairCallback callback, VoidCallback failureCallback)
>       ^
> In file included from
> ../../Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:27:0:
> ../../Source/WebCore/crypto/keys/CryptoKeyRSA.h:101:17: error: candidate is:
> static void
> WebCore::CryptoKeyRSA::generatePair(WebCore::CryptoAlgorithmIdentifier,
> WebCore::CryptoAlgorithmIdentifier, bool, unsigned int, const
> WTF::Vector<unsigned char>&, bool, WebCore::CryptoKeyUsage,
> WebCore::CryptoKeyRSA::KeyPairCallback, WebCore::CryptoKeyRSA::VoidCallback,
> WebCore::ScriptExecutionContext*)
>      static void generatePair(CryptoAlgorithmIdentifier,
> CryptoAlgorithmIdentifier hash, bool hasHash, unsigned modulusLength, const
> Vector<uint8_t>& publicExponent, bool extractable, CryptoKeyUsage,
> KeyPairCallback, VoidCallback failureCallback, ScriptExecutionContext*);

Fixed.

[WebKit Commit Bot]

Attachment 292294 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 150 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Created attachment 292302 [details]
Patch

[WebKit Commit Bot]

Attachment 292302 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 150 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 292302 [details]
Patch

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

Sorry, I did not have time to do a full review yet but here is some feedback.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:53
> +static std::unique_ptr<CryptoAlgorithmParameters> nomalizeCryptoAlgorithmParameters(ExecState&, JSValue, Operations);

Typo: nomalize

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:55
> +static CryptoAlgorithmIdentifier getHashIdentifier(ExecState& state, JSValue value)

We normally do not use get prefix in WebKit for getters. Maybe hashIdentifier() or toHashIdentifier().

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:65
> +static std::unique_ptr<CryptoAlgorithmParameters> nomalizeCryptoAlgorithmParameters(ExecState& state, JSValue value, Operations op)

We do not use abbreviations in WebKit: op -> operation

There is also a typo: nomalize

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:71
> +        String algorithmName = value.toString(&state)->value(&state);

You need a RETURN_IF_EXCEPTION() after this call as it may throw.

Also, there is a value.toWTFString(&state) for convenience.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:72
> +        JSObject* newParams = constructEmptyObject(&state);

Probably need a RETURN_IF_EXCEPTION() after this one too since we pass state.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:74
> +        newParams->putDirect(vm, identifier, jsString(&state, algorithmName));

Probably need a RETURN_IF_EXCEPTION() after this one too since we pass state.

Also, it does not make sense to reconstruct a JSString from your algorithmName String.
newParams->putDirect(vm, identifier, value); seems like it should just work. In which case, you don't even need to convert the JSValue to a WTFString in the first place.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:83
> +        if (!CryptoAlgorithmRegistry::singleton().getIdentifierForName(params.name, identifier)) {

Another getter with a get prefix :(

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:95
> +                params.arrayToVector();

This looks super weird. You'd expect a method named like this to return something.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:168
> +    if (!isJSArray(value)) {

This is not how you convert an IDL sequence. I believe you'll need to use forEachInIterable(). You only support arrays as input right now, which is wrong.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:178
> +        String usageString = element.toString(&state)->value(&state);

toWTFString().

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:252
> +    RETURN_IF_EXCEPTION(scope, static_cast<void>(0));

I think the following would work:
RETURN_IF_EXCEPTION(scope, void());

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:284
> +    // The spec suggests we should perform the following task asyncronizedly regardless what kind of keys it produces

I don't think asyncronizedly is a word.

> Source/WebCore/crypto/CryptoAlgorithm.h:58
> +    typedef WTF::Function<void(bool)> BoolCallback;

We prefer 'using' instead of typedef nowadays:
using BoolCallback = WTF::Function<void(bool)>;

> Source/WebCore/crypto/CryptoAlgorithm.h:67
> +    // The followings will be deprecated.

'following' without 's' I believe.

> Source/WebCore/crypto/CryptoAlgorithmParameters.h:36
> +enum class CryptoAlgorithmParametersClass {

This duplicates the name of the class. Could it be moved inside the class so we can use a shorter name?

e.g. CryptoAlgorithmParameters::Class::AesKeyGenParams.

> Source/WebCore/crypto/CryptoAlgorithmParameters.h:46
> +    // Maybe we could somehow combine name and identifier?

// FIXME: Consider merging name and identifier.

> Source/WebCore/crypto/CryptoKey.cpp:71
> +void CryptoKey::intersects(CryptoKeyUsage usage)

The naming is weird.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69
> +void CryptoAlgorithmAES_CBC::generateKey(const CryptoAlgorithmParameters& parameters, bool extractable, CryptoKeyUsage usages, KeyOrKeyPairCallback&& callback, ExceptionCallback&& exceptionCallback, ScriptExecutionContext*)

I think we may want to consider passing parameters to the implementation as a std::unique_ptr<CryptoAlgorithmParameters>&& considering you're using polymorphism.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:73
> +    if (usages & (CryptoKeyUsageSign | CryptoKeyUsageVerify | CryptoKeyUsageDeriveKey | CryptoKeyUsageDeriveBits)) {

It'd be nice to use WTF::OptionSet.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:85
> +    RefPtr<CryptoKeyHMAC> result = CryptoKeyHMAC::generate(hmacParameters.length ? hmacParameters.length.value() : 0, hmacParameters.hashIdentifier, extractable, usages);

hmacParameters.length.valueOr(0)

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:80
> +        pair.publicKey()->intersects(CryptoKeyUsageEncrypt);

insersects sounds like a setter, but it modifies the key.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:99
> +    typedef WTF::Function<void(CryptoKeyPair&)> KeyPairCallback;

typedef -> using

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:233
> +void CryptoKeyRSA::generatePair(CryptoAlgorithmIdentifier algorithm, CryptoAlgorithmIdentifier hash, bool hasHash, unsigned modulusLength, const Vector<uint8_t>& publicExponent, bool extractable, CryptoKeyUsage usage, KeyPairCallback callback, VoidCallback failureCallback, ScriptExecutionContext* context)

It'd be good if Brady took a look at this. Seems weird we have to pass the ScriptExecutionContext just to ref it.

> Source/WebCore/crypto/parameters/CryptoAlgorithmHmacKeyGenParams.h:35
> +class CryptoAlgorithmHmacKeyGenParams final : public CryptoAlgorithmParameters {

Where is the IDL for this?

> Source/WebCore/crypto/parameters/CryptoAlgorithmHmacKeyGenParams.h:37
> +    // Maybe we could somehow combine hash and hashIdentifier?

FIXME: consider...

> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaHashedKeyGenParams.h:34
> +class CryptoAlgorithmRsaHashedKeyGenParams final : public CryptoAlgorithmRsaKeyGenParams {

Why no IDL for this?

> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:36
> +class CryptoAlgorithmRsaKeyGenParams : public CryptoAlgorithmParameters {

Why no IDL for this?

Should at least some of these be marked as final?

> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:45
> +    void arrayToVector() { publicExponentVector.append(publicExponent->data(), publicExponent->byteLength()); }

This is weird.

> Source/WebCore/crypto/parameters/RsaHashedKeyGenParams.idl:30
> +    // FIXME: We currently lack of support for Union with Dictionary type.

I do not believe this is true anymore.

[Chris Dumez]

Comment on attachment 292302 [details]
Patch

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

>> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:45
>> +    void arrayToVector() { publicExponentVector.append(publicExponent->data(), publicExponent->byteLength()); }
> 
> This is weird.

I can see at least one way to improve this:
1. Make publicExponentVector private.
2. Add a getter for publicExponentVector() which populates it from publicExponent if missing.

Depending on if publicExponentVector is accessed repeatedly or not, we may not even need to cache the Vector into a member.

[Jiewen Tan]

Comment on attachment 292302 [details]
Patch

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

First of all, thanks Chris for reviewing my patch.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:53
>> +static std::unique_ptr<CryptoAlgorithmParameters> nomalizeCryptoAlgorithmParameters(ExecState&, JSValue, Operations);
> 
> Typo: nomalize

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:55
>> +static CryptoAlgorithmIdentifier getHashIdentifier(ExecState& state, JSValue value)
> 
> We normally do not use get prefix in WebKit for getters. Maybe hashIdentifier() or toHashIdentifier().

Used toHashIdentifier().

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:65
>> +static std::unique_ptr<CryptoAlgorithmParameters> nomalizeCryptoAlgorithmParameters(ExecState& state, JSValue value, Operations op)
> 
> We do not use abbreviations in WebKit: op -> operation
> 
> There is also a typo: nomalize

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:71
>> +        String algorithmName = value.toString(&state)->value(&state);
> 
> You need a RETURN_IF_EXCEPTION() after this call as it may throw.
> 
> Also, there is a value.toWTFString(&state) for convenience.

Removed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:72
>> +        JSObject* newParams = constructEmptyObject(&state);
> 
> Probably need a RETURN_IF_EXCEPTION() after this one too since we pass state.

It seems that it won't throw exceptions.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:74
>> +        newParams->putDirect(vm, identifier, jsString(&state, algorithmName));
> 
> Probably need a RETURN_IF_EXCEPTION() after this one too since we pass state.
> 
> Also, it does not make sense to reconstruct a JSString from your algorithmName String.
> newParams->putDirect(vm, identifier, value); seems like it should just work. In which case, you don't even need to convert the JSValue to a WTFString in the first place.

It seems that it won't throw exceptions.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:83
>> +        if (!CryptoAlgorithmRegistry::singleton().getIdentifierForName(params.name, identifier)) {
> 
> Another getter with a get prefix :(

CryptoAlgorithmRegistry is shared with the old interface. Probably needs a separate bug to get rid of it.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:95
>> +                params.arrayToVector();
> 
> This looks super weird. You'd expect a method named like this to return something.

Adopted the newer comments.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:168
>> +    if (!isJSArray(value)) {
> 
> This is not how you convert an IDL sequence. I believe you'll need to use forEachInIterable(). You only support arrays as input right now, which is wrong.

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:178
>> +        String usageString = element.toString(&state)->value(&state);
> 
> toWTFString().

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:252
>> +    RETURN_IF_EXCEPTION(scope, static_cast<void>(0));
> 
> I think the following would work:
> RETURN_IF_EXCEPTION(scope, void());

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:284
>> +    // The spec suggests we should perform the following task asyncronizedly regardless what kind of keys it produces
> 
> I don't think asyncronizedly is a word.

Oops. Used asynchronously instead.

>> Source/WebCore/crypto/CryptoAlgorithm.h:58
>> +    typedef WTF::Function<void(bool)> BoolCallback;
> 
> We prefer 'using' instead of typedef nowadays:
> using BoolCallback = WTF::Function<void(bool)>;

Fixed.

>> Source/WebCore/crypto/CryptoAlgorithm.h:67
>> +    // The followings will be deprecated.
> 
> 'following' without 's' I believe.

Fixed.

>> Source/WebCore/crypto/CryptoAlgorithmParameters.h:36
>> +enum class CryptoAlgorithmParametersClass {
> 
> This duplicates the name of the class. Could it be moved inside the class so we can use a shorter name?
> 
> e.g. CryptoAlgorithmParameters::Class::AesKeyGenParams.

Fixed.

>> Source/WebCore/crypto/CryptoAlgorithmParameters.h:46
>> +    // Maybe we could somehow combine name and identifier?
> 
> // FIXME: Consider merging name and identifier.

Fixed.

>> Source/WebCore/crypto/CryptoKey.cpp:71
>> +void CryptoKey::intersects(CryptoKeyUsage usage)
> 
> The naming is weird.

intersectsUsagesWith()?

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69
>> +void CryptoAlgorithmAES_CBC::generateKey(const CryptoAlgorithmParameters& parameters, bool extractable, CryptoKeyUsage usages, KeyOrKeyPairCallback&& callback, ExceptionCallback&& exceptionCallback, ScriptExecutionContext*)
> 
> I think we may want to consider passing parameters to the implementation as a std::unique_ptr<CryptoAlgorithmParameters>&& considering you're using polymorphism.

Sure.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:73
>> +    if (usages & (CryptoKeyUsageSign | CryptoKeyUsageVerify | CryptoKeyUsageDeriveKey | CryptoKeyUsageDeriveBits)) {
> 
> It'd be nice to use WTF::OptionSet.

CryptoKeyUsage is shared with the old interface. Probably need a separate bug to make the change.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:85
>> +    RefPtr<CryptoKeyHMAC> result = CryptoKeyHMAC::generate(hmacParameters.length ? hmacParameters.length.value() : 0, hmacParameters.hashIdentifier, extractable, usages);
> 
> hmacParameters.length.valueOr(0)

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:80
>> +        pair.publicKey()->intersects(CryptoKeyUsageEncrypt);
> 
> insersects sounds like a setter, but it modifies the key.

intersectsUsagesWith()?

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:99
>> +    typedef WTF::Function<void(CryptoKeyPair&)> KeyPairCallback;
> 
> typedef -> using

Fixed.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:233
>> +void CryptoKeyRSA::generatePair(CryptoAlgorithmIdentifier algorithm, CryptoAlgorithmIdentifier hash, bool hasHash, unsigned modulusLength, const Vector<uint8_t>& publicExponent, bool extractable, CryptoKeyUsage usage, KeyPairCallback callback, VoidCallback failureCallback, ScriptExecutionContext* context)
> 
> It'd be good if Brady took a look at this. Seems weird we have to pass the ScriptExecutionContext just to ref it.

Nope. We don't only ref it. The ref operation is to keep the passed context live long enough for the secondary thread to post the callback back to the main/worker thread. It will be deref once the callback is returned.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmHmacKeyGenParams.h:35
>> +class CryptoAlgorithmHmacKeyGenParams final : public CryptoAlgorithmParameters {
> 
> Where is the IDL for this?

In the later part.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmHmacKeyGenParams.h:37
>> +    // Maybe we could somehow combine hash and hashIdentifier?
> 
> FIXME: consider...

Fixed.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaHashedKeyGenParams.h:34
>> +class CryptoAlgorithmRsaHashedKeyGenParams final : public CryptoAlgorithmRsaKeyGenParams {
> 
> Why no IDL for this?

In the later part.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:36
>> +class CryptoAlgorithmRsaKeyGenParams : public CryptoAlgorithmParameters {
> 
> Why no IDL for this?
> 
> Should at least some of these be marked as final?

In the later part. This one is not final. RsaHashedKeyGenParams inherits it.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:45
>> +    void arrayToVector() { publicExponentVector.append(publicExponent->data(), publicExponent->byteLength()); }
> 
> This is weird.

Fixed.

>> Source/WebCore/crypto/parameters/RsaHashedKeyGenParams.idl:30
>> +    // FIXME: We currently lack of support for Union with Dictionary type.
> 
> I do not believe this is true anymore.

Need to rebase my code. Will upload a patch later on to address this problem.

[Jiewen Tan]

Created attachment 292425 [details]
Patch

[WebKit Commit Bot]

Attachment 292425 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 150 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 292302 [details]
Patch

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

>>> Source/WebCore/crypto/parameters/RsaHashedKeyGenParams.idl:30
>>> +    // FIXME: We currently lack of support for Union with Dictionary type.
>> 
>> I do not believe this is true anymore.
> 
> Need to rebase my code. Will upload a patch later on to address this problem.

Tried to use Union, then felt any is more suitable in this scenario.

[Jiewen Tan]

Created attachment 292441 [details]
Patch

[WebKit Commit Bot]

Attachment 292441 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 150 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Created attachment 292628 [details]
Patch

[WebKit Commit Bot]

Attachment 292628 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 150 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 292628 [details]
Patch

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

r=me with comments

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:73
> +        Identifier identifier = Identifier::fromString(&vm, "name");

nit: I don't think having this as a local variable is really useful, I would just inline it.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:103
> +                auto hashIdentifier = toHashIdentifier(state, params.hash);

nit: I would directly assign to params.hashIdentifier.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:123
> +                auto hashIdentifier = toHashIdentifier(state, params.hash);

nit: I would directly assign to params.hashIdentifier.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:237
> +    if (state.argumentCount() < 3) {

We normally use UNLIKELY() for these sort of checks in the bindings.

> Source/WebCore/crypto/CryptoAlgorithmParameters.idl:28
> +    ImplementedAs=CryptoAlgorithmParameters

This is likely not needed because the dictionary is in its own IDL.

> Source/WebCore/crypto/CryptoKey.cpp:71
> +void CryptoKey::setUsagesBitmap(CryptoKeyUsage usage)

nit: I would just inline this in the header, similarly to the getter.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:71
> +    const CryptoAlgorithmAesKeyGenParams& aesParameters = downcast<CryptoAlgorithmAesKeyGenParams>(*parameters);

auto& aesParameters

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:78
> +    RefPtr<CryptoKeyAES> result = CryptoKeyAES::generate(CryptoAlgorithmIdentifier::AES_CBC, aesParameters.length, extractable, usages);

If generate() returns a RefPtr<>, this should use auto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:70
> +    const CryptoAlgorithmAesKeyGenParams& aesParameters = downcast<CryptoAlgorithmAesKeyGenParams>(*parameters);

auto&

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:77
> +    RefPtr<CryptoKeyAES> result = CryptoKeyAES::generate(CryptoAlgorithmIdentifier::AES_KW, aesParameters.length, extractable, usages);

If generate() returns a RefPtr<>, this should use auto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:73
> +    const CryptoAlgorithmHmacKeyGenParams& hmacParameters = downcast<CryptoAlgorithmHmacKeyGenParams>(*parameters);

auto&

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:85
> +    RefPtr<CryptoKeyHMAC> result = CryptoKeyHMAC::generate(hmacParameters.length.valueOr(0), hmacParameters.hashIdentifier, extractable, usages);

If generate() returns a RefPtr<>, this should use auto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:72
> +    const CryptoAlgorithmRsaKeyGenParams& rsaParameters = downcast<CryptoAlgorithmRsaKeyGenParams>(*parameters);

auto&

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:76
> +    const CryptoAlgorithmRsaHashedKeyGenParams& rsaParameters = downcast<CryptoAlgorithmRsaHashedKeyGenParams>(*parameters);

auto&

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:76
> +    const CryptoAlgorithmRsaHashedKeyGenParams& rsaParameters = downcast<CryptoAlgorithmRsaHashedKeyGenParams>(*parameters);

auto&

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:246
> +    context->ref();

A nicer way to do this would be to have a:
Ref<ScriptExecutionContext> protectedScriptExecutionContext(*context);

and then capture it in both lambdas below like so:
[protectedScriptExecutionContext = protectedScriptExecutionContext.copyRef()]

We should really avoid explicit ref/derefing.

> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:43
> +    Vector<uint8_t>& publicExponentVector() const

The method is const, I think it should return a const Vector&

> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:45
> +        if (!m_publicExponentVector.isEmpty())

if (!m_publicExponentVector.isEmpty() || !publicExponent->byteLength())

> LayoutTests/imported/w3c/WebCryptoAPI/generateKey/test_aes-ctr-expected.txt:6
> +FAIL Success: generateKey({length: 128, name: AES-CTR}, false, [encrypt]) assert_unreached: Threw an unexpected error: NotSupportedError (DOM Exception 9): The operation is not supported. Reached unreachable code

Is this expected to fail?

> LayoutTests/imported/w3c/WebCryptoAPI/generateKey/test_failures-expected.txt:328
> +FAIL Bad usages: generateKey({length: 128, name: AES-CTR}, true, [sign]) assert_equals: Bad usages not supported expected "SyntaxError" but got "NotSupportedError"

Is this expected to fail?

> LayoutTests/imported/w3c/WebCryptoAPI/generateKey/test_failures-expected.txt:2354
> +FAIL Empty usages: generateKey({hash: SHA-256, modulusLength: 2048, name: RSA-PSS, publicExponent: {0: 1, 1: 0, 2: 1}}, false, []) assert_equals: Empty usages not supported expected "SyntaxError" but got "NotSupportedError"

Is this expected to fail?

[Chris Dumez]

Please make sure it builds everywhere before landing.

[Jiewen Tan]

Comment on attachment 292628 [details]
Patch

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

Thanks Chris for r+ my patch.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:73
>> +        Identifier identifier = Identifier::fromString(&vm, "name");
> 
> nit: I don't think having this as a local variable is really useful, I would just inline it.

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:103
>> +                auto hashIdentifier = toHashIdentifier(state, params.hash);
> 
> nit: I would directly assign to params.hashIdentifier.

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:123
>> +                auto hashIdentifier = toHashIdentifier(state, params.hash);
> 
> nit: I would directly assign to params.hashIdentifier.

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:237
>> +    if (state.argumentCount() < 3) {
> 
> We normally use UNLIKELY() for these sort of checks in the bindings.

Fixed.

>> Source/WebCore/crypto/CryptoAlgorithmParameters.idl:28
>> +    ImplementedAs=CryptoAlgorithmParameters
> 
> This is likely not needed because the dictionary is in its own IDL.

Fixed.

>> Source/WebCore/crypto/CryptoKey.cpp:71
>> +void CryptoKey::setUsagesBitmap(CryptoKeyUsage usage)
> 
> nit: I would just inline this in the header, similarly to the getter.

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:71
>> +    const CryptoAlgorithmAesKeyGenParams& aesParameters = downcast<CryptoAlgorithmAesKeyGenParams>(*parameters);
> 
> auto& aesParameters

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:78
>> +    RefPtr<CryptoKeyAES> result = CryptoKeyAES::generate(CryptoAlgorithmIdentifier::AES_CBC, aesParameters.length, extractable, usages);
> 
> If generate() returns a RefPtr<>, this should use auto.

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:70
>> +    const CryptoAlgorithmAesKeyGenParams& aesParameters = downcast<CryptoAlgorithmAesKeyGenParams>(*parameters);
> 
> auto&

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:77
>> +    RefPtr<CryptoKeyAES> result = CryptoKeyAES::generate(CryptoAlgorithmIdentifier::AES_KW, aesParameters.length, extractable, usages);
> 
> If generate() returns a RefPtr<>, this should use auto.

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:73
>> +    const CryptoAlgorithmHmacKeyGenParams& hmacParameters = downcast<CryptoAlgorithmHmacKeyGenParams>(*parameters);
> 
> auto&

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:85
>> +    RefPtr<CryptoKeyHMAC> result = CryptoKeyHMAC::generate(hmacParameters.length.valueOr(0), hmacParameters.hashIdentifier, extractable, usages);
> 
> If generate() returns a RefPtr<>, this should use auto.

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:72
>> +    const CryptoAlgorithmRsaKeyGenParams& rsaParameters = downcast<CryptoAlgorithmRsaKeyGenParams>(*parameters);
> 
> auto&

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:76
>> +    const CryptoAlgorithmRsaHashedKeyGenParams& rsaParameters = downcast<CryptoAlgorithmRsaHashedKeyGenParams>(*parameters);
> 
> auto&

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:76
>> +    const CryptoAlgorithmRsaHashedKeyGenParams& rsaParameters = downcast<CryptoAlgorithmRsaHashedKeyGenParams>(*parameters);
> 
> auto&

Fixed.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:246
>> +    context->ref();
> 
> A nicer way to do this would be to have a:
> Ref<ScriptExecutionContext> protectedScriptExecutionContext(*context);
> 
> and then capture it in both lambdas below like so:
> [protectedScriptExecutionContext = protectedScriptExecutionContext.copyRef()]
> 
> We should really avoid explicit ref/derefing.

Seems the block under prevents me from using Ref. And if I use RefPtr instead, it causes crashes. Actually Anders suggested me to use the explicit ref/derefing here. I guess it should be fine for ScriptExecutionContext.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:43
>> +    Vector<uint8_t>& publicExponentVector() const
> 
> The method is const, I think it should return a const Vector&

Fixed.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h:45
>> +        if (!m_publicExponentVector.isEmpty())
> 
> if (!m_publicExponentVector.isEmpty() || !publicExponent->byteLength())

Fixed.

>> LayoutTests/imported/w3c/WebCryptoAPI/generateKey/test_aes-ctr-expected.txt:6
>> +FAIL Success: generateKey({length: 128, name: AES-CTR}, false, [encrypt]) assert_unreached: Threw an unexpected error: NotSupportedError (DOM Exception 9): The operation is not supported. Reached unreachable code
> 
> Is this expected to fail?

Yes, we currently don't support AES-CTR.

>> LayoutTests/imported/w3c/WebCryptoAPI/generateKey/test_failures-expected.txt:328
>> +FAIL Bad usages: generateKey({length: 128, name: AES-CTR}, true, [sign]) assert_equals: Bad usages not supported expected "SyntaxError" but got "NotSupportedError"
> 
> Is this expected to fail?

Sure. We currently don't support AES-CTR.

>> LayoutTests/imported/w3c/WebCryptoAPI/generateKey/test_failures-expected.txt:2354
>> +FAIL Empty usages: generateKey({hash: SHA-256, modulusLength: 2048, name: RSA-PSS, publicExponent: {0: 1, 1: 0, 2: 1}}, false, []) assert_equals: Empty usages not supported expected "SyntaxError" but got "NotSupportedError"
> 
> Is this expected to fail?

Sure. We currently don't support RSA-PSS.

[Jiewen Tan]

Created attachment 292666 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 292666 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 149 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Created attachment 292671 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 292671 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:74:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:118:  CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106:  CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:111:  CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:74:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:118:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:111:  CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 149 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Committed r207809: <http://trac.webkit.org/changeset/207809>