[webcrypto] Add the KeyAlgorithm interface.

[webcrypto] Add the KeyAlgorithm interface.

This matches the changes in https://dvcs.w3.org/hg/webcrypto-api/rev/2fa3494f0179

 * Key.algorithm is now reflected as a KeyAlgorithm interface rather than Algorithm
 * HMAC no longer takes the hash as an operational parameter (key move to import/generation)
 * RSA-SSA no longer takes the hash as an operation (move to key import/generation)parameter

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

[haraken, 2014-02-25 06:41:51]

Once the CL looks OK in terms of semantics, I'm happy to review in terms of
oilpan :)

[abarth-chromium, 2014-02-25 07:51:41]

lgtm

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ae...
File Source/modules/crypto/AesKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ae...
Source/modules/crypto/AesKeyAlgorithm.h:49: void trace(Visitor*);
I don't think this is needed given that we're not adding any members.

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Hm...
File Source/modules/crypto/HmacKeyAlgorithm.cpp (right):

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Hm...
Source/modules/crypto/HmacKeyAlgorithm.cpp:51: void
HmacKeyAlgorithm::trace(Visitor* visitor)
This one is needed.  :)

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ke...
File Source/modules/crypto/KeyAlgorithm.cpp (right):

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ke...
Source/modules/crypto/KeyAlgorithm.cpp:84: void KeyAlgorithm::trace(Visitor*) {
}
The { and } should each be on separate lines.

[eroman, 2014-02-25 08:33:14]

Thanks!

@haraken: Could you take a look at the oilpan side of things, and give me any
pointers on how to verify it?

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ae...
File Source/modules/crypto/AesKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ae...
Source/modules/crypto/AesKeyAlgorithm.h:49: void trace(Visitor*);
On 2014/02/25 07:51:41, abarth wrote:
> I don't think this is needed given that we're not adding any members.

Inlined it instead (I believe it needs to be defined even when empty)

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ke...
File Source/modules/crypto/KeyAlgorithm.cpp (right):

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ke...
Source/modules/crypto/KeyAlgorithm.cpp:84: void KeyAlgorithm::trace(Visitor*) {
}
On 2014/02/25 07:51:41, abarth wrote:
> The { and } should each be on separate lines.

Moved to header

[Mads Ager (chromium), 2014-02-25 08:49:24]

LGTM on the oilpan side.

You can test it by putting enable_oilpan=1 in your GYP_DEFINES and running
layout tests. We are still working on try bot support.

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ae...
File Source/modules/crypto/AesKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/20001/Source/modules/crypto/Ae...
Source/modules/crypto/AesKeyAlgorithm.h:49: void trace(Visitor*);
On 2014/02/25 08:33:15, eroman wrote:
> On 2014/02/25 07:51:41, abarth wrote:
> > I don't think this is needed given that we're not adding any members.
> 
> Inlined it instead (I believe it needs to be defined even when empty)

It doesn't actually have to be there. If it is not, the one in KeyAlgorithm will
be called. I like having it though. When we have a hierarchy with virtual
methods and therefore a virtual trace method, we define the trace methods for
the entire hierarchy in order to avoid bugs where we end up visiting only half
of the inheritance hierarchy. I think it makes it simpler if the rule of thumb
is to always define trace methods for the entire hierarchy.

[haraken, 2014-02-25 09:15:45]

https://codereview.chromium.org/179353002/diff/40001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8KeyAlgorithmCustom.cpp (right):

https://codereview.chromium.org/179353002/diff/40001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8KeyAlgorithmCustom.cpp:42: v8::Handle<v8::Object>
wrap(KeyAlgorithm* impl, v8::Handle<v8::Object> creationContext, v8::Isolate*
isolate)

Probably you might want to remove the custom binding by using [SpecialWrapFor].
Then you can remove this file.

See
https://code.google.com/p/chromium/codesearch#search/&q=specialwrapfor&sq=pac...

