Bug 128748 – WebCrypto algorithms should be exposed via KeyAlgorithm interface

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

WebCrypto spec used to have Key.algorithm as dictionary, which is invalid per WebIDL.

Now it's a KeyAlgorithm interface, and there is going to be a better specification of what's exposed for each type of key.

[Alexey Proskuryakov]

See <https://dvcs.w3.org/hg/webcrypto-api/rev/2fa3494f0179> and <https://dvcs.w3.org/hg/webcrypto-api/rev/2fa3494f0179>.

[Radar WebKit Bug Importer]

<rdar://problem/27359438>

[Jiewen Tan]

Created attachment 288055 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 16 files


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

[Jiewen Tan]

Created attachment 288274 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 16 files


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

[Brent Fulgham]

Comment on attachment 288274 [details]
Patch

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

Very nice! r=me. Please correct the typos I mentioned before landing.

> Source/WebCore/ChangeLog:12
> +        No change of behaviors.

No change in behavior, so no new tests.

> Source/WebCore/ChangeLog:17
> +        Remove CryptoAlgorithmDescriptionBuilder.cpp.

Do you know why this class existed? Seems like it might have been intended to be fleshed out later, but was never completed.

> Source/WebCore/ChangeLog:25
> +        Get ride of dependency on CryptoAlgorithmDescriptionBuilder.

Get rid of dependency ...

[Jiewen Tan]

Comment on attachment 288274 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        Remove CryptoAlgorithmDescriptionBuilder.cpp.
> 
> Do you know why this class existed? Seems like it might have been intended to be fleshed out later, but was never completed.

Previously, this is the base (virtual) class of JSCryptoAlgorithmBuilder, which is created to add information to the JSObject that describes algorithms binding to the key. I am not quite sure why it is needed, but I assume that Alexey did it before is try to separate codes relating to JS from non-JS ones. So, with this class, the callers, i.e. non-JS codes will not need the knowledge of underlying implementation, i.e. JS codes to add information to describe corresponding algorithms.
However, in the new implementation, I will only return the KeyAlgorithm object to JS codes and the JS codes itself convert the object to a JSObject. Therefore, this base class is not needed. The actual JSCryptoAlgorithmBuilder is therefore kept to do the conversion.

[Jiewen Tan]

Comment on attachment 288274 [details]
Patch

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

Thanks Brent for reviewing my patch.

>> Source/WebCore/ChangeLog:12
>> +        No change of behaviors.
> 
> No change in behavior, so no new tests.

Fixed.

>> Source/WebCore/ChangeLog:25
>> +        Get ride of dependency on CryptoAlgorithmDescriptionBuilder.
> 
> Get rid of dependency ...

Fixed.

[Jiewen Tan]

Created attachment 288613 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 16 files


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

[Jiewen Tan]

Created attachment 288655 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 17 files


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

[Chris Dumez]

Comment on attachment 288655 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:10
> +        defined by the spec.

Please link to the spec that define this interface in the Changelog.

[Jiewen Tan]

Created attachment 288657 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 17 files


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

[Chris Dumez]

Comment on attachment 288657 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:3
> +        WebCrypto algorithms should be exposed via KeyAlgorithm interface

KeyAlgorithm is a dictionary, not an interface

> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:48
> +        AesKeyAlgorithm& aesAlgorithm = downcast<AesKeyAlgorithm>(algorithm.get());

auto&

> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:54
> +        HmacKeyAlgorithm& hmacAlgorithm = downcast<HmacKeyAlgorithm>(algorithm.get());

auto&

> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:63
> +        RsaHashedKeyAlgorithm& rsaAlgorithm = downcast<RsaHashedKeyAlgorithm>(algorithm.get());

auto&

> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:73
> +        RsaKeyAlgorithm& rsaAlgorithm = downcast<RsaKeyAlgorithm>(algorithm.get());

auto&

> Source/WebCore/crypto/CryptoKey.h:64
> +    String name;

Why is this public?

