http://www.w3.org/TR/WebCryptoAPI/#dfn-HmacKeyGenParams "The length (in bits) of the key to generate. If unspecified, the recommended length will be used, which is the size of the associated hash function's block size."

Is there a better place or way to do this?

Comment on attachment 8434847 [details] [diff] [review] 0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch The IDL interaction bits look good to me. Richard, can you review the switch bit and tests?

Comment on attachment 8434847 [details] [diff] [review] 0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch Review of attachment 8434847 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for catching this. There might be a few other default cases that are missing, so your review is very helpful. This is the right place to address the problem. ::: dom/crypto/WebCryptoTask.cpp @@ +1032,5 @@ > > + if (params.mLength.WasPassed()) { > + mLength = params.mLength.Value(); > + } else { > + KeyAlgorithm hashAlg(global, hashName); This seems duplicative of the HmacKeyAlgorithm construction below. @@ +1039,5 @@ > + case CKM_SHA224: mLength = 224; break; > + case CKM_SHA256: mLength = 256; break; > + case CKM_SHA384: mLength = 384; break; > + case CKM_SHA512: mLength = 512; break; > + default: mLength = 0; break; I'm concerned about this default case. If Mechanism() returns something unknown, we're in more serious trouble. Could you check to see where we fail for an unknown hash algorithm?

(In reply to Richard Barnes [:rbarnes] from comment #3) > ::: dom/crypto/WebCryptoTask.cpp > @@ +1032,5 @@ > > > > + if (params.mLength.WasPassed()) { > > + mLength = params.mLength.Value(); > > + } else { > > + KeyAlgorithm hashAlg(global, hashName); > > This seems duplicative of the HmacKeyAlgorithm construction below. I'm only doing this to let it parse the string into a Mechanism. HmacKeyAlgorithm wants the length given to the constructor and I can't change it afterwards. I could as well just do string comparisons like the KeyAlgorithm constructor does?

(In reply to Tim Taubert [:ttaubert] from comment #4) > (In reply to Richard Barnes [:rbarnes] from comment #3) > > ::: dom/crypto/WebCryptoTask.cpp > > @@ +1032,5 @@ > > > > > > + if (params.mLength.WasPassed()) { > > > + mLength = params.mLength.Value(); > > > + } else { > > > + KeyAlgorithm hashAlg(global, hashName); > > > > This seems duplicative of the HmacKeyAlgorithm construction below. > > I'm only doing this to let it parse the string into a Mechanism. > HmacKeyAlgorithm wants the length given to the constructor and I can't > change it afterwards. I could as well just do string comparisons like the > KeyAlgorithm constructor does? Good point. Let's just leave it.

You're absolutely right, I missed that the spec says we should fail with a DataError when length is zero. Added two more test cases.

Comment on attachment 8435229 [details] [diff] [review] 0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch, v2 Review of attachment 8435229 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!

Comment on attachment 8435229 [details] [diff] [review] 0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch, v2 Review of attachment 8435229 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!