Bug 128748

Summary: WebCrypto algorithms should be exposed via KeyAlgorithm interface
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, esprehn+autocc, jiewen_tan, kondapallykalyan, syoichi, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=160877
Bug Depends on:    
Bug Blocks: 160877    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexey Proskuryakov 2014-02-13 10:20:37 PST
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.
Comment 2 Radar WebKit Bug Importer 2016-07-14 15:20:44 PDT
<rdar://problem/27359438>
Comment 3 Jiewen Tan 2016-09-06 15:46:05 PDT
Created attachment 288055 [details]
Patch
Comment 4 WebKit Commit Bot 2016-09-06 15:47:45 PDT
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.
Comment 5 Jiewen Tan 2016-09-08 10:29:10 PDT
Created attachment 288274 [details]
Patch
Comment 6 WebKit Commit Bot 2016-09-08 10:31:05 PDT
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.
Comment 7 Brent Fulgham 2016-09-08 18:36:37 PDT
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 ...
Comment 8 Jiewen Tan 2016-09-12 14:54:13 PDT
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.
Comment 9 Jiewen Tan 2016-09-12 14:59:50 PDT
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.
Comment 10 Jiewen Tan 2016-09-12 15:03:13 PDT
Created attachment 288613 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2016-09-12 15:04:58 PDT
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.
Comment 12 Jiewen Tan 2016-09-12 19:09:28 PDT
Created attachment 288655 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2016-09-12 19:12:24 PDT
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.
Comment 14 Chris Dumez 2016-09-12 19:13:50 PDT
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.
Comment 15 Jiewen Tan 2016-09-12 19:14:47 PDT
Created attachment 288657 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2016-09-12 19:18:11 PDT
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.
Comment 17 Chris Dumez 2016-09-12 19:26:24 PDT
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?
Comment 18 Chris Dumez 2016-09-12 19:30:02 PDT
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?).
Comment 19 Chris Dumez 2016-09-12 19:35:15 PDT
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.
Comment 20 Chris Dumez 2016-09-12 19:50:46 PDT
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
Comment 21 Jiewen Tan 2016-09-12 22:33:38 PDT
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?
Comment 22 Chris Dumez 2016-09-13 07:44:59 PDT
(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.
Comment 23 Chris Dumez 2016-09-13 07:47:31 PDT
(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.
Comment 24 Jiewen Tan 2016-09-13 16:25:45 PDT
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.
Comment 25 Jiewen Tan 2016-09-13 16:56:11 PDT
Created attachment 288747 [details]
Patch
Comment 26 WebKit Commit Bot 2016-09-13 16:57:51 PDT
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.
Comment 27 Chris Dumez 2016-09-13 18:28:04 PDT
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");
Comment 28 Chris Dumez 2016-09-13 18:44:08 PDT
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&.
Comment 29 Chris Dumez 2016-09-13 18:46:56 PDT
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.
Comment 30 Chris Dumez 2016-09-13 18:51:10 PDT
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.
Comment 31 Jiewen Tan 2016-09-13 19:22:39 PDT
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.
Comment 32 Jiewen Tan 2016-09-13 23:37:32 PDT
Created attachment 288775 [details]
Patch
Comment 33 WebKit Commit Bot 2016-09-13 23:39:16 PDT
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.
Comment 34 Jiewen Tan 2016-09-14 00:36:31 PDT
Created attachment 288779 [details]
Patch
Comment 35 WebKit Commit Bot 2016-09-14 00:37:30 PDT
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.
Comment 36 Chris Dumez 2016-09-14 09:12:45 PDT
Looks like the patch does not build on GTK/EFL.
Comment 37 Chris Dumez 2016-09-14 09:22:56 PDT
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
Comment 38 Jiewen Tan 2016-09-14 11:48:48 PDT
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.
Comment 39 Jiewen Tan 2016-09-14 11:50:49 PDT
Created attachment 288841 [details]
Patch
Comment 40 WebKit Commit Bot 2016-09-14 11:52:51 PDT
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.
Comment 41 Jiewen Tan 2016-09-14 17:21:40 PDT
Committed r205941: <http://trac.webkit.org/changeset/205941>