> Source/WebCore/crypto/CryptoKey.h:66
> +    KeyAlgorithm(const String& n)

RefCounted classes normally do not have a public constructor but a factory function that returns a Ref<> since we don't want those allocated on the stack.

> Source/WebCore/crypto/keys/CryptoKeyAES.h:39
> +    size_t length;

Why is this public?

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38
> +    KeyHashAlgorithm hash;

Why are those public?

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52
> +    size_t modulusLength;

Why are those public?

[Chris Dumez]

Comment on attachment 288657 [details]
Patch for landing

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

> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81
>      return builder.result();

The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?).

[Chris Dumez]

Comment on attachment 288657 [details]
Patch for landing

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

> Source/WebCore/crypto/CryptoKey.h:87
> +    virtual Ref<KeyAlgorithm> algorithm() const = 0;

Given that this constructs a new object and transfers ownership, I think we need to choose a better name for this method, maybe buildAlgorithm() or createAlgorithm().

> Source/WebCore/crypto/CryptoKey.h:104
> +    CryptoAlgorithmIdentifier m_algorithmIdentifier;

I think subclasses should use the algorithmIdentifier() getter instead of making this member protected.

[Chris Dumez]

Comment on attachment 288657 [details]
Patch for landing

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

> Source/WebCore/crypto/CryptoKey.h:55
> +    HRSA

Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it.

> Source/WebCore/crypto/CryptoKey.h:58
> +struct KeyHashAlgorithm {

Do we really need a struct for this? Why don't we use a String instead?

>> Source/WebCore/crypto/CryptoKey.h:66
>> +    KeyAlgorithm(const String& n)
> 
> RefCounted classes normally do not have a public constructor but a factory function that returns a Ref<> since we don't want those allocated on the stack.

This constructor should be protected and marked as explicit. This base class does not need a factory function since it is pure virtual, only subclasses do. The constructors in the subclasses should be protected or private, depending if the subclass is final or not.

> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86
> +    Vector<uint8_t> publicExponent;

Not needed.

> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:87
> +    return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, WTFMove(publicExponent)));

return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, {}));

> Source/WebCore/crypto/keys/CryptoKeyAES.h:37
> +class AesKeyAlgorithm : public KeyAlgorithm {

Should be final

> Source/WebCore/crypto/keys/CryptoKeyAES.h:41
> +    AesKeyAlgorithm(const String& n, size_t l)

Bad parameter names. Should be private and there should be a public factory function instead.

> Source/WebCore/crypto/keys/CryptoKeyAES.h:46
> +    virtual ~AesKeyAlgorithm()

I don't need this destructor it seems.

> Source/WebCore/crypto/keys/CryptoKeyAES.h:50
> +    KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::AES; }

override -> final

> Source/WebCore/crypto/keys/CryptoKeyAES.h:72
> +    Ref<KeyAlgorithm> algorithm() const override;

override -> final

> Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:71
> +    KeyHashAlgorithm hashAlgorithm;

KeyHashAlgorithm hashAlgorithm { CryptoAlgorithmRegistry::singleton().nameForIdentifier(m_hash) };

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:36
> +class HmacKeyAlgorithm : public KeyAlgorithm {

Should be final

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:41
> +    HmacKeyAlgorithm(const String& n, KeyHashAlgorithm&& h, size_t l)

Bad parameter names. Should be private.

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47
> +    virtual ~HmacKeyAlgorithm()

I don't need this destructor it seems.

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:51
> +    KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HMAC; }

override -> final

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:74
> +    Ref<KeyAlgorithm> algorithm() const override;

override -> final

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:55
> +    RsaKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p)

Bad parameter names. Should be protected.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:61
> +    virtual ~RsaKeyAlgorithm()

I don't need this destructor it seems.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:68
> +class RsaHashedKeyAlgorithm : public RsaKeyAlgorithm {

Should be final.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:70
> +    KeyHashAlgorithm hash;

Why is this public?

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:72
> +    RsaHashedKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p, KeyHashAlgorithm&& h)

