Add Decryptor interface.

Add Decryptor interface.

This is in preparation for the ProxyDecryptor and PpapiDecryptor.

BUG=123260
TEST=media_unittests, media layout tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143030

[xhwang, 2012-06-14 00:17:15]

Hello scherkus and ddorwin,

I created this patch just to get preliminary comments about what I plan to do
(so please don't take time reviewing details of this patch), which is as
follows:

1) Extract Decryptor interface and let AesDecryptor be a subclass of it. (This
issue).
2) Make a ProxyDecryptor which creates the AesDecryptor when GenerateKeyRequest
is called, and forwards all other calls to the AesDecryptor.
3) Make a CdmDecryptor which is another subclass of Decryptor. Having the
ProxyDecryptor create the CdmDecryptor when proper key system is specified.

Please let me know if this plan sounds right or not.

xhwang

[scherkus (not reviewing), 2012-06-14 22:41:02]

looks sane to me

let me know when you want me to review

http://codereview.chromium.org/10539150/diff/1/media/crypto/decryptor.h
File media/crypto/decryptor.h (right):

http://codereview.chromium.org/10539150/diff/1/media/crypto/decryptor.h#newco...
media/crypto/decryptor.h:40: DecryptorClient* client() const;
do users of Decryptor need to access client() or is this here for convenience
for subclasses?

[xhwang, 2012-06-18 04:29:49]

Hello scherkus and ddorwin,

I have rebased this CL and made some minor fixes. Please review.

xhwang

http://codereview.chromium.org/10539150/diff/1/media/crypto/decryptor.h
File media/crypto/decryptor.h (right):

http://codereview.chromium.org/10539150/diff/1/media/crypto/decryptor.h#newco...
media/crypto/decryptor.h:40: DecryptorClient* client() const;
On 2012/06/14 22:41:02, scherkus wrote:
> do users of Decryptor need to access client() or is this here for convenience
> for subclasses?

It's only for convenience for subclasses. I'll move it to "protected".

[xhwang, 2012-06-18 18:34:16]

On 2012/06/18 04:29:49, xhwang wrote:
> Hello scherkus and ddorwin,
> 
> I have rebased this CL and made some minor fixes. Please review.
> 
> xhwang
> 
> http://codereview.chromium.org/10539150/diff/1/media/crypto/decryptor.h
> File media/crypto/decryptor.h (right):
> 
>
http://codereview.chromium.org/10539150/diff/1/media/crypto/decryptor.h#newco...
> media/crypto/decryptor.h:40: DecryptorClient* client() const;
> On 2012/06/14 22:41:02, scherkus wrote:
> > do users of Decryptor need to access client() or is this here for
convenience
> > for subclasses?
> 
> It's only for convenience for subclasses. I'll move it to "protected".

Also, based on the discussion offline, are we in agreement that:

1, Decryptor.h lives in src/media/base/
2, AesDecryptor stays in src/media/crypto/
3, The plugin based decryptor will be named PpapiDecryptor.
4, The upcoming ProxyDecryptor (the wrapper) and PpapiDecryptor live in
src/webkit/media/. Note that ProxyDecryptor needs to be in src/webkit/media also
because it needs to instantiate the PpapiDecryptor.

[ddorwin, 2012-06-18 19:07:21]

lgtm

http://codereview.chromium.org/10539150/diff/3001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10539150/diff/3001/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:23: // Decrypts AES encrypted buffer into
unencrypted buffer.
Is this comment really descriptive of the class? Maybe "AES Clear Key decryptor"
or something similar.

s/unencrypted/decrypted/. Unencrypted generally means it was never encrypted.

[xhwang, 2012-06-19 01:27:23]

Hello scherkus and ddorwin,

I moved the decryptor.* and decryptor_client.h from src/media/crypto/ to
src/media/base. Please review again.

xhwang

http://codereview.chromium.org/10539150/diff/3001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10539150/diff/3001/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:23: // Decrypts AES encrypted buffer into
unencrypted buffer.
On 2012/06/18 19:07:22, ddorwin wrote:
> Is this comment really descriptive of the class? Maybe "AES Clear Key
decryptor"
> or something similar.
> 
> s/unencrypted/decrypted/. Unencrypted generally means it was never encrypted.

Done.

[ddorwin, 2012-06-19 01:39:35]

lgtm

http://codereview.chromium.org/10539150/diff/9001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10539150/diff/9001/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:23: // Clear Key Decryptor implementation that
decrypts AES encrypted buffer.
nit: AES-encrypted (with dash)

[scherkus (not reviewing), 2012-06-19 03:03:24]

http://codereview.chromium.org/10539150/diff/9001/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/10539150/diff/9001/media/base/decryptor.h#newc...
media/base/decryptor.h:73: DecryptorClient* client_;
I really don't like the idea of having this here out of convenience -- I don't
think it's saving us any work.

http://codereview.chromium.org/10539150/diff/9001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10539150/diff/9001/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:23: // Clear Key Decryptor implementation that
decrypts AES encrypted buffer.
I'd remove mention of "Clear Key" -- that has more to do w/ how the key is
transported. The AesDecryptor could decrypt AES-encrypted content where the key
was transmitted by carrier pigeon :)

[xhwang, 2012-06-19 16:29:53]

http://codereview.chromium.org/10539150/diff/9001/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/10539150/diff/9001/media/base/decryptor.h#newc...
media/base/decryptor.h:73: DecryptorClient* client_;
On 2012/06/19 03:03:24, scherkus wrote:
> I really don't like the idea of having this here out of convenience -- I don't
> think it's saving us any work.

Done.

http://codereview.chromium.org/10539150/diff/9001/media/crypto/aes_decryptor.h
File media/crypto/aes_decryptor.h (right):

http://codereview.chromium.org/10539150/diff/9001/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:23: // Clear Key Decryptor implementation that
decrypts AES encrypted buffer.
On 2012/06/19 03:03:24, scherkus wrote:
> I'd remove mention of "Clear Key" -- that has more to do w/ how the key is
> transported. The AesDecryptor could decrypt AES-encrypted content where the
key
> was transmitted by carrier pigeon :)

Done.

http://codereview.chromium.org/10539150/diff/9001/media/crypto/aes_decryptor....
media/crypto/aes_decryptor.h:23: // Clear Key Decryptor implementation that
decrypts AES encrypted buffer.
On 2012/06/19 01:39:35, ddorwin wrote:
> nit: AES-encrypted (with dash)

Done.

[scherkus (not reviewing), 2012-06-19 16:46:00]

LGTM w/ nit

http://codereview.chromium.org/10539150/diff/22001/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/10539150/diff/22001/media/base/decryptor.h#new...
media/base/decryptor.h:34: Decryptor();
nit: since this is a pure virtual class you can inline the ctor + dtor and
delete the .cc file

[xhwang, 2012-06-19 16:54:57]

http://codereview.chromium.org/10539150/diff/22001/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/10539150/diff/22001/media/base/decryptor.h#new...
media/base/decryptor.h:34: Decryptor();
On 2012/06/19 16:46:00, scherkus wrote:
> nit: since this is a pure virtual class you can inline the ctor + dtor and
> delete the .cc file

Done.

[commit-bot: I haz the power, 2012-06-19 18:47:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10539150/19006

[commit-bot: I haz the power, 2012-06-19 20:06:22]

Change committed as 143030