Refactor the interface for generating keys.

Refactor the interface for generating keys.

This allows the removal of the IsAlgorithmAsymmetric() helper.

BUG=407846
Committed: https://crrev.com/9b747eaff7397e5eaf7e122161797ba1e8e9afd2
Cr-Commit-Position: refs/heads/master@{#300219}

[eroman, 2014-08-28 00:12:49]

Patchset #1 (id:1) has been deleted

[eroman, 2014-08-28 00:12:58]

eroman@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2014-08-28 00:35:33]

https://codereview.chromium.org/512023002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/algorithm_dispatch.h (right):

https://codereview.chromium.org/512023002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/algorithm_dispatch.h:54: blink::WebCryptoKey*
private_key);
From an API level, this feels wrong. I liked your original interfaces much
better.

[eroman, 2014-08-28 14:34:19]

https://codereview.chromium.org/512023002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/algorithm_dispatch.h (right):

https://codereview.chromium.org/512023002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/algorithm_dispatch.h:54: blink::WebCryptoKey*
private_key);
On 2014/08/28 00:35:33, Ryan Sleevi wrote:
> From an API level, this feels wrong. I liked your original interfaces much
> better.

Would you prefer:
  (1) Keep the algorithm_dispatch exported interface the same as before, but the
internal AlgorithmImplementation the way it is in this changelist
  (2) Remove this API change alltogether, and introduce a new virtual method on
AlgorithmImplementation for "bool KeysAreAsymmetric()"

[Ryan Sleevi, 2014-08-28 21:15:04]

https://codereview.chromium.org/512023002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/algorithm_dispatch.h (right):

https://codereview.chromium.org/512023002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/algorithm_dispatch.h:54: blink::WebCryptoKey*
private_key);
On 2014/08/28 14:34:19, eroman wrote:
> On 2014/08/28 00:35:33, Ryan Sleevi wrote:
> > From an API level, this feels wrong. I liked your original interfaces much
> > better.
> 
> Would you prefer:
>   (1) Keep the algorithm_dispatch exported interface the same as before, but
the
> internal AlgorithmImplementation the way it is in this changelist

To be clearer/explicit, I dislike the union of both symmetric and asymmetric
keys into the same interface on a C++ level.

>   (2) Remove this API change alltogether, and introduce a new virtual method
on
> AlgorithmImplementation for "bool KeysAreAsymmetric()"

I'm not sure why this is necessary.

[eroman, 2014-08-28 21:28:11]

See the bug description. My goal is to remove IsAlgorithmAsymmetric().

It is not something I care for from a maintainability standpoint, since it is
yet another switch statement that needs to be edited when new algorithms are
introduced.

Instead I want new algorithm implementations to be self-contained by created an
AlgorithmImplementation subclass and registering it.

So any per-algorithm metadata should be contained in the
AlgorithmImplementation.

[eroman, 2014-09-12 02:33:57]

Patchset #3 (id:60001) has been deleted

[eroman, 2014-09-12 02:34:04]

Patchset #3 (id:80001) has been deleted

[eroman, 2014-09-12 02:34:37]

I have updated this CL based on our conversation, PTAL

[eroman, 2014-10-15 03:02:00]

ping

[Ryan Sleevi, 2014-10-17 21:01:58]

Mostly a good cleanup, but I'm a bit confused about the seemingly non-idiomatic
mutable_* result (which I've only really seen in the protobuf code), which you
seem to be doing to facilitate one or two files that want to write into a
pointer.

Wouldn't it make more sense, and be a bit idiomatic, to do the

Foo some_value;
CallSomeMethod(..., &some_value);
result->set_public_key(some_value);

then the (non-idiomatic?)

CallSomeMethod(..., &result->mutable_public_key())

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/generate_key_result.cc (right):

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
content/child/webcrypto/generate_key_result.cc:51: ;
del

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/generate_key_result.h (right):

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
content/child/webcrypto/generate_key_result.h:57: blink::WebCryptoKey
private_key_;
since blink::WebCryptoKey types are direct-assignable (you relied on this in the
old and new code), why have mutable_secret_key instead of set_secret_key? The
latter makes it clear it's an assignment, while the former allows you to mux
around with the internal state beyond assignment (which none of the files seem
to do / need)

[eroman, 2014-10-17 22:43:12]

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/generate_key_result.cc (right):

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
content/child/webcrypto/generate_key_result.cc:51: ;
On 2014/10/17 21:01:58, Ryan Sleevi wrote:
> del

Done.

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
File content/child/webcrypto/generate_key_result.h (right):

https://codereview.chromium.org/512023002/diff/100001/content/child/webcrypto...
content/child/webcrypto/generate_key_result.h:57: blink::WebCryptoKey
private_key_;
On 2014/10/17 21:01:58, Ryan Sleevi wrote:
> since blink::WebCryptoKey types are direct-assignable (you relied on this in
the
> old and new code), why have mutable_secret_key instead of set_secret_key? The
> latter makes it clear it's an assignment, while the former allows you to mux
> around with the internal state beyond assignment (which none of the files seem
> to do / need)

Done. I agree mutable_ was a bit odd and not idiomatic.

... I don't remember what my thinking was at time time. Probably I saw that as
the most direct translation when refactoring, or was overly concerned with
copying.

[Ryan Sleevi, 2014-10-17 22:44:25]

lgtm

[eroman, 2014-10-17 23:05:01]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-10-17 23:06:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/512023002/160001

[commit-bot: I haz the power, 2014-10-18 01:43:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-18 01:43:58]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[eroman, 2014-10-18 19:16:36]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-10-18 19:17:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/512023002/160001

[commit-bot: I haz the power, 2014-10-18 22:03:38]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2014-10-18 22:04:31]

Patchset 6 (id:??) landed as
https://crrev.com/9b747eaff7397e5eaf7e122161797ba1e8e9afd2
Cr-Commit-Position: refs/heads/master@{#300219}