Those parameter names are really not great. We try to avoid abbreviations in WebKit.
Should be private.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:77
> +    virtual ~RsaHashedKeyAlgorithm()

I don't need this destructor it seems.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:82
> +    KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HRSA; }

override -> final

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:109
> +    Ref<KeyAlgorithm> algorithm() const override;

override -> final

[Jiewen Tan]

Comment on attachment 288657 [details]
Patch for landing

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

>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81
>>      return builder.result();
> 
> The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?).

Thank you for pointing out. I forgot to implement it. Oops.

>> Source/WebCore/crypto/CryptoKey.h:55
>> +    HRSA
> 
> Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it.

There are two types of RSAs, one with hash, another without. So I name the one with hash as HRSA.

>> Source/WebCore/crypto/CryptoKey.h:58
>> +struct KeyHashAlgorithm {
> 
> Do we really need a struct for this? Why don't we use a String instead?

I am struggling with this part. The hash is supposed to be of type KeyAlgorithm as well. However, according to the current spec, only SHA1, 256, 384, 512 could serve the purpose of digest, see https://www.w3.org/TR/WebCryptoAPI/#algorithm-overview. Therefore, according to https://www.w3.org/TR/WebCryptoAPI/#sha and https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an-algorithm, the KeyAlgorithm type will only have the name attribute which is the below KeyAlgorithm virtual class. But I can't initialize a virtual class. The reason why the below KeyAlgorithm class is virtual is because I want the type-check downcast trait. Hence, I make a struct here. However, I think a String will serve the purpose as well.

>> Source/WebCore/crypto/CryptoKey.h:64
>> +    String name;
> 
> Why is this public?

I want a Struct initially, but later on realize that I would like to return a reference and use polymorphism as well. I can't think of any other approach to make it possible except inheriting RefCounted. Hence, those members are no needed to be private or protected. Do you have any better idea to achieve the same purpose with Struct only?

[Chris Dumez]

(In reply to comment #21)
> Comment on attachment 288657 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> 
> >> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81
> >>      return builder.result();
> > 
> > The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?).
> 
> Thank you for pointing out. I forgot to implement it. Oops.
> 
> >> Source/WebCore/crypto/CryptoKey.h:55
> >> +    HRSA
> > 
> > Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it.
> 
> There are two types of RSAs, one with hash, another without. So I name the
> one with hash as HRSA.
> 
> >> Source/WebCore/crypto/CryptoKey.h:58
> >> +struct KeyHashAlgorithm {
> > 
> > Do we really need a struct for this? Why don't we use a String instead?
> 
> I am struggling with this part. The hash is supposed to be of type
> KeyAlgorithm as well. However, according to the current spec, only SHA1,
> 256, 384, 512 could serve the purpose of digest, see
> https://www.w3.org/TR/WebCryptoAPI/#algorithm-overview. Therefore, according
> to https://www.w3.org/TR/WebCryptoAPI/#sha and
> https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an-
> algorithm, the KeyAlgorithm type will only have the name attribute which is
> the below KeyAlgorithm virtual class. But I can't initialize a virtual
> class. The reason why the below KeyAlgorithm class is virtual is because I
> want the type-check downcast trait. Hence, I make a struct here. However, I
> think a String will serve the purpose as well.
> 
> >> Source/WebCore/crypto/CryptoKey.h:64
> >> +    String name;
> > 
> > Why is this public?
> 
> I want a Struct initially, but later on realize that I would like to return
> a reference and use polymorphism as well. I can't think of any other
> approach to make it possible except inheriting RefCounted. Hence, those
> members are no needed to be private or protected. Do you have any better
> idea to achieve the same purpose with Struct only?

Given how this is currently implemented, I still do not think they should be public.

[Chris Dumez]

(In reply to comment #22)
> (In reply to comment #21)
> > Comment on attachment 288657 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=288657&action=review
> > 
> > >> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81
> > >>      return builder.result();
> > > 
> > > The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?).
> > 
> > Thank you for pointing out. I forgot to implement it. Oops.
> > 
> > >> Source/WebCore/crypto/CryptoKey.h:55
> > >> +    HRSA
> > > 
> > > Where is HRSA coming from? I do not see it in the spec. And this change is not mentioned in the Changelog unless I missed it.
> > 
> > There are two types of RSAs, one with hash, another without. So I name the
> > one with hash as HRSA.
> > 
> > >> Source/WebCore/crypto/CryptoKey.h:58
> > >> +struct KeyHashAlgorithm {
> > > 
> > > Do we really need a struct for this? Why don't we use a String instead?
> > 
> > I am struggling with this part. The hash is supposed to be of type
> > KeyAlgorithm as well. However, according to the current spec, only SHA1,
> > 256, 384, 512 could serve the purpose of digest, see
> > https://www.w3.org/TR/WebCryptoAPI/#algorithm-overview. Therefore, according
> > to https://www.w3.org/TR/WebCryptoAPI/#sha and
> > https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an-
> > algorithm, the KeyAlgorithm type will only have the name attribute which is
> > the below KeyAlgorithm virtual class. But I can't initialize a virtual
> > class. The reason why the below KeyAlgorithm class is virtual is because I
> > want the type-check downcast trait. Hence, I make a struct here. However, I
> > think a String will serve the purpose as well.
> > 
> > >> Source/WebCore/crypto/CryptoKey.h:64
> > >> +    String name;
> > > 
> > > Why is this public?
> > 
> > I want a Struct initially, but later on realize that I would like to return
> > a reference and use polymorphism as well. I can't think of any other
> > approach to make it possible except inheriting RefCounted. Hence, those
> > members are no needed to be private or protected. Do you have any better
> > idea to achieve the same purpose with Struct only?
> 
> Given how this is currently implemented, I still do not think they should be
> public.

I have not looked in too much details but if you do not need ref counting, the factory could return a unique_ptr instead of a Ref, and you would not need to subclass RefCounted.

[Jiewen Tan]

Comment on attachment 288657 [details]
Patch for landing

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

Thank you for reviewing my patch, Chris.

>> Source/WebCore/ChangeLog:3
>> +        WebCrypto algorithms should be exposed via KeyAlgorithm interface
> 
> KeyAlgorithm is a dictionary, not an interface

Fixed.

>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:48
>> +        AesKeyAlgorithm& aesAlgorithm = downcast<AesKeyAlgorithm>(algorithm.get());
> 
> auto&

Fixed.

>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:54
>> +        HmacKeyAlgorithm& hmacAlgorithm = downcast<HmacKeyAlgorithm>(algorithm.get());
> 
> auto&

Fixed.

>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:63
>> +        RsaHashedKeyAlgorithm& rsaAlgorithm = downcast<RsaHashedKeyAlgorithm>(algorithm.get());
> 
> auto&

Fixed.

>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:73
>> +        RsaKeyAlgorithm& rsaAlgorithm = downcast<RsaKeyAlgorithm>(algorithm.get());
> 
> auto&

Fixed.

>>>>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:81
>>>>>      return builder.result();
>>>> 
>>>> The specification seems to say that we should return a *cached* JS Object. However, it seems you are returning a new object every time (or I am missing the caching?).
>>> 
>>> Thank you for pointing out. I forgot to implement it. Oops.
>> 
>> Given how this is currently implemented, I still do not think they should be public.
> 
> I have not looked in too much details but if you do not need ref counting, the factory could return a unique_ptr instead of a Ref, and you would not need to subclass RefCounted.

Fixed.

>>> Source/WebCore/crypto/CryptoKey.h:66
>>> +    KeyAlgorithm(const String& n)
>> 
>> RefCounted classes normally do not have a public constructor but a factory function that returns a Ref<> since we don't want those allocated on the stack.
> 
> This constructor should be protected and marked as explicit. This base class does not need a factory function since it is pure virtual, only subclasses do. The constructors in the subclasses should be protected or private, depending if the subclass is final or not.

Now, I will be using a unique_ptr, in this case, probably there is no need to use a factory function there?

>> Source/WebCore/crypto/CryptoKey.h:87
>> +    virtual Ref<KeyAlgorithm> algorithm() const = 0;
> 
> Given that this constructs a new object and transfers ownership, I think we need to choose a better name for this method, maybe buildAlgorithm() or createAlgorithm().

Changed it to buildAlgorithm().

>> Source/WebCore/crypto/CryptoKey.h:104
>> +    CryptoAlgorithmIdentifier m_algorithmIdentifier;
> 
> I think subclasses should use the algorithmIdentifier() getter instead of making this member protected.

Fixed. I am kind of thinking both of them are okay.

>> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86
>> +    Vector<uint8_t> publicExponent;
> 
> Not needed.

Fixed.

>> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:87
>> +    return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, WTFMove(publicExponent)));
> 
> return adoptRef(*new RsaKeyAlgorithm(emptyString(), 0, {}));

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:37
>> +class AesKeyAlgorithm : public KeyAlgorithm {
> 
> Should be final

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:39
>> +    size_t length;
> 
> Why is this public?

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:41
>> +    AesKeyAlgorithm(const String& n, size_t l)
> 
> Bad parameter names. Should be private and there should be a public factory function instead.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:46
>> +    virtual ~AesKeyAlgorithm()
> 
> I don't need this destructor it seems.

Removed.

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:50
>> +    KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::AES; }
> 
> override -> final

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:72
>> +    Ref<KeyAlgorithm> algorithm() const override;
> 
> override -> final

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:36
>> +class HmacKeyAlgorithm : public KeyAlgorithm {
> 
> Should be final

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38
>> +    KeyHashAlgorithm hash;
> 
> Why are those public?

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:41
>> +    HmacKeyAlgorithm(const String& n, KeyHashAlgorithm&& h, size_t l)
> 
> Bad parameter names. Should be private.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47
>> +    virtual ~HmacKeyAlgorithm()
> 
> I don't need this destructor it seems.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:51
>> +    KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HMAC; }
> 
> override -> final

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:74
>> +    Ref<KeyAlgorithm> algorithm() const override;
> 
> override -> final

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52
>> +    size_t modulusLength;
> 
> Why are those public?

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:55
>> +    RsaKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p)
> 
> Bad parameter names. Should be protected.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:61
>> +    virtual ~RsaKeyAlgorithm()
> 
> I don't need this destructor it seems.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:68
>> +class RsaHashedKeyAlgorithm : public RsaKeyAlgorithm {
> 
> Should be final.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:70
>> +    KeyHashAlgorithm hash;
> 
> Why is this public?

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:72
>> +    RsaHashedKeyAlgorithm(const String& n, size_t m, Vector<uint8_t>&& p, KeyHashAlgorithm&& h)
> 
> Those parameter names are really not great. We try to avoid abbreviations in WebKit.
> Should be private.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:77
>> +    virtual ~RsaHashedKeyAlgorithm()
> 
> I don't need this destructor it seems.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:82
>> +    KeyAlgorithmClass keyAlgorithmClass() const override { return KeyAlgorithmClass::HRSA; }
> 
> override -> final

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:109
>> +    Ref<KeyAlgorithm> algorithm() const override;
> 
> override -> final

Fixed.

[Jiewen Tan]

Created attachment 288747 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 17 files


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

[Chris Dumez]

Comment on attachment 288747 [details]
Patch

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

Looks like the patch does not build on ews.

> Source/WebCore/crypto/CryptoKey.idl:50
> +    [CachedAttribute, CustomGetter] readonly attribute object algorithm;

CachedAttribute does not do anything if you have a CustomGetter. You would have to do the caching yourself in the custom bindings.

An easy way to write a test for this is:
shouldBeTrue("key.algorithm === key.algorithm");
key.algorithm.foo = 1;
gc();
shouldBe("key.algorithm.foo", "1");

[Chris Dumez]

Comment on attachment 288747 [details]
Patch

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

> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:33
> +#include "CryptoKeyRSA.h"

Based on the build error, I think you'll need to include:
#include <heap/HeapInlines.h>

> Source/WebCore/crypto/CryptoKey.h:60
> +    KeyAlgorithm(const String& name)

This one should be explicit.

> Source/WebCore/crypto/CryptoKey.h:70
> +    String name() const { return m_name; }

should return a const String&.

> Source/WebCore/crypto/CryptoKey.h:74
> +

nit: extra line here.

>> Source/WebCore/crypto/CryptoKey.idl:50
>> +    [CachedAttribute, CustomGetter] readonly attribute object algorithm;
> 
> CachedAttribute does not do anything if you have a CustomGetter. You would have to do the caching yourself in the custom bindings.
> 
> An easy way to write a test for this is:
> shouldBeTrue("key.algorithm === key.algorithm");
> key.algorithm.foo = 1;
> gc();
> shouldBe("key.algorithm.foo", "1");

Never mind, it does have the impact of generating the m_algorithm member and it will be fine as long as you do the caching in the custom bindings (which I think you do). We should still add the layout test to confirm this works (and keeps working in the future).

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38
> +    explicit HmacKeyAlgorithm(const String& name, const String& hash, size_t length)

explicit is not useful here.

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47
> +    String hash() const { return m_hash; }

should return a const String&.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52
> +    explicit RsaKeyAlgorithm(const String& name, size_t modulusLength, Vector<uint8_t>&& publicExponent)

explicit is not really useful here since there is more than 1 mandatory parameter.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:62
> +    Vector<uint8_t> publicExponent() const { return m_publicExponent; }

nit: Should return a const Vector<uint8_t>&.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:79
> +    String hash() const { return m_hash; }

nit: should return a const String&.

[Chris Dumez]

Comment on attachment 288747 [details]
Patch

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

> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:43
> +    if (m_algorithm)

Yeah, your caching code in this function looks good. [CachedAttribute] should work.

[Chris Dumez]

Comment on attachment 288747 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by Brent Fulgham.

Oh, and please add my name since I reviewed too :)
Reviewed by Brent Fulgham and Chris Dumez.

[Jiewen Tan]

Comment on attachment 288747 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +        Reviewed by Brent Fulgham.
> 
> Oh, and please add my name since I reviewed too :)
> Reviewed by Brent Fulgham and Chris Dumez.

