Expose WebCrypto's lookupAlgorithmInfo.

Expose WebCrypto's lookupAlgorithmInfo.

Will allow reuse of this function in the implementation of the enterprise.platfromKeys Chrome extension API.

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

[pneubeck (no reviews), 2014-06-06 12:23:42]

ptal

@Adam, I need your lgtm for
  Source/platform/exported/WebCryptoAlgorithm.cpp
  public/platform/WebCryptoAlgorithm.h

[abarth-chromium, 2014-06-06 17:07:51]

What is enterprise.platfromKeys?

[abarth-chromium, 2014-06-06 17:10:09]

I'd like to understand better why we're making this change...

https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:49: const char*
kAlgorithmNameAesKw = "AES-KW";
char* -> char[]

https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:106: }
Can we use a static array instead of a switch statement?

[pneubeck (no reviews), 2014-06-06 18:08:32]

https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:106: }
On 2014/06/06 17:10:08, abarth wrote:
> Can we use a static array instead of a switch statement?

I did this so that the compiler can potentially check whether a case is missing.

[pneubeck (no reviews), 2014-06-06 18:15:26]

On 2014/06/06 17:10:09, abarth wrote:
> I'd like to understand better why we're making this change...
I was asked for this by Eric Roman:
https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...

The enterprise.platformKeys Chrome extension API is implementing the
SubtleCrypto interface of WebCrypto.
Therefor it also has to do the normalization of algorithm parameters. For that
we're already reusing the normalizeAlgorithm function (see the already committed
CL).
The normalization result contains algorithm IDs which I have to translate back
to the algorithm names (to pass them on to the custom bindings or browser
process).
Instead of copying that translation, Eric asked for sharing that function.

> 
>
https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
> File Source/platform/exported/WebCryptoAlgorithm.cpp (right):
> 
>
https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
> Source/platform/exported/WebCryptoAlgorithm.cpp:49: const char*
> kAlgorithmNameAesKw = "AES-KW";
> char* -> char[]
> 
>
https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
> Source/platform/exported/WebCryptoAlgorithm.cpp:106: }
> Can we use a static array instead of a switch statement?

[abarth-chromium, 2014-06-06 18:20:53]

On 2014/06/06 18:15:26, pneubeck wrote:
> On 2014/06/06 17:10:09, abarth wrote:
> > I'd like to understand better why we're making this change...
> I was asked for this by Eric Roman:
>
https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
> 
> The enterprise.platformKeys Chrome extension API is implementing the
> SubtleCrypto interface of WebCrypto.

Why are we creating a second implementation of the SubtleCrypto interface?

It looks like you've already landed a number of CLs along these lines.  It might
be better for you to continue working with your existing reviewers who are
likely to have more context for what you're working on...  Please feel free to
ignore me.

[pneubeck (no reviews), 2014-06-06 18:24:14]

On 2014/06/06 18:20:53, abarth wrote:
> On 2014/06/06 18:15:26, pneubeck wrote:
> > On 2014/06/06 17:10:09, abarth wrote:
> > > I'd like to understand better why we're making this change...
> > I was asked for this by Eric Roman:
> >
>
https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
> > 
> > The enterprise.platformKeys Chrome extension API is implementing the
> > SubtleCrypto interface of WebCrypto.
> 
> Why are we creating a second implementation of the SubtleCrypto interface?
> 
> It looks like you've already landed a number of CLs along these lines.  It
might
> be better for you to continue working with your existing reviewers who are
> likely to have more context for what you're working on...  Please feel free to
> ignore me.

Yeah, I landed one blink change and that was reviewed by Jochen who is now on
vacation :-/

[pneubeck (no reviews), 2014-06-10 12:24:35]

@Eric, friendly ping
@Adam, can you nonetheless take a look? Not sure whether you want to rely on
Eric's judgement or whether I shall give you the necessary context. In case of
the latter, we could chat/VC and I can explain the necessary to you.

[pneubeck (no reviews), 2014-06-10 12:33:02]

https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:49: const char*
kAlgorithmNameAesKw = "AES-KW";
On 2014/06/06 17:10:09, abarth wrote:
> char* -> char[]

Done.

[eroman, 2014-06-12 05:54:45]

