WebCrypto: Add WebKit API structure for keys.

WebCrypto: Add WebKit API structure for keys.

This corresponds to the "Key interface" in the spec:
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#key-interface

This changelist introduces "WebCryptoKey" as a reference-counted wrapper to an opaque key handle.
This opaque key handle is a pointer created by the embedder, which derives from WebCryptoKeyHandle.

Testing: This change does not have any accompanying tests, as there isn't a way to create key objects from Javascript yet. These interfaces will get test coverage once key generation is hooked up.

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

[eroman, 2013-07-03 02:22:57]

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

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Algo...
Source/modules/crypto/Algorithm.cpp:44: return
ASCIILiteral(m_algorithm.algorithmName());
Unrelated cleanup.
I can extract these to a separate changelist if you prefer.

[abarth-chromium, 2013-07-03 03:47:37]

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

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Algo...
Source/modules/crypto/Algorithm.cpp:44: return
ASCIILiteral(m_algorithm.algorithmName());
On 2013/07/03 02:22:57, eroman wrote:
> Unrelated cleanup.
> I can extract these to a separate changelist if you prefer.

I don't think this works.  ASCIILiteral needs to be applied to the actual
literal so that it can autodetect the length.

[abarth-chromium, 2013-07-03 04:07:00]

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

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Algo...
Source/modules/crypto/Algorithm.cpp:44: return
ASCIILiteral(m_algorithm.algorithmName());
On 2013/07/03 03:47:37, abarth wrote:
> On 2013/07/03 02:22:57, eroman wrote:
> > Unrelated cleanup.
> > I can extract these to a separate changelist if you prefer.
> 
> I don't think this works.  ASCIILiteral needs to be applied to the actual
> literal so that it can autodetect the length.

Oh, actually, you're right.  ASCIILiteral doesn't autodetect the length.  It
just saves the copy.

[eroman, 2013-07-03 04:13:55]

My understanding is that ASCIILiteral() creates a String which does not have to
copy the data. Instead, it calls strlen() on the given cstring, and then just
saves the pointer and length.

Here is what I believe to be the relevant code:

