[webcrypto] Update to use the KeyAlgorithm.

[webcrypto] Update to use the KeyAlgorithm.

The WEBCRYPTO_HAS_KEY_ALGORITHM ifdef will be introduced by https://codereview.chromium.org/179353002/ on the blink side. 

This patch needs to land first in order to make the API changes on blink side without breaking the build. Sigh for non-atomic blink/chromium commits.

BUG=245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253463

[Ryan Sleevi, 2014-02-25 22:26:26]

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/j...
File content/renderer/webcrypto/jwk.cc (right):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/j...
content/renderer/webcrypto/jwk.cc:18: #include
"third_party/WebKit/public/platform/WebCrypto.h"  // TODO(eroman):
What's the TODO?

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/p...
File content/renderer/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/p...
content/renderer/webcrypto/platform_crypto_nss.cc:411: if (!key || key->keyType
!= rsaKey)
rsaPssKey
rsaOaepKey

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/p...
content/renderer/webcrypto/platform_crypto_nss.cc:434: algorithm.paramsType() ==
blink::WebCryptoAlgorithmParamsTypeNone) {
Does it make sense to make these a set of switch statements?

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
File content/renderer/webcrypto/shared_crypto_unittest.cc (left):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:77:
base::HexEncode(actual.data(), actual.byteLength()));
Can you avoid making (good-but-unrelated) changes like this in the future?

I would have been happy with ExpectCryptoDataMatches and cleaning up in a
separate CL. It just seems like you did a lot of CryptoData cleanup in this CL
that would have been better if separated from the KeyAlgorithm this CL is
supposed to be about.

Minimally, update the CL description to call this out as well.

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
File content/renderer/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:898:
EXPECT_EQ(blink::WebCryptoAlgorithmIdSha1,
Why are you testing this, rather than algorithm like you do on line 617

Fewer magic constants (eg: line 891) are a good thing?

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:930:
key.algorithm().hmacParams()->hash().id());
Ditto

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:1288:
EXPECT_EQ(blink::WebCryptoAlgorithmIdSha256,
ditto

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:1564:
EXPECT_EQ(blink::WebCryptoAlgorithmIdSha256,
why not |algorithm| here as well?

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:1591:
EXPECT_EQ(blink::WebCryptoAlgorithmIdSha1,
ditto

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/w...
File content/renderer/webcrypto/webcrypto_util.h (right):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_util.h:13: #include
"third_party/WebKit/public/platform/WebCrypto.h" // TODO(eroman):
TODO?

[eroman, 2014-02-25 23:26:47]

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/j...
File content/renderer/webcrypto/jwk.cc (right):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/j...
content/renderer/webcrypto/jwk.cc:18: #include
"third_party/WebKit/public/platform/WebCrypto.h"  // TODO(eroman):
On 2014/02/25 22:26:26, Ryan Sleevi wrote:
> What's the TODO?

Done.

The todo is for me to delete that line. Added a comment to that effect (that
header is where the #define is pulled in from blink).

Apparently clang-format doesn't care about the line being longer than 80 chars

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/p...
File content/renderer/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/p...
content/renderer/webcrypto/platform_crypto_nss.cc:411: if (!key || key->keyType
!= rsaKey)
On 2014/02/25 22:26:26, Ryan Sleevi wrote:
> rsaPssKey
> rsaOaepKey

Added a TODO.

I don't want to add that handling until there are corresponding tests that would
exercise it (the current ones do not).

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/p...
content/renderer/webcrypto/platform_crypto_nss.cc:434: algorithm.paramsType() ==
blink::WebCryptoAlgorithmParamsTypeNone) {
On 2014/02/25 22:26:26, Ryan Sleevi wrote:
> Does it make sense to make these a set of switch statements?

Yes, that makes more sense.
Done.
Thanks

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
File content/renderer/webcrypto/shared_crypto_unittest.cc (left):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:77:
base::HexEncode(actual.data(), actual.byteLength()));
On 2014/02/25 22:26:26, Ryan Sleevi wrote:
> Can you avoid making (good-but-unrelated) changes like this in the future?
> 
> I would have been happy with ExpectCryptoDataMatches and cleaning up in a
> separate CL. It just seems like you did a lot of CryptoData cleanup in this CL
> that would have been better if separated from the KeyAlgorithm this CL is
> supposed to be about.
> 
> Minimally, update the CL description to call this out as well.

Removed the CryptoData changes in unittest file.
Also removed an unnecessary CryptoData change in platform_crypto_nss.cc
(BigIntegerToLong).

There is one remaining CryptoData change to the signature of generatersakeypair.
This one is relevant to the changelist, since the parameter names have changed
on the blink side and need to be if-def

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
File content/renderer/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/178073007/diff/1/content/renderer/webcrypto/s...
content/renderer/webcrypto/shared_crypto_unittest.cc:898:
EXPECT_EQ(blink::WebCryptoAlgorithmIdSha1,
On 2014/02/25 22:26:26, Ryan Sleevi wrote:
> Why are you testing this, rather than algorithm like you do on line 617
> 
> Fewer magic constants (eg: line 891) are a good thing?

Unless you feel strongly about this will leave as is.

I prefer to use constant wherever possible in test expectations. Using a dynamic
value makes it possible to make two mistakes and end up with a test that still
passes, but not for the reason you were expecting.

Line 891 is different because the test expectations come from a json file (so
can't be expressed as a constant).

[eroman, 2014-02-26 00:01:55]

I also removed the unrelated change which make public keys extractable despite
the import parameter

[Ryan Sleevi, 2014-02-26 00:06:13]

lgtm

[eroman, 2014-02-26 00:08:19]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:36:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/178073007/40001

[Paweł Hajdan Jr., 2014-02-26 05:31:48]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 05:41:22]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 06:04:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/178073007/40001

[commit-bot: I haz the power, 2014-02-26 15:16:27]

Change committed as 253463