The refactor LGTM.

However it is unfortunate that the platform/exported code can't just call into
the modules/crypto code to do this, so that the single source of truth can just
live there.

[pneubeck (no reviews), 2014-06-12 07:58:15]

On 2014/06/12 05:54:45, eroman wrote:
> The refactor LGTM.
> 
> However it is unfortunate that the platform/exported code can't just call into
> the modules/crypto code to do this, so that the single source of truth can
just
> live there.

@Adam, still needs your lgtm for Source/platform/exported/ and public/platform/
. Thanks

[abarth-chromium, 2014-06-12 17:25:16]

On 2014/06/06 at 18:08:32, pneubeck wrote:
>
https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
> File Source/platform/exported/WebCryptoAlgorithm.cpp (right):
> 
>
https://codereview.chromium.org/312393004/diff/20001/Source/platform/exported...
> Source/platform/exported/WebCryptoAlgorithm.cpp:106: }
> On 2014/06/06 17:10:08, abarth wrote:
> > Can we use a static array instead of a switch statement?
> 
> I did this so that the compiler can potentially check whether a case is
missing.

We can use an ASSERT for the same purpose via WTF_ARRAY_LENGTH.  We prefer
static tables because they take less binary.

[abarth-chromium, 2014-06-12 17:25:27]

https://codereview.chromium.org/312393004/diff/40001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/40001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:106: }
Can this be a static table instead of code?

[pneubeck (no reviews), 2014-06-12 18:06:46]

https://codereview.chromium.org/312393004/diff/40001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/40001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:106: }
On 2014/06/12 17:25:27, abarth wrote:
> Can this be a static table instead of code?
Is a map of pairs
  struct { blink::WebCryptoAlgorithmId id; const char* name }
ok, or is even that too much overhead?
The most light-weight being an array of const char*, which would however rely on
the IDs' integer values never being changed (if it's not combined with an ugly
list of asserts).

[eroman, 2014-06-12 19:16:58]

https://codereview.chromium.org/312393004/diff/40001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/40001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:106: }
On 2014/06/12 18:06:46, pneubeck wrote:
> On 2014/06/12 17:25:27, abarth wrote:
> > Can this be a static table instead of code?
> Is a map of pairs
>   struct { blink::WebCryptoAlgorithmId id; const char* name }
> ok, or is even that too much overhead?
> The most light-weight being an array of const char*, which would however rely
on
> the IDs' integer values never being changed (if it's not combined with an ugly
> list of asserts).

Perhaps a solution would be to extract all of AlgorithmInfo from
NormalizeAlgorithm.cpp to blink::WebCryptoAlgorithmInfo, rather than splitting
off just the id to name mapping.

[pneubeck (no reviews), 2014-06-13 18:31:18]

ptal. I followed Eric's last suggestion, which should address Adams request.
If you could manage to review it today, that would help me a lot, as this is
blocking 3 other CLs.

Thanks!

[abarth-chromium, 2014-06-13 21:53:02]

lgtm

[eroman, 2014-06-13 22:57:55]

lgtm

https://codereview.chromium.org/312393004/diff/80001/Source/modules/crypto/No...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/80001/Source/modules/crypto/No...
Source/modules/crypto/NormalizeAlgorithm.cpp:707:
context.add(blink::WebCryptoAlgorithm::idToName(algorithmId));
This could be algorithmInfo->name instead

[pneubeck (no reviews), 2014-06-16 07:42:12]

I removed the idToName function from the public interface, to keep the interface
redundancy free.

https://codereview.chromium.org/312393004/diff/80001/Source/modules/crypto/No...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/312393004/diff/80001/Source/modules/crypto/No...
Source/modules/crypto/NormalizeAlgorithm.cpp:707:
context.add(blink::WebCryptoAlgorithm::idToName(algorithmId));
On 2014/06/13 22:57:55, eroman wrote:
> This could be algorithmInfo->name instead

Done.

[pneubeck (no reviews), 2014-06-16 07:42:21]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-16 07:42:25]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-16 07:43:17]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-16 07:44:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/312393004/120001

[commit-bot: I haz the power, 2014-06-16 08:51:44]

Change committed as 176190