Refactor to share more code between OpenSSL and NSS implementations.

[webcrypto] Refactor to share more code between OpenSSL and NSS implementations.

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

[Ryan Sleevi, 2014-02-07 01:19:21]

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.cc (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.cc:22: CryptoData::CryptoData(const
blink::WebArrayBuffer& buffer)
new lines between each ctor

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.h (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:23: // similar to StringPiece, but for
crypto byte data.
base::StringPiece

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:37: CryptoData(const
blink::WebArrayBuffer& buffer);
style guide says: boo

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic...

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:47:
DISALLOW_COPY_AND_ASSIGN(CryptoData);
This comment doesn't make any sense. Just because the data isn't owned doesn't
mean it can't be copied.

StringPiece can be copied. It can also slice data.

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:23: class PlatformSymKey;
Don't use whitespace on lines 20/22

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:33: class CONTENT_EXPORT
PlatformCrypto {
Use a namespace.

There's no reason to make this a class - there are no members, this is
essentially a class full of statics, and just adds more indirection for the
compiler to fight through.

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/webcrypto_util.h (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_util.h:126: static Status
ErrorKeyAlgorithmMismatch();
Why are you introducing this in this change?

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_util.h:266: bool
IsAlgorithmAsymmetric(const blink::WebCryptoAlgorithm& algorithm);
Ditto

[eroman, 2014-02-07 21:15:56]

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.cc (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.cc:22: CryptoData::CryptoData(const
blink::WebArrayBuffer& buffer)
On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> new lines between each ctor

Done (i think git cl format put it that way).

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.h (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:23: // similar to StringPiece, but for
crypto byte data.
On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> base::StringPiece

Done.

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:37: CryptoData(const
blink::WebArrayBuffer& buffer);
On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> style guide says: boo
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic...
> 

I am aware of the style guide, however I was modelling after base::StringPiece
which uses implicit constructors. I will switch it to explicit and upload as
separate patchset, since it has a large impact on the unittests (which largely
work with std::vectors).

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:47:
DISALLOW_COPY_AND_ASSIGN(CryptoData);
On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> This comment doesn't make any sense. Just because the data isn't owned doesn't
> mean it can't be copied.
> 
> StringPiece can be copied. It can also slice data.

Copies are dangerous, since they encourage developpers to pass around copies,
perhaps not realizing that the underlying memory is now owned.

I'll remove the comment, but I stand by it as being a meaningful restriction.

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:33: class CONTENT_EXPORT
PlatformCrypto {
On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> Use a namespace.
> 
> There's no reason to make this a class - there are no members, this is
> essentially a class full of statics, and just adds more indirection for the
> compiler to fight through.

Done.

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/webcrypto_util.h (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_util.h:126: static Status
ErrorKeyAlgorithmMismatch();
On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> Why are you introducing this in this change?

Will change to ErrorUnexpected for now()

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_util.h:266: bool
IsAlgorithmAsymmetric(const blink::WebCryptoAlgorithm& algorithm);
On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> Ditto

Was moving it out of webcrypto_impl.cc.
Will leave it to a followup cl then.

[Ryan Sleevi, 2014-02-07 21:18:48]

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.h (right):

https://codereview.chromium.org/155623005/diff/280001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:47:
DISALLOW_COPY_AND_ASSIGN(CryptoData);
On 2014/02/07 21:15:57, eroman wrote:
> On 2014/02/07 01:19:21, Ryan Sleevi wrote:
> > This comment doesn't make any sense. Just because the data isn't owned
doesn't
> > mean it can't be copied.
> > 
> > StringPiece can be copied. It can also slice data.
> 
> Copies are dangerous, since they encourage developpers to pass around copies,
> perhaps not realizing that the underlying memory is now owned.
> 
> I'll remove the comment, but I stand by it as being a meaningful restriction.

I still don't believe this does anything to reduce the danger. It's still
incredibly easy to misuse this class

CryptoData foo(SomethingReturnsAString());
foo.bytes();  // INVALID

If you're going to have a class not take ownership (which is fine, I think),
then I don't think it makes sense to suggest that some things are more dangerous
than others, given how much you're allowing for potential misuse.

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.h (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:27: class CONTENT_EXPORT CryptoData {
CryptoDataReference?

[Ryan Sleevi, 2014-02-07 21:26:39]

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/jwk.cc (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/jwk.cc:558: key);
alignment

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/jwk.cc:571: 
unnecessary newline?

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:29: void Init();
Why does this need to be the caller's responsibility?

Can't you just do all of this internal to your methods, in the same way that the
Ensure* Init methods work?

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:120: //  * |algorithm| is a Sha
function.
s/Sha/SHA/ in the comment

That said, I don't understand why you've broken out "DigestSha" as a special
case, rather than Digest? What about non-SHA digest algorithms?

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.h:60: GenerateSecretKey(const
blink::WebCryptoAlgorithm& algorithm,
git-cl-format did this?

The Chromium Style Arbitrars (... pkasting) in the past said it should be

CONTENT_EXPORT Status
GenerateSecretKey(...)

I'm fine if git-cl-format did this.

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/webcrypto_impl.h (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_impl.h:11: #include
"third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
Why is this needed?

[eroman, 2014-02-08 02:23:50]

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/jwk.cc (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/jwk.cc:558: key);
On 2014/02/07 21:26:40, Ryan Sleevi wrote:
> alignment

Done.

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/jwk.cc:571: 
On 2014/02/07 21:26:40, Ryan Sleevi wrote:
> unnecessary newline?

Ick git cl format added these.

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:29: void Init();
On 2014/02/07 21:26:40, Ryan Sleevi wrote:
> Why does this need to be the caller's responsibility?
> 
> Can't you just do all of this internal to your methods, in the same way that
the
> Ensure* Init methods work?

There are a ton of places this would need to be added. I don't have confidence
that would be maintained correct. Easier (and more efficient) to just do it once
during setup.

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:120: //  * |algorithm| is a Sha
function.
On 2014/02/07 21:26:40, Ryan Sleevi wrote:
> s/Sha/SHA/ in the comment
> 
> That said, I don't understand why you've broken out "DigestSha" as a special
> case, rather than Digest? What about non-SHA digest algorithms?

The purpose for this layer is to do the fanout which can then be implemented by
the nss/openssl versions. If non-sha digests are supported in the future they
can either have their own method (if they require other parameters), or can
rework this one to include them too.

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.h:60: GenerateSecretKey(const
blink::WebCryptoAlgorithm& algorithm,
On 2014/02/07 21:26:40, Ryan Sleevi wrote:
> git-cl-format did this?
> 
> The Chromium Style Arbitrars (... pkasting) in the past said it should be
> 
> CONTENT_EXPORT Status
> GenerateSecretKey(...)
> 
> I'm fine if git-cl-format did this.

Correct, this is git-cl format.

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
File content/renderer/webcrypto/webcrypto_impl.h (right):

https://codereview.chromium.org/155623005/diff/570001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_impl.h:11: #include
"third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
On 2014/02/07 21:26:40, Ryan Sleevi wrote:
> Why is this needed?

Done (leftover from code shuffling).

[eroman, 2014-02-12 23:35:59]

ping

[Ryan Sleevi, 2014-02-13 04:24:24]

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.h (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:34: // Allow implicit conversions for
convenience.
comment is no longer accurate

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:43: const unsigned char* bytes_;
const unsigned char* const?

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:21: namespace platform {
Include documentation about what belongs in this namespace and how it fits into
the overall design.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:38: // Guarantees:
s/Guarantees/Preconditions/

Guarantees make me think you're stating post-conditions, but these are the set
of requirements.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:47: Status ImportRsaPublicKey(const
CryptoData& modulus_data,
No preconditions here?

Also, shouldn't |algorithm| be first in all of these? The asymmetry bothers me a
bit. Suggested order for all calls:

algorithm, extractable, usage_mask (each of which are required for all keys),
then any key specific data, then |key| for output.

For example, compare this method with GenerateSecretKey - inconsistent.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:151: Status ImportKeySpki(const
CryptoData& key_data,
No pre-conditions here?

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:157: Status ImportKeyPkcs8(const
CryptoData& key_data,
for these, suggested is algorithm_or_null, extractable, usage_mask, key_data,
key

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:173: }  // namespace content
new lines between each of these

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto_nss.cc:115: namespace platform {
Compare with your OpenSSL implementation, where you (correctly, AIUI) included
line breaks.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto_nss.cc:122: };
Why do you declare this duplicate interface in both NSS and OpenSSL?

If you do want to keep it in the impl-files, it'd be good to use NSSKey and
OpenSSLKey (or something similarly distinct)

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto_nss.cc:304: const
blink::WebCryptoAesGcmParams* params,
Do you mutate params here? If not, why not pass as const-ref? Compare to your
RsaSsa which passes hash as const-ref

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.h:24: CONTENT_EXPORT void Init();
FTR, I still really dislike this. This is inconsistent with all of our other
patterns of using crypto in Chromium (where it's the impl. responsibility to do
the Ensure* Init)

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.h:31: // one of the "Platform*()"
methods for the platform specific implementation.
You should document what PlatformCrypto is within PlatformCrypto. Then document
how this implementation overlays on it.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto_unittest.cc:346: // Wrappers to pass
vector<> in place of CryptoData.
This is not clear as to why this exists.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto_unittest.cc:354: return
ImportKey(format,
This is... a very odd wrap. That Internal() calls the non-Internal form seems
counter-intuitive.

[eroman, 2014-02-13 23:05:38]

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/crypto_data.h (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:34: // Allow implicit conversions for
convenience.
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> comment is no longer accurate

Done. Thanks for spotting that.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/crypto_data.h:43: const unsigned char* bytes_;
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> const unsigned char* const?

Done.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:21: namespace platform {
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> Include documentation about what belongs in this namespace and how it fits
into
> the overall design.

Done.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:38: // Guarantees:
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> s/Guarantees/Preconditions/
> 
> Guarantees make me think you're stating post-conditions, but these are the set
> of requirements.

Done.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:47: Status ImportRsaPublicKey(const
CryptoData& modulus_data,
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> No preconditions here?
> 
> Also, shouldn't |algorithm| be first in all of these? The asymmetry bothers me
a
> bit. Suggested order for all calls:
> 
> algorithm, extractable, usage_mask (each of which are required for all keys),
> then any key specific data, then |key| for output.

Done.

> 
> For example, compare this method with GenerateSecretKey - inconsistent.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:173: }  // namespace content
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> new lines between each of these

Done.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto_nss.cc:115: namespace platform {
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> Compare with your OpenSSL implementation, where you (correctly, AIUI) included
> line breaks.

Done.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto_nss.cc:122: };
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> Why do you declare this duplicate interface in both NSS and OpenSSL?
> 
> If you do want to keep it in the impl-files, it'd be good to use NSSKey and
> OpenSSLKey (or something similarly distinct)

Done.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto_nss.cc:304: const
blink::WebCryptoAesGcmParams* params,
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> Do you mutate params here? If not, why not pass as const-ref? Compare to your
> RsaSsa which passes hash as const-ref

Removed all together: Now passes in the parameters with defaults resolved.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.h:24: CONTENT_EXPORT void Init();
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> FTR, I still really dislike this. This is inconsistent with all of our other
> patterns of using crypto in Chromium (where it's the impl. responsibility to
do
> the Ensure* Init)

I'll explore this as a followup.

My concern is it is tricky to test correctness when lazy initialization is done
within each of the platform:: functions. An earlier test may do the
initialization and hence the unittests of new functionality passes.

However then in the field it calls things in a different order, and ends up
crashing inside of NSS because things weren't initialized. Being able to
guarantee that initialization is done at a consistent time helps with future
proofing and also means less calls to ensure initialized...

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.h:31: // one of the "Platform*()"
methods for the platform specific implementation.
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> You should document what PlatformCrypto is within PlatformCrypto. Then
document
> how this implementation overlays on it.

I revamped this documentation by adding an ASCII art diagram.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto_unittest.cc:346: // Wrappers to pass
vector<> in place of CryptoData.
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> This is not clear as to why this exists.

Because otherwise this refactor has a very large impact on the unittests. I
would rather do this in a second pass because otherwise the diffs are hard to
follow.

https://codereview.chromium.org/155623005/diff/750001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto_unittest.cc:354: return
ImportKey(format,
On 2014/02/13 04:24:24, Ryan Sleevi wrote:
> This is... a very odd wrap. That Internal() calls the non-Internal form seems
> counter-intuitive.

I changed the wrapping.

The naming containing "Internal" is a left over from the earlier code. I don't
want to change that in this changelist because these pre-existing helpers have
tons of callsites. I will clean it up in a followup change however. I originally
did it as part of this changelist but the diffs were too extensive that it was
really hard to tell the essence of what I had changed.

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:16: enum EncryptOrDecrypt {
This is new to latest patchset..

[Ryan Sleevi, 2014-02-14 00:24:18]

LGTM, mod one nit about the ordering of parameters in platform_crypto

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:50: 
unnecessary new-line

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:57: const CryptoData& data,
Ordering: Compare with lines 65-66, where |iv| and |data| are passed in a
different order.

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:120: //    bits, 256 bits.
AES-GCM too, oui?

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.cc:100: tag_length_bits, buffer);
is this clang-cl formatted?

I would expect it to wrap at |mode| onto the new line, four space indented.

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.cc:112: // RSAES encryption does not
support empty input
Should you mention this in the pre-conditions for EncryptRsaEsPkcs1v1_5 ?

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.cc:167: // Do not allow verification of
truncated signatures.
s/signatures/MACs/

[eroman, 2014-02-14 05:52:41]

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
File content/renderer/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:50: 
On 2014/02/14 00:24:18, Ryan Sleevi wrote:
> unnecessary new-line

Done.

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:57: const CryptoData& data,
On 2014/02/14 00:24:18, Ryan Sleevi wrote:
> Ordering: Compare with lines 65-66, where |iv| and |data| are passed in a
> different order.

Done.

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/platform_crypto.h:120: //    bits, 256 bits.
On 2014/02/14 00:24:18, Ryan Sleevi wrote:
> AES-GCM too, oui?

Done for all of the AES-*

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
File content/renderer/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.cc:100: tag_length_bits, buffer);
On 2014/02/14 00:24:18, Ryan Sleevi wrote:
> is this clang-cl formatted?
> 
> I would expect it to wrap at |mode| onto the new line, four space indented.

My bad

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.cc:112: // RSAES encryption does not
support empty input
On 2014/02/14 00:24:18, Ryan Sleevi wrote:
> Should you mention this in the pre-conditions for EncryptRsaEsPkcs1v1_5 ?

Yes. Done.

https://codereview.chromium.org/155623005/diff/950001/content/renderer/webcry...
content/renderer/webcrypto/shared_crypto.cc:167: // Do not allow verification of
truncated signatures.
On 2014/02/14 00:24:18, Ryan Sleevi wrote:
> s/signatures/MACs/

Done.

[eroman, 2014-02-14 05:54:26]

+avi for content/*.gypi approval

[Avi (use Gerrit), 2014-02-18 23:52:58]

gpyi changes lgtm

[eroman, 2014-02-18 23:53:28]

The CQ bit was checked by eroman@chromium.org

[eroman, 2014-02-18 23:53:38]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-02-18 23:54:42]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-18 23:55:53]

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

[eroman, 2014-02-19 00:32:04]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-19 00:33:54]

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

[commit-bot: I haz the power, 2014-02-19 06:11:07]

Change committed as 251961