Since you haven't r+, I didn't add you. Fixed now. :)

>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:33
>> +#include "CryptoKeyRSA.h"
> 
> Based on the build error, I think you'll need to include:
> #include <heap/HeapInlines.h>

Fixed.

>> Source/WebCore/bindings/js/JSCryptoKeyCustom.cpp:43
>> +    if (m_algorithm)
> 
> Yeah, your caching code in this function looks good. [CachedAttribute] should work.

:)

>> Source/WebCore/crypto/CryptoKey.h:60
>> +    KeyAlgorithm(const String& name)
> 
> This one should be explicit.

Fixed.

>> Source/WebCore/crypto/CryptoKey.h:70
>> +    String name() const { return m_name; }
> 
> should return a const String&.

Fixed.

>> Source/WebCore/crypto/CryptoKey.h:74
>> +
> 
> nit: extra line here.

Fixed.

>>> Source/WebCore/crypto/CryptoKey.idl:50
>>> +    [CachedAttribute, CustomGetter] readonly attribute object algorithm;
>> 
>> CachedAttribute does not do anything if you have a CustomGetter. You would have to do the caching yourself in the custom bindings.
>> 
>> An easy way to write a test for this is:
>> shouldBeTrue("key.algorithm === key.algorithm");
>> key.algorithm.foo = 1;
>> gc();
>> shouldBe("key.algorithm.foo", "1");
> 
> Never mind, it does have the impact of generating the m_algorithm member and it will be fine as long as you do the caching in the custom bindings (which I think you do). We should still add the layout test to confirm this works (and keeps working in the future).

Testcases are added.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:38
>> +    explicit HmacKeyAlgorithm(const String& name, const String& hash, size_t length)
> 
> explicit is not useful here.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:47
>> +    String hash() const { return m_hash; }
> 
> should return a const String&.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:52
>> +    explicit RsaKeyAlgorithm(const String& name, size_t modulusLength, Vector<uint8_t>&& publicExponent)
> 
> explicit is not really useful here since there is more than 1 mandatory parameter.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:62
>> +    Vector<uint8_t> publicExponent() const { return m_publicExponent; }
> 
> nit: Should return a const Vector<uint8_t>&.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.h:79
>> +    String hash() const { return m_hash; }
> 
> nit: should return a const String&.

Fixed.

[Jiewen Tan]

Created attachment 288775 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 21 files


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

[Jiewen Tan]

Created attachment 288779 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 21 files


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

[Chris Dumez]

Looks like the patch does not build on GTK/EFL.

[Chris Dumez]

Comment on attachment 288779 [details]
Patch

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

r=me with comments.

> Source/WebCore/crypto/CryptoKey.h:60
> +    explicit KeyAlgorithm(const String& name)

nit: This should be protected, not public.

> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86
> +    return std::make_unique<RsaKeyAlgorithm>(emptyString(), 0, { });

Looks like EFL / GTK do not like the { } here :/

> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:11
> +description("Test that CryptoKey.algoithm preserves custom properties.");

typo: algoithm

> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:16
> +    key = result;

Is this async? If so, what guarantees that this completion handler has been called *before* startTest() is executed?

> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:36
> +    shouldBe('key.algorithm.foo', '"bar"');

shouldBeEqualToString('key.algorithm.foo', 'bar');

> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:11
> +description("Test that CryptoKey.algoithm preserves custom properties.");

Typo but anyway, it should be 'usages'

> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:16
> +    key = result;

Same comment as earlier about the potential race.

> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:21
> +    shouldBeTrue("key.algorithm === key.algorithm");

key.usages === key.usages

[Jiewen Tan]

Comment on attachment 288779 [details]
Patch

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

>> Source/WebCore/crypto/CryptoKey.h:60
>> +    explicit KeyAlgorithm(const String& name)
> 
> nit: This should be protected, not public.

Fixed.

>> Source/WebCore/crypto/gnutls/CryptoKeyRSAGnuTLS.cpp:86
>> +    return std::make_unique<RsaKeyAlgorithm>(emptyString(), 0, { });
> 
> Looks like EFL / GTK do not like the { } here :/

Created a Vector instance instead.

>> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:11
>> +description("Test that CryptoKey.algoithm preserves custom properties.");
> 
> typo: algoithm

Fixed.

>> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:16
>> +    key = result;
> 
> Is this async? If so, what guarantees that this completion handler has been called *before* startTest() is executed?

Fixed.

>> LayoutTests/crypto/subtle/crypto-key-algorithm-gc.html:36
>> +    shouldBe('key.algorithm.foo', '"bar"');
> 
> shouldBeEqualToString('key.algorithm.foo', 'bar');

Fixed.

>> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:11
>> +description("Test that CryptoKey.algoithm preserves custom properties.");
> 
> Typo but anyway, it should be 'usages'

Fixed.

>> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:16
>> +    key = result;
> 
> Same comment as earlier about the potential race.

Fixed.

>> LayoutTests/crypto/subtle/crypto-key-usages-gc.html:21
>> +    shouldBeTrue("key.algorithm === key.algorithm");
> 
> key.usages === key.usages

Fixed.

[Jiewen Tan]

Created attachment 288841 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/crypto/CryptoKey.h:47:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:53:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/crypto/CryptoKey.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 21 files


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

[Jiewen Tan]

Committed r205941: <http://trac.webkit.org/changeset/205941>