https://codereview.chromium.org/179353002/diff/40001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/179353002/diff/40001/Source/heap/ThreadState....
Source/heap/ThreadState.h:550: USED_FROM_MULTIPLE_THREADS(KeyAlgorithm);

Nit: Alphabetical order please?

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Ae...
File Source/modules/crypto/AesKeyAlgorithm.idl (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Ae...
Source/modules/crypto/AesKeyAlgorithm.idl:32: WillBeGarbageCollected,

[WillBeGarbageCollected] is not needed in subclasses. The IDL attribute is
inherited.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Hm...
File Source/modules/crypto/HmacKeyAlgorithm.idl (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Hm...
Source/modules/crypto/HmacKeyAlgorithm.idl:32: WillBeGarbageCollected,

Ditto. Remove this.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaHashedKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:44: void trace(Visitor*);

Add 'virtual' and 'OVERRIDE'. The same comment for other places.

Probably you don't want to add virtual functions for some reason? Then you can
use traceAfterDispatch() pattern. See how the CSSValue hierarchy is implemented.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:47: RsaHashedKeyAlgorithm(const
blink::WebCryptoKeyAlgorithm&);

Add 'explicit'.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:49:
RefPtrWillBeMember<KeyAlgorithm> m_hash;

Can this be a private member?

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaHashedKeyAlgorithm.idl (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.idl:32: WillBeGarbageCollected,

Remove [WillBeGarbageCollected].

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaKeyAlgorithm.h:51: RefPtr<Uint8Array> m_publicExponent;

Can this be a private member?

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaKeyAlgorithm.idl (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaKeyAlgorithm.idl:32: WillBeGarbageCollected,

Remove [WillBeGarbageCollected].

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

https://codereview.chromium.org/179353002/diff/40001/Source/platform/exported...
Source/platform/exported/WebCryptoKeyAlgorithm.cpp:2: * Copyright (C) 2014
Google Inc. All rights reserved.

You can use the 4-line copyright if you want. The same comment for other new
files.

[Mads Ager (chromium), 2014-02-25 09:34:17]

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaHashedKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:44: void trace(Visitor*);
On 2014/02/25 09:15:46, haraken wrote:
> 
> Add 'virtual' and 'OVERRIDE'. The same comment for other places.
> 
> Probably you don't want to add virtual functions for some reason? Then you can
> use traceAfterDispatch() pattern. See how the CSSValue hierarchy is
implemented.

Good catch Haraken! KeyAlgorithm.h does not have a vtable. Therefore, the
question here is if it should have or if you always know the exact type of the
KeyAlgorithm you have a pointer to? I was assuming that you do, but that is
incorrect (right below there is a pointer to a KeyAlgorithm). If trace is not
virtual it needs to be explicitly dispatched (which means that you need a type
marker in the superclass to know the actual subtype you have - see CSSValue.h
and CSSValue.cpp for an example). I agree with Haraken that we should add a
vtable to this one and make the dispatch virtual here.

[eroman, 2014-02-26 01:37:35]

Thanks for the comments haraken/mads!

https://codereview.chromium.org/179353002/diff/40001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8KeyAlgorithmCustom.cpp (right):

https://codereview.chromium.org/179353002/diff/40001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8KeyAlgorithmCustom.cpp:42: v8::Handle<v8::Object>
wrap(KeyAlgorithm* impl, v8::Handle<v8::Object> creationContext, v8::Isolate*
isolate)
On 2014/02/25 09:15:46, haraken wrote:
> 
> Probably you might want to remove the custom binding by using
[SpecialWrapFor].
> Then you can remove this file.
> 
> See
>
https://code.google.com/p/chromium/codesearch#search/&q=specialwrapfor&sq=pac...

Done. Cool, didn't know about that!

https://codereview.chromium.org/179353002/diff/40001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/179353002/diff/40001/Source/heap/ThreadState....
Source/heap/ThreadState.h:550: USED_FROM_MULTIPLE_THREADS(KeyAlgorithm);
On 2014/02/25 09:15:46, haraken wrote:
> 
> Nit: Alphabetical order please?

Done.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Ae...
File Source/modules/crypto/AesKeyAlgorithm.idl (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Ae...
Source/modules/crypto/AesKeyAlgorithm.idl:32: WillBeGarbageCollected,
On 2014/02/25 09:15:46, haraken wrote:
> 
> [WillBeGarbageCollected] is not needed in subclasses. The IDL attribute is
> inherited.

Done.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaHashedKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:44: void trace(Visitor*);
On 2014/02/25 09:34:18, Mads Ager (chromium) wrote:
> On 2014/02/25 09:15:46, haraken wrote:
> > 
> > Add 'virtual' and 'OVERRIDE'. The same comment for other places.
> > 
> > Probably you don't want to add virtual functions for some reason? Then you
can
> > use traceAfterDispatch() pattern. See how the CSSValue hierarchy is
> implemented.
> 
> Good catch Haraken! KeyAlgorithm.h does not have a vtable. Therefore, the
> question here is if it should have or if you always know the exact type of the
> KeyAlgorithm you have a pointer to? I was assuming that you do, but that is
> incorrect (right below there is a pointer to a KeyAlgorithm). If trace is not
> virtual it needs to be explicitly dispatched (which means that you need a type
> marker in the superclass to know the actual subtype you have - see CSSValue.h
> and CSSValue.cpp for an example). I agree with Haraken that we should add a
> vtable to this one and make the dispatch virtual here.

Done.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:47: RsaHashedKeyAlgorithm(const
blink::WebCryptoKeyAlgorithm&);
On 2014/02/25 09:15:46, haraken wrote:
> 
> Add 'explicit'.

Done.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:47: RsaHashedKeyAlgorithm(const
blink::WebCryptoKeyAlgorithm&);
On 2014/02/25 09:15:46, haraken wrote:
> 
> Add 'explicit'.

Done.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.h:49:
RefPtrWillBeMember<KeyAlgorithm> m_hash;
On 2014/02/25 09:15:46, haraken wrote:
> 
> Can this be a private member?

Done.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaHashedKeyAlgorithm.idl (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaHashedKeyAlgorithm.idl:32: WillBeGarbageCollected,
On 2014/02/25 09:15:46, haraken wrote:
> 
> Remove [WillBeGarbageCollected].

Done.

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
File Source/modules/crypto/RsaKeyAlgorithm.h (right):

https://codereview.chromium.org/179353002/diff/40001/Source/modules/crypto/Rs...
Source/modules/crypto/RsaKeyAlgorithm.h:51: RefPtr<Uint8Array> m_publicExponent;
On 2014/02/25 09:15:46, haraken wrote:
> 
> Can this be a private member?

Done.

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

https://codereview.chromium.org/179353002/diff/40001/Source/platform/exported...
Source/platform/exported/WebCryptoKeyAlgorithm.cpp:2: * Copyright (C) 2014
Google Inc. All rights reserved.
On 2014/02/25 09:15:46, haraken wrote:
> 
> You can use the 4-line copyright if you want. The same comment for other new
> files.

Thanks. I will keep it as-is, to match the other files for now.

[haraken, 2014-02-26 02:33:09]

Thanks for the update! LGTM.

https://codereview.chromium.org/179353002/diff/80001/public/platform/WebCrypto.h
File public/platform/WebCrypto.h (right):

https://codereview.chromium.org/179353002/diff/80001/public/platform/WebCrypt...
public/platform/WebCrypto.h:40: #define WEBCRYPTO_HAS_KEY_ALGORITHM 1

Where is this used?

[eroman, 2014-02-26 02:35:42]

https://codereview.chromium.org/179353002/diff/80001/public/platform/WebCrypto.h
File public/platform/WebCrypto.h (right):

https://codereview.chromium.org/179353002/diff/80001/public/platform/WebCrypt...
public/platform/WebCrypto.h:40: #define WEBCRYPTO_HAS_KEY_ALGORITHM 1
On 2014/02/26 02:33:09, haraken wrote:
> 
> Where is this used?

It is used by this changelist for the blink side:
https://codereview.chromium.org/178073007/

(Since I made changes to the blink API which break the chromium side)

[eroman, 2014-02-26 15:47:13]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-26 15:47:27]

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

[commit-bot: I haz the power, 2014-02-26 15:47:37]

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

[commit-bot: I haz the power, 2014-02-26 15:47:37]

Failed to apply patch for Source/modules/crypto/RsaHashedKeyAlgorithm.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source/modules/crypto/RsaHashedKeyAlgorithm.h
  Copied Source/modules/serviceworkers/InstallPhaseEvent.h ->
Source/modules/crypto/RsaHashedKeyAlgorithm.h
  patching file Source/modules/crypto/RsaHashedKeyAlgorithm.h
  Hunk #1 FAILED at 28.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/crypto/RsaHashedKeyAlgorithm.h.rej

Patch:   NR 
Source/modules/serviceworkers/InstallPhaseEvent.h->Source/modules/crypto/RsaHashedKeyAlgorithm.h
Index: Source/modules/crypto/RsaHashedKeyAlgorithm.h
diff --git a/Source/modules/serviceworkers/InstallPhaseEvent.h
b/Source/modules/crypto/RsaHashedKeyAlgorithm.h
similarity index 75%
copy from Source/modules/serviceworkers/InstallPhaseEvent.h
copy to Source/modules/crypto/RsaHashedKeyAlgorithm.h
index
da6f057a2cb65ff028f4bfd4d5648350850834d9..53ad9767082d87f1b807caef5b27467e873dee93
100644
--- a/Source/modules/serviceworkers/InstallPhaseEvent.h
+++ b/Source/modules/crypto/RsaHashedKeyAlgorithm.h
@@ -28,31 +28,29 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef InstallPhaseEvent_h
-#define InstallPhaseEvent_h
+#ifndef RsaHashedKeyAlgorithm_h
+#define RsaHashedKeyAlgorithm_h
 
-#include "bindings/v8/ScriptValue.h"
-#include "core/events/Event.h"
+#include "modules/crypto/RsaKeyAlgorithm.h"
 
 namespace WebCore {
 
-class WaitUntilObserver;
-
-class InstallPhaseEvent : public Event {
+class RsaHashedKeyAlgorithm : public RsaKeyAlgorithm {
 public:
-    static PassRefPtr<InstallPhaseEvent> create();
+    static PassRefPtrWillBeRawPtr<RsaHashedKeyAlgorithm> create(const
blink::WebCryptoKeyAlgorithm&);
 
-    virtual ~InstallPhaseEvent();
+    KeyAlgorithm* hash();
 
-    void waitUntil(const ScriptValue&);
+    virtual void trace(Visitor*) OVERRIDE;
 
-protected:
-    InstallPhaseEvent();
-    InstallPhaseEvent(const AtomicString& type, const EventInit&,
PassRefPtr<WaitUntilObserver>);
+private:
+    explicit RsaHashedKeyAlgorithm(const blink::WebCryptoKeyAlgorithm&);
 
-    RefPtr<WaitUntilObserver> m_observer;
+    RefPtrWillBeMember<KeyAlgorithm> m_hash;
 };
 
+DEFINE_KEY_ALGORITHM_TYPE_CASTS(RsaHashedKeyAlgorithm);
+
 } // namespace WebCore
 
-#endif // InstallPhaseEvent_h
+#endif

[eroman, 2014-02-26 19:57:39]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-26 19:57:49]

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

[eroman, 2014-02-26 21:02:18]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-26 21:02:32]

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

[commit-bot: I haz the power, 2014-02-27 00:06:53]

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

[commit-bot: I haz the power, 2014-02-27 00:06:54]

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

[eroman, 2014-02-27 00:13:47]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-27 00:14:31]

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

[commit-bot: I haz the power, 2014-02-27 04:10:23]

Change committed as 167979