PassRefPtr<StringImpl> StringImpl::createFromLiteral(const char* characters)
{
    size_t length = strlen(characters);
    ASSERT_WITH_MESSAGE(length, "Use StringImpl::empty() to create an empty
string");
    ASSERT(charactersAreAllASCII<LChar>(reinterpret_cast<const
LChar*>(characters), length));
    return adoptRef(new StringImpl(characters, length, ConstructFromLiteral));
}

Code links:

[1] 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I have run the layout tests and it appears to work as promised, but I can step
through with the debugger to check for sure if you have concerns on this
approach.

Thank you!

[abarth-chromium, 2013-07-03 04:16:57]

Yeah, you're right.  I was thinking about another place we do something similar.

[abarth-chromium, 2013-07-03 18:01:41]

LGTM modulo the minor issue about keyUsage in the IDL.  We can land this CL and
iterate on that issue with the working group because it's likely a bug in the
spec.

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key.cpp
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key....
Source/modules/crypto/Key.cpp:72: return 0;
ASSERT_NOT_REACHED();

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key....
Source/modules/crypto/Key.cpp:102: Vector<String> v;
v -> result

Please use complete words in variable names.  (We do use i as a look index
though.)

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key.idl
File Source/modules/crypto/Key.idl (right):

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key....
Source/modules/crypto/Key.idl:53: readonly attribute KeyUsage[] keyUsage;
KeyUsage[]   ->  sequence<KeyUsage>  ?

Maybe what you've got here is right, but I'm not sure how foo[] works as a
readonly attribute.  For example, what if JS appends to the array?  Should that
change the keyUsage?

Also, what you've got here returns a new array each time.  Are we supposed to
return the same array object each time script reads the attribute (e.g.,
key.keyUsage === key.keyUsage) ?

Finally, isn't the word "key" here a bit of a stutter?  Why not just "usage"?

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

https://codereview.chromium.org/18033004/diff/3001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:95: WebCryptoAesKeyGenParams*
aesKeyGenParams() const;
Don't you need to add WEBKIT_EXPORT here?

https://codereview.chromium.org/18033004/diff/3001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:99: void reset();
WHy did you remove these WEBKIT_EXPORT macros?  They're needed in the component
build.

https://codereview.chromium.org/18033004/diff/3001/public/platform/WebCryptoK...
File public/platform/WebCryptoKey.h (right):

https://codereview.chromium.org/18033004/diff/3001/public/platform/WebCryptoK...
public/platform/WebCryptoKey.h:100: //   * Returned pointer's lifetime is bound
to |this|
Thanks for the comments about lifetime.

https://codereview.chromium.org/18033004/diff/3001/public/platform/WebCryptoK...
public/platform/WebCryptoKey.h:111: void reset();
These probably need WEBKIT_EXPORT

[eroman, 2013-07-03 19:18:06]

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key.cpp
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key....
Source/modules/crypto/Key.cpp:72: return 0;
On 2013/07/03 18:01:41, abarth wrote:
> ASSERT_NOT_REACHED();

I am in fact relying on the 0 case here:

<long_winded_optional_explanation>

In order to enumerate the WebCrytoKeyUsage in a generic fashion, in
"Key::keyUsage()" I construct WebCryptoKeyUsage values directly out of integers
by using the expression (1 << i), for i starting at 0, up until N.

I didn't want to hard-code the maximum value of "i", for fear that the code
would silently break if new values are added to WebCryptoKeyUsage in the future.
What I did instead is to keep iterating and increasing "i" until
keyUsageToString() returns 0. This indicates that the "usage" value passed in
was not a valid enum, and hence iteration is done.

With this approach, if a new enumerated value gets added to WebCryptoKeyUsage
then compilation of the switch statement will fail with a warning, as it has no
"default" case. Once the developer updates the switch statement to fix the
compile break, then everything else "just works".

The alternative would be to add some "last usage" value to the end of the enum
definition (so the hardcoded bit is closer to the definition, and more likely to
get updated). In this case though, since the enumerated values define a bitfield
(and are not just increasing numbers), then defining and using the sentinel is a
bit uglier (and it ends up polluting the enum with a non-value)

</long_winded_optional_explanation>

In summary: I can add an ASSERT_NOT_REACHED() here, however it will mean
changing the design of the keyUsage() function below.

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key....
Source/modules/crypto/Key.cpp:102: Vector<String> v;
On 2013/07/03 18:01:41, abarth wrote:
> v -> result
> 
> Please use complete words in variable names.  (We do use i as a look index
> though.)

Done.

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key.idl
File Source/modules/crypto/Key.idl (right):

https://codereview.chromium.org/18033004/diff/3001/Source/modules/crypto/Key....
Source/modules/crypto/Key.idl:53: readonly attribute KeyUsage[] keyUsage;
On 2013/07/03 18:01:41, abarth wrote:
> KeyUsage[]   ->  sequence<KeyUsage>  ?
> 
> Maybe what you've got here is right, but I'm not sure how foo[] works as a
> readonly attribute.  For example, what if JS appends to the array?  Should
that
> change the keyUsage?
> 
> Also, what you've got here returns a new array each time.  Are we supposed to
> return the same array object each time script reads the attribute (e.g.,
> key.keyUsage === key.keyUsage) ?
> 
> Finally, isn't the word "key" here a bit of a stutter?  Why not just "usage"?

The web crypto spec currently defines it as an array. I will raise the question
of if it should be a sequence.

That said, Web IDL says that the elements of a read-only array cannot be changed
by the caller:

http://www.w3.org/TR/WebIDL/#idl-array

"Arrays can also be designated as being read only. User code cannot modify the
values of read only array elements. If an array is read only, then it is also
implicitly fixed length."

So this sounds in line with what we want. I will do some tests to verify that
this is what happens from our bindings generator and report back.

As far as not returning a new array each time: that is a good question. Do you
know how I can accomplish that from the blink bindings? Currently I am returning
a |Vector<String>|. However my expectation is that since Vector<> does not
extend ScriptWrappable, it cannot re-use the javascript array object even if I
were to return a |const Vector<String>&| (which is why I didn't bother caching
that vector).

Finally, I agree that the naming of "keyUsage" is not ideal. I have raised the
concern here:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22561

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

https://codereview.chromium.org/18033004/diff/3001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:95: WebCryptoAesKeyGenParams*
aesKeyGenParams() const;
On 2013/07/03 18:01:41, abarth wrote:
> Don't you need to add WEBKIT_EXPORT here?

Done.

https://codereview.chromium.org/18033004/diff/3001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithm.h:99: void reset();
Done.

Sorry, I got confused on what WEBKIT_EXPORT meant, and after talking with
someone got the impression they were not needed on private methods. However
given that these "private methods" are used by the public inlined methods, an
export is required.

[abarth-chromium, 2013-07-03 20:44:34]

On 2013/07/03 19:18:06, eroman wrote:
> In summary: I can add an ASSERT_NOT_REACHED() here, however it will mean
> changing the design of the keyUsage() function below.

Sounds like you're abusing the type system and passing a value outside the range
of the enum...  How hard is it to change the usage to respect the type system?

> That said, Web IDL says that the elements of a read-only array cannot be
changed
> by the caller:
> 
> http://www.w3.org/TR/WebIDL/#idl-array
> 
> "Arrays can also be designated as being read only. User code cannot modify the
> values of read only array elements. If an array is read only, then it is also
> implicitly fixed length."
> 
> So this sounds in line with what we want. I will do some tests to verify that
> this is what happens from our bindings generator and report back.

I don't think we implement read-only arrays.

> As far as not returning a new array each time: that is a good question. Do you
> know how I can accomplish that from the blink bindings? Currently I am
returning
> a |Vector<String>|. However my expectation is that since Vector<> does not
> extend ScriptWrappable, it cannot re-use the javascript array object even if I
> were to return a |const Vector<String>&| (which is why I didn't bother caching
> that vector).

I don't think we implement that either...  We can implement that by caching the
V8 array that gets created on the wrapper object, but they you'll run into
trouble because the array is mutable in JavaScript...

> Sorry, I got confused on what WEBKIT_EXPORT meant, and after talking with
> someone got the impression they were not needed on private methods. However
> given that these "private methods" are used by the public inlined methods, an
> export is required.

Yep.

[eroman, 2013-07-03 21:14:30]

I uploaded 2 alternate implementations in patchset (3), both of which avoid
creating an enum outside of expected range:

https://codereview.chromium.org/18033004/diff2/15002:23001/Source/modules/cry...
Let me know if you prefer either of these approaches.

Regarding the read-only array issue: how about I leave this as a FIXME and
tackle it as a followup?
It will be easier to explore once I have layouttests that exercise Key
creation. In the meantime I don't foresee the slightly off semantics
causing problems with the implementation.

Cheers, and thanks for all the great comments!


On Wed, Jul 3, 2013 at 1:44 PM, <abarth@chromium.org> wrote:

> On 2013/07/03 19:18:06, eroman wrote:
>
>> In summary: I can add an ASSERT_NOT_REACHED() here, however it will mean
>> changing the design of the keyUsage() function below.
>>
>
> Sounds like you're abusing the type system and passing a value outside the
> range
> of the enum...  How hard is it to change the usage to respect the type
> system?
>
>
>  That said, Web IDL says that the elements of a read-only array cannot be
>>
> changed
>
>> by the caller:
>>
>
> 
http://www.w3.org/TR/WebIDL/#**idl-array<http://www.w3.org/TR/WebIDL/#idl-array>
>>
>
>  "Arrays can also be designated as being read only. User code cannot
>> modify the
>> values of read only array elements. If an array is read only, then it is
>> also
>> implicitly fixed length."
>>
>
>  So this sounds in line with what we want. I will do some tests to verify
>> that
>> this is what happens from our bindings generator and report back.
>>
>
> I don't think we implement read-only arrays.
>
>
>  As far as not returning a new array each time: that is a good question.
>> Do you
>> know how I can accomplish that from the blink bindings? Currently I am
>>
> returning
>
>> a |Vector<String>|. However my expectation is that since Vector<> does not
>> extend ScriptWrappable, it cannot re-use the javascript array object even
>> if I
>> were to return a |const Vector<String>&| (which is why I didn't bother
>> caching
>> that vector).
>>
>
> I don't think we implement that either...  We can implement that by
> caching the
> V8 array that gets created on the wrapper object, but they you'll run into
> trouble because the array is mutable in JavaScript...
>
>
>  Sorry, I got confused on what WEBKIT_EXPORT meant, and after talking with
>> someone got the impression they were not needed on private methods.
>> However
>> given that these "private methods" are used by the public inlined
>> methods, an
>> export is required.
>>
>
> Yep.
>
>
https://codereview.chromium.**org/18033004/<https://codereview.chromium.org/1...
>

[abarth-chromium, 2013-07-03 21:22:54]

https://codereview.chromium.org/18033004/diff/23001/Source/modules/crypto/Key...
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/18033004/diff/23001/Source/modules/crypto/Key...
Source/modules/crypto/Key.cpp:109: for (int i = 0; (1 << i) <=
WebKit::EndOfWebCryptoKeyUsage - 1; ++i) {
Why <= and -1 ?  The normal idiom is < and no -1...

https://codereview.chromium.org/18033004/diff/23001/Source/modules/crypto/Key...
Source/modules/crypto/Key.cpp:119: // must also update this function.
I think the first version is better because it's less maintenance.  The
static_cast doesn't bother me as long as we're staying in-range for the enum.

[abarth-chromium, 2013-07-03 21:23:18]

On 2013/07/03 21:14:30, eroman wrote:
> Regarding the read-only array issue: how about I leave this as a FIXME and
> tackle it as a followup?

That's fine.

[eroman, 2013-07-03 21:35:18]

https://codereview.chromium.org/18033004/diff/23001/Source/modules/crypto/Key...
File Source/modules/crypto/Key.cpp (right):

https://codereview.chromium.org/18033004/diff/23001/Source/modules/crypto/Key...
Source/modules/crypto/Key.cpp:119: // must also update this function.
On 2013/07/03 21:22:54, abarth wrote:
> I think the first version is better because it's less maintenance.  The
> static_cast doesn't bother me as long as we're staying in-range for the enum.

I have uploaded a new patch which switches over to the first of the alternate
implementations. PTAL. 

(I still need to do some spot manual testing to make sure it works properly,
since this doesnt have layout tests)

[abarth-chromium, 2013-07-03 21:38:44]

lgtm

[commit-bot: I haz the power, 2013-07-08 00:26:40]

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

[commit-bot: I haz the power, 2013-07-08 01:47:48]

Change committed as 153657