WebCrypto: Add framework for AlgorithmIdentifier normalization.

WebCrypto: Add framework for AlgorithmIdentifier normalization.

In this changelist, the "normalizeAlgorithm()" function corresponds with https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#algorithm-normalizing-rules

A threadsafe and copiable representation of the normalized algorithm has been added to the WebKit API as WebCryptoAlgorithm. This is how WebCore will inform the embedder of the cryptographic operation to perform and its parameters.

WebCryptoAlgorithm is also used by WebCore to pass around normalized "algorithm identifiers".

There is a lot of boiler plate to achieve both parsing of algorithm parameters from a dictionary, as well as reflecting back the normalized results to javascript.

The parsing is done manually as it needs to extract from a Dictionary. The reflection back to javascript is done using IDL and some glue to WebCryptoAlgorithm.

This changelist only adds a small number of algorithm parameters. (Enough to be minimally testable). The rest will follow in later changes.

BUG=245025
R=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153450

[eroman, 2013-07-02 06:06:52]

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
File public/platform/WebCryptoAlgorithm.h (right):

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:44: enum WebCryptoAlgorithmId {
Note: I only included a few algorithms and parameter types in this CL to keep
things simple (and minimally testable).

I will be adding the others incrementally as follow-ups once we have settled on
the approach.

[abarth-chromium, 2013-07-02 06:12:51]

Please add more information to the description.  For example, you can link to
the spec you're implementing and explain what parts you're implementing in this
CL.

[eroman, 2013-07-02 06:24:38]

Thanks Adam, apologies for the terseness!

I have updated the CL description with more information.

Tomorrow I will also move the design document that I shared with you privately
to my @chromium.org, and link to it on the bug tracker. This will serve as a
"living document" for the implementation work.

[abarth-chromium, 2013-07-02 06:46:35]

Thanks for updating the description.  This CL is actually looks to be in pretty
good shape.  I've left you a very large number of minor nit-pick comments.

Thanks!!!

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp:37: #include
<string.h>
Should this be "#include <string>" ?  I though the ".h" versions were
deprecated.

I don't see where this header is used.  Can we remove it?

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp:41: // Use a
reference counted to make copying easier.
I'd skip this comment.

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp:51:
WebCryptoAlgorithmId algorithmId;
Id -> ID ?  I'm not sure what's most consistent with existing style...

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebCryptoAlgorithmParams.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithmParams.cpp:40:
WebCryptoAlgorithmParams::Bytes::Bytes(const unsigned char* data, size_t size)
How is this different from WebVector?

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
File Source/modules/crypto/AesCbcParams.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
Source/modules/crypto/AesCbcParams.cpp:45: AesCbcParams::AesCbcParams(const
WebKit::WebCryptoAlgorithm& algorithm) : Algorithm(algorithm)
": Algorithm(algorithm)" should be on it's own line.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
File Source/modules/crypto/AesCbcParams.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
Source/modules/crypto/AesCbcParams.h:45: PassRefPtr<Uint8Array> iv();
This should just be Uint8Array*.  PassRefPtr is used to pass ownership.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
Source/modules/crypto/AesCbcParams.h:48: AesCbcParams(const
WebKit::WebCryptoAlgorithm&);
Please mark one-argument constructors "explicit".

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
File Source/modules/crypto/AesKeyGenParams.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
Source/modules/crypto/AesKeyGenParams.cpp:40: return
m_algorithm.getAesKeyGenParams()->length();
getAesKeyGenParams -> aesKeyGenParams

There's no need to use the verb "get".  It doesn't mean anything and can just be
omitted.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
File Source/modules/crypto/AesKeyGenParams.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
Source/modules/crypto/AesKeyGenParams.h:48: AesKeyGenParams(const
WebKit::WebCryptoAlgorithm&);
Please mark one-argument constructors explicit.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Algo...
File Source/modules/crypto/Algorithm.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Algo...
Source/modules/crypto/Algorithm.cpp:42: String Algorithm::name() const
This should return a const String&

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Cryp...
File Source/modules/crypto/CryptoOperation.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Cryp...
Source/modules/crypto/CryptoOperation.h:49: PassRefPtr<Algorithm> algorithm();
PassRefPtr<Algorithm>    ->    Algorithm*

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:55: enum { UnsupportedOp = -1 };
Rather than use an enum, you can just use a global constant holding the value
-1.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:130: // static
Please skip these sorts of comments.  They're ignored by the compiler and
therefore become out of date.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:133: // Singleton
Please omit comments that redundantly state what the code does.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:134: static AlgorithmRegistry
registry;
Please use DEFINE_STATIC_LOCAL.  That lets us avoid exit-time destructors.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:136: // Do a case-insensitive
lookup for algorithmName.
Please omit comments that redundantly state what the code does.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:145: // Initialize
AlgorithmRegistry by populating it with the static mappings.
Please skip comments that say what the code does.  We like comments that explain
why the code does what it does, but comments that say what the code does are
redundant can get out of date quickly.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:158:
WebKit::WebCryptoAlgorithmParams* parseAesCbcParams(const Dictionary& raw)
WebKit::WebCryptoAlgorithmParams*   --> 
PassOwnPtr<WebKit::WebCryptoAlgorithmParams>

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:167: return new
WebKit::WebCryptoAesCbcParams(static_cast<unsigned char*>(iv->baseAddress()),
iv->byteLength());
We try to avoid "naked new" calls.  Please wrap all calls to "new" either in
adoptPtr or adoptRef.  In this case, we should use adoptPtr.  At the API
boundary, we'll need to call leakPtr() to state that we're leaking this pointer
to the embedder.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:177: return new
WebKit::WebCryptoAesKeyGenParams(length);
Naked new

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:190: 
ASSERT_NOT_REACHED() ?

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:196: // TODO(eroman): Throw the
correct exception types!
"TODO(eroman)"  -->  "FIXME"

We use FIXME rather than TODO, and we skip the name of the author.  If someone
wants to know who authored the comment, they can look in the revision history.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:201: // No name was specified by
the AlgorithmIdentifier.
Please remove these sorts of redundant comments.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:207: // The spec defines this case
separately.
Please add a link to the part of the spec you're referring to.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:214: // No algorithm by that name
exists.
Please omit this comment as well.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:220: // The algorithm does not
support the requested operation.
Please omit this redundant comment.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:225: // Parse the
algorithm-specific parameters.
Please omit this redundant comment.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:227:
OwnPtr<WebKit::WebCryptoAlgorithmParams> params =
adoptPtr(parseAlgorithmParams(raw, paramsType));
I see, we're not actually leaking this pointer.  The proper place to call
adoptPtr is immediately after calling "new".  That way we can check that there
aren't any leaks by looking at each line individually instead of having to
understand the whole file at once.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:230: // Failed parsing the
parameters.
Please omit this redundant comment.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
File Source/modules/crypto/NormalizeAlgorithm.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.h:55: NumAlgorithmOperations,
NumAlgorithmOperations -> NumberOfAlgorithmOperations

Please use complete works in variable names.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.h:60: bool normalizeAlgorithm(const
Dictionary& raw, AlgorithmOperation, WebKit::WebCryptoAlgorithm&,
ExceptionCode&) WARN_UNUSED_RETURN;
raw -> You can omit this argument name.  The name doesn't add any information.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Subt...
File Source/modules/crypto/SubtleCrypto.idl (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.idl:32: // TODO(eroman): Work in progress:
http://crbug.com/245025.
Please remove this comment.  All code is a work in progress.  :)

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
File public/platform/WebCryptoAlgorithm.h (right):

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:43: // The possible algorithm names.
These aren't algorithm names.  They're algorithm IDs.  I'd probably just remove
this comment since it's not correct and redundant with the code.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:45: AesCbc,
The pattern we use for these API enums is to repeat the name of the enum in the
values:

WebCryptoAlgorithmIdAesCbc

I realize that it's a mouthful, but we do that to avoid naming conflicts.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:56: // The possible algorithm-specific
parameters.
Please omit this redundant comment.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:63: // Forward declaration for the
algorithm parameters types.
Please omit this comment that just says what the code does.  We know these are
forward declarations.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:73: //   * Copiable (cheaply)
By contrast, this is a very helpful comment!

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:81: WEBKIT_EXPORT ~WebCryptoAlgorithm();
We generally don't export destructors.  Instead, we make the destructors inline
and call a reset() method that's exported.  You can see the pattern in other API
classes.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:84: WEBKIT_EXPORT WebCryptoAlgorithm&
operator=(const WebCryptoAlgorithm&);
Similarly, we don't export constructors or operators.  Instead, we define these
sorts of functions inline and call an exported assign function.  There should be
many examples of this pattern in the API.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:86: // Returns a unique identifier for the
algorithm.
Unique in what sense?  They're just identifiers for the algorithms.  I don't
think there's anything particularly unique about them.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:95: WebCryptoAesCbcParams*
getAesCbcParams() const;
getAesCbcParams -> aesCbcParams

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:96: WebCryptoAesKeyGenParams*
getAesKeyGenParams() const;
getAesKeyGenParams  ->  aesKeyGenParams

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
File public/platform/WebCryptoAlgorithmParams.h (right):

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithmParams.h:42: //       Crypto spec. Parameters
have the same name (minus "WebCrypto" prefix).
Please add a link to the spec.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithmParams.h:55: // Helper class for a chunk of
owned immutable data.
We should be able to just use WebVector

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithmParams.h:85: // The initialization vector.
MUST be 16 bytes.
Can you enforce that with an ASSERT rather than a comment?  Comments can be
incorrect, but ASSERTs are verified by tests.

[eroman, 2013-07-02 07:00:27]

Excellent thankyou!

Nits are much appreciated, as I am still learning the way of the Blink.

[eroman, 2013-07-02 08:12:26]

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp:37: #include
<string.h>
On 2013/07/02 06:46:36, abarth wrote:
> Should this be "#include <string>" ?  I though the ".h" versions were
> deprecated.
> 
> I don't see where this header is used.  Can we remove it?

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp:41: // Use a
reference counted to make copying easier.
On 2013/07/02 06:46:36, abarth wrote:
> I'd skip this comment.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp:51:
WebCryptoAlgorithmId algorithmId;
On 2013/07/02 06:46:36, abarth wrote:
> Id -> ID ?  I'm not sure what's most consistent with existing style...

I will do some more research on this matter tomorrow. For now I have left it as
"Id".

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebCryptoAlgorithmParams.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithmParams.cpp:40:
WebCryptoAlgorithmParams::Bytes::Bytes(const unsigned char* data, size_t size)
On 2013/07/02 06:46:36, abarth wrote:
> How is this different from WebVector?

Perfect! Somehow I didn't see that class. Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
File Source/modules/crypto/AesCbcParams.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
Source/modules/crypto/AesCbcParams.cpp:45: AesCbcParams::AesCbcParams(const
WebKit::WebCryptoAlgorithm& algorithm) : Algorithm(algorithm)
On 2013/07/02 06:46:36, abarth wrote:
> ": Algorithm(algorithm)" should be on it's own line.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
File Source/modules/crypto/AesCbcParams.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
Source/modules/crypto/AesCbcParams.h:45: PassRefPtr<Uint8Array> iv();
On 2013/07/02 06:46:36, abarth wrote:
> This should just be Uint8Array*.  PassRefPtr is used to pass ownership.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesC...
Source/modules/crypto/AesCbcParams.h:48: AesCbcParams(const
WebKit::WebCryptoAlgorithm&);
On 2013/07/02 06:46:36, abarth wrote:
> Please mark one-argument constructors "explicit".

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
File Source/modules/crypto/AesKeyGenParams.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
Source/modules/crypto/AesKeyGenParams.cpp:40: return
m_algorithm.getAesKeyGenParams()->length();
On 2013/07/02 06:46:36, abarth wrote:
> getAesKeyGenParams -> aesKeyGenParams
> 
> There's no need to use the verb "get".  It doesn't mean anything and can just
be
> omitted.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
File Source/modules/crypto/AesKeyGenParams.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/AesK...
Source/modules/crypto/AesKeyGenParams.h:48: AesKeyGenParams(const
WebKit::WebCryptoAlgorithm&);
On 2013/07/02 06:46:36, abarth wrote:
> Please mark one-argument constructors explicit.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Algo...
File Source/modules/crypto/Algorithm.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Algo...
Source/modules/crypto/Algorithm.cpp:42: String Algorithm::name() const
On 2013/07/02 06:46:36, abarth wrote:
> This should return a const String&

Done (however this did mean adding a new field, since
m_algorithm.algorithmName() is a const char*)

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Cryp...
File Source/modules/crypto/CryptoOperation.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Cryp...
Source/modules/crypto/CryptoOperation.h:49: PassRefPtr<Algorithm> algorithm();
On 2013/07/02 06:46:36, abarth wrote:
> PassRefPtr<Algorithm>    ->    Algorithm*

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:55: enum { UnsupportedOp = -1 };
On 2013/07/02 06:46:36, abarth wrote:
> Rather than use an enum, you can just use a global constant holding the value
> -1.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:130: // static
On 2013/07/02 06:46:36, abarth wrote:
> Please skip these sorts of comments.  They're ignored by the compiler and
> therefore become out of date.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:133: // Singleton
On 2013/07/02 06:46:36, abarth wrote:
> Please omit comments that redundantly state what the code does.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:134: static AlgorithmRegistry
registry;
On 2013/07/02 06:46:36, abarth wrote:
> Please use DEFINE_STATIC_LOCAL.  That lets us avoid exit-time destructors.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:136: // Do a case-insensitive
lookup for algorithmName.
On 2013/07/02 06:46:36, abarth wrote:
> Please omit comments that redundantly state what the code does.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:145: // Initialize
AlgorithmRegistry by populating it with the static mappings.
On 2013/07/02 06:46:36, abarth wrote:
> Please skip comments that say what the code does.  We like comments that
explain
> why the code does what it does, but comments that say what the code does are
> redundant can get out of date quickly.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:158:
WebKit::WebCryptoAlgorithmParams* parseAesCbcParams(const Dictionary& raw)
On 2013/07/02 06:46:36, abarth wrote:
> WebKit::WebCryptoAlgorithmParams*   --> 
> PassOwnPtr<WebKit::WebCryptoAlgorithmParams>

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:167: return new
WebKit::WebCryptoAesCbcParams(static_cast<unsigned char*>(iv->baseAddress()),
iv->byteLength());
On 2013/07/02 06:46:36, abarth wrote:
> We try to avoid "naked new" calls.  Please wrap all calls to "new" either in
> adoptPtr or adoptRef.  In this case, we should use adoptPtr.  At the API
> boundary, we'll need to call leakPtr() to state that we're leaking this
pointer
> to the embedder.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:177: return new
WebKit::WebCryptoAesKeyGenParams(length);
On 2013/07/02 06:46:36, abarth wrote:
> Naked new

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:190: 
On 2013/07/02 06:46:36, abarth wrote:
> ASSERT_NOT_REACHED() ?

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:196: // TODO(eroman): Throw the
correct exception types!
On 2013/07/02 06:46:36, abarth wrote:
> "TODO(eroman)"  -->  "FIXME"
> 
> We use FIXME rather than TODO, and we skip the name of the author.  If someone
> wants to know who authored the comment, they can look in the revision history.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:201: // No name was specified by
the AlgorithmIdentifier.
On 2013/07/02 06:46:36, abarth wrote:
> Please remove these sorts of redundant comments.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:207: // The spec defines this case
separately.
On 2013/07/02 06:46:36, abarth wrote:
> Please add a link to the part of the spec you're referring to.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:214: // No algorithm by that name
exists.
On 2013/07/02 06:46:36, abarth wrote:
> Please omit this comment as well.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:220: // The algorithm does not
support the requested operation.
On 2013/07/02 06:46:36, abarth wrote:
> Please omit this redundant comment.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:225: // Parse the
algorithm-specific parameters.
On 2013/07/02 06:46:36, abarth wrote:
> Please omit this redundant comment.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:227:
OwnPtr<WebKit::WebCryptoAlgorithmParams> params =
adoptPtr(parseAlgorithmParams(raw, paramsType));
On 2013/07/02 06:46:36, abarth wrote:
> I see, we're not actually leaking this pointer.  The proper place to call
> adoptPtr is immediately after calling "new".  That way we can check that there
> aren't any leaks by looking at each line individually instead of having to
> understand the whole file at once.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:230: // Failed parsing the
parameters.
On 2013/07/02 06:46:36, abarth wrote:
> Please omit this redundant comment.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
File Source/modules/crypto/NormalizeAlgorithm.h (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.h:55: NumAlgorithmOperations,
On 2013/07/02 06:46:36, abarth wrote:
> NumAlgorithmOperations -> NumberOfAlgorithmOperations
> 
> Please use complete works in variable names.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.h:60: bool normalizeAlgorithm(const
Dictionary& raw, AlgorithmOperation, WebKit::WebCryptoAlgorithm&,
ExceptionCode&) WARN_UNUSED_RETURN;
On 2013/07/02 06:46:36, abarth wrote:
> raw -> You can omit this argument name.  The name doesn't add any information.

Done.

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Subt...
File Source/modules/crypto/SubtleCrypto.idl (right):

https://codereview.chromium.org/18475002/diff/6001/Source/modules/crypto/Subt...
Source/modules/crypto/SubtleCrypto.idl:32: // TODO(eroman): Work in progress:
http://crbug.com/245025.
On 2013/07/02 06:46:36, abarth wrote:
> Please remove this comment.  All code is a work in progress.  :)

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
File public/platform/WebCryptoAlgorithm.h (right):

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:43: // The possible algorithm names.
On 2013/07/02 06:46:36, abarth wrote:
> These aren't algorithm names.  They're algorithm IDs.  I'd probably just
remove
> this comment since it's not correct and redundant with the code.

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:56: // The possible algorithm-specific
parameters.
On 2013/07/02 06:46:36, abarth wrote:
> Please omit this redundant comment.

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:63: // Forward declaration for the
algorithm parameters types.
On 2013/07/02 06:46:36, abarth wrote:
> Please omit this comment that just says what the code does.  We know these are
> forward declarations.

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:81: WEBKIT_EXPORT ~WebCryptoAlgorithm();
On 2013/07/02 06:46:36, abarth wrote:
> We generally don't export destructors.  Instead, we make the destructors
inline
> and call a reset() method that's exported.  You can see the pattern in other
API
> classes.

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:84: WEBKIT_EXPORT WebCryptoAlgorithm&
operator=(const WebCryptoAlgorithm&);
On 2013/07/02 06:46:36, abarth wrote:
> Similarly, we don't export constructors or operators.  Instead, we define
these
> sorts of functions inline and call an exported assign function.  There should
be
> many examples of this pattern in the API.

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:86: // Returns a unique identifier for the
algorithm.
On 2013/07/02 06:46:36, abarth wrote:
> Unique in what sense?  They're just identifiers for the algorithms.  I don't
> think there's anything particularly unique about them.

Removed comment.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:95: WebCryptoAesCbcParams*
getAesCbcParams() const;
On 2013/07/02 06:46:36, abarth wrote:
> getAesCbcParams -> aesCbcParams

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:96: WebCryptoAesKeyGenParams*
getAesKeyGenParams() const;
On 2013/07/02 06:46:36, abarth wrote:
> getAesKeyGenParams  ->  aesKeyGenParams

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
File public/platform/WebCryptoAlgorithmParams.h (right):

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithmParams.h:42: //       Crypto spec. Parameters
have the same name (minus "WebCrypto" prefix).
On 2013/07/02 06:46:36, abarth wrote:
> Please add a link to the spec.

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithmParams.h:55: // Helper class for a chunk of
owned immutable data.
On 2013/07/02 06:46:36, abarth wrote:
> We should be able to just use WebVector

Done.

https://codereview.chromium.org/18475002/diff/6001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithmParams.h:85: // The initialization vector.
MUST be 16 bytes.
On 2013/07/02 06:46:36, abarth wrote:
> Can you enforce that with an ASSERT rather than a comment?  Comments can be
> incorrect, but ASSERTs are verified by tests.

Removed the comment.

I had copy-pasted it from the W3C document, and forgot to delete.

No assert is necessary, since the guarantee is handled by
NormalizeAlgorithm.cpp, and tested for explicitly by the layouttest.

[eroman, 2013-07-02 18:03:43]

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/18475002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp:51:
WebCryptoAlgorithmId algorithmId;
On 2013/07/02 08:12:27, eroman wrote:
> On 2013/07/02 06:46:36, abarth wrote:
> > Id -> ID ?  I'm not sure what's most consistent with existing style...
> 
> I will do some more research on this matter tomorrow. For now I have left it
as
> "Id".

It looks like both styles are equally common in the blink codebase:

Across all directories in WebKit/Source the counts are:
  ID: 2283
  Id: 2450

For just the WebKit API directory however "Id" is 4x more common than "ID":
  ID: 11
  Id: 44

This data doesn't strongly motivate using one style over the other. Given the
slight lead of "Id" (especially in WebKit API) I will use that style.

Testing methodology:

$ cd ROOT_DIR
$ grep -r '[a-z]ID\>' * | egrep '[.](cc|cpp|m|mm|h): ' | wc -l
$ grep -r '[a-z]Id\>' * | egrep '[.](cc|cpp|m|mm|h): ' | wc -l

[abarth-chromium, 2013-07-02 19:02:24]

lgtm

Looks great!  Thanks.

[commit-bot: I haz the power, 2013-07-02 19:02:38]

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

[commit-bot: I haz the power, 2013-07-02 20:26:06]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[eroman, 2013-07-02 21:30:42]

The layout test failure on linux_layout_rel is happening for other commit queue
attempts, so I am presuming unrelated.

Will commit manually once I have verified.

[eroman, 2013-07-02 21:56:58]

Committed patchset #5 manually as r153450 (presubmit successful).