Separate MediaKeys interface from Decryptor interface.

Separate MediaKeys interface from Decryptor interface.

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203026

[xhwang, 2013-05-24 00:25:12]

The first attempt based on our offline discussion. I have a question in
proxy_decryptor.h.

media_unittests needs more update which I will do after we agree upon details
about this CL.

PTAL.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
File webkit/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.h:102: media::Decryptor* decryptor_;
I like keeping MediaKeys and Decryptor separate. But this is the ugly part of
this CL that I don't like.

I am thinking: In desktop chromium's implementation the real Decryptor
(AesDecryptor and PpapiDecryptor) always implements both MediaKeys and Decryptor
interface. So maybe Decryptor interface could also inherit MediaKeys interface.
Then we don't have this problem (two pointers for the same object).

WDYT?

[ddorwin, 2013-05-24 20:39:12]

LG. Thanks.

https://codereview.chromium.org/15772012/diff/1/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/15772012/diff/1/media/base/decryptor.h#newcode31
media/base/decryptor.h:31: // TODO(xhwang): Replace kError with kDecryptError
and kDecodeError.
There are a bunch of cleanup TODOs in this file. Probably a good idea to address
them (in a future CL) before adding more implementations.

https://codereview.chromium.org/15772012/diff/1/media/base/decryptor.h#newcode49
media/base/decryptor.h:49: // Indicates that a new key has been added to the
Decryptor.
Is "Decryptor" correct?

https://codereview.chromium.org/15772012/diff/1/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/15772012/diff/1/media/base/media_keys.h#newco...
media/base/media_keys.h:29: kServiceError,
At some point, we should comment out this one and the rest until the errors are
finalized. We only use the first two so far.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/ppapi_dec...
File webkit/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/ppapi_dec...
webkit/media/crypto/ppapi_decryptor.cc:265: void PpapiDecryptor::KeyError(const
std::string& key_system,
Why do we need a proxy for key_error_cb_? Is it just how plugin_cdm_delegate_
works? (I noticed that AesDecryptor does not have such a proxy.)

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
File webkit/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:125: 
We should probably
  DCHECK((media_keys_ == NULL) == (decryptor_ == NULL));
And maybe similar checks at 128 and 134.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:136: if
(!media_keys_->GenerateKeyRequest(key_system, type,
We only use media_keys_ in three places. This class would probably be cleaner if
we had decryptor_ continue to own the object and called decryptor_->media_keys()
in those three locations.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:197: scoped_ptr<media::AesDecryptor>
aes_decryptor(new media::AesDecryptor(
media_keys_ =

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:211: scoped_ptr<PpapiDecryptor>
ppapi_decryptor = CreatePpapiDecryptor(key_system);
same

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
File webkit/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.h:29: class ProxyDecryptor : public
media::MediaKeys {
We should probably rename this class in a future CL.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.h:102: media::Decryptor* decryptor_;
On 2013/05/24 00:25:12, xhwang wrote:
> I like keeping MediaKeys and Decryptor separate. But this is the ugly part of
> this CL that I don't like.
> 
> I am thinking: In desktop chromium's implementation the real Decryptor
> (AesDecryptor and PpapiDecryptor) always implements both MediaKeys and
Decryptor
> interface. So maybe Decryptor interface could also inherit MediaKeys
interface.
> Then we don't have this problem (two pointers for the same object).
> 
> WDYT?

The inheritance would be for convenience and not really any real relationship.
What about exposing MediaKeys from the Decryptor? (I really like the idea of a
Decryptor(MediaKeys*) constructor, but that may not actually be practical. This
is close.

See my comment in the .cc file, which I think makes an argument for exposing it
in this way.

[xhwang, 2013-05-24 23:15:56]

Comments resolved. I still need to update tests later but before that
PTAnotherL.

https://codereview.chromium.org/15772012/diff/1/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/15772012/diff/1/media/base/decryptor.h#newcode31
media/base/decryptor.h:31: // TODO(xhwang): Replace kError with kDecryptError
and kDecodeError.
On 2013/05/24 20:39:12, ddorwin wrote:
> There are a bunch of cleanup TODOs in this file. Probably a good idea to
address
> them (in a future CL) before adding more implementations.

Will do after this CL.

https://codereview.chromium.org/15772012/diff/1/media/base/decryptor.h#newcode49
media/base/decryptor.h:49: // Indicates that a new key has been added to the
Decryptor.
On 2013/05/24 20:39:12, ddorwin wrote:
> Is "Decryptor" correct?

Done.

https://codereview.chromium.org/15772012/diff/1/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/15772012/diff/1/media/base/media_keys.h#newco...
media/base/media_keys.h:29: kServiceError,
On 2013/05/24 20:39:12, ddorwin wrote:
> At some point, we should comment out this one and the rest until the errors
are
> finalized. We only use the first two so far.

ok

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/ppapi_dec...
File webkit/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/ppapi_dec...
webkit/media/crypto/ppapi_decryptor.cc:265: void PpapiDecryptor::KeyError(const
std::string& key_system,
On 2013/05/24 20:39:12, ddorwin wrote:
> Why do we need a proxy for key_error_cb_? Is it just how plugin_cdm_delegate_
> works? (I noticed that AesDecryptor does not have such a proxy.)

We don't like to have callbacks jump layers, i.e. an object should not pass a
callback that belongs to another object to a third object. Passing key_error_cb_
directly in line 64 would violate this rule.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
File webkit/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:125: 
On 2013/05/24 20:39:12, ddorwin wrote:
> We should probably
>   DCHECK((media_keys_ == NULL) == (decryptor_ == NULL));
> And maybe similar checks at 128 and 134.

This is not necessary now.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:136: if
(!media_keys_->GenerateKeyRequest(key_system, type,
On 2013/05/24 20:39:12, ddorwin wrote:
> We only use media_keys_ in three places. This class would probably be cleaner
if
> we had decryptor_ continue to own the object and called
decryptor_->media_keys()
> in those three locations.

Done.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:197: scoped_ptr<media::AesDecryptor>
aes_decryptor(new media::AesDecryptor(
On 2013/05/24 20:39:12, ddorwin wrote:
> media_keys_ =

This change is reverted.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.cc:211: scoped_ptr<PpapiDecryptor>
ppapi_decryptor = CreatePpapiDecryptor(key_system);
On 2013/05/24 20:39:12, ddorwin wrote:
> same

This change is reverted.

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
File webkit/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.h:29: class ProxyDecryptor : public
media::MediaKeys {
On 2013/05/24 20:39:12, ddorwin wrote:
> We should probably rename this class in a future CL.

Any idea on the new name? How about MediaKeysManager or DecryptorManager?

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.h:102: media::Decryptor* decryptor_;
On 2013/05/24 20:39:12, ddorwin wrote:
> On 2013/05/24 00:25:12, xhwang wrote:
> > I like keeping MediaKeys and Decryptor separate. But this is the ugly part
of
> > this CL that I don't like.
> > 
> > I am thinking: In desktop chromium's implementation the real Decryptor
> > (AesDecryptor and PpapiDecryptor) always implements both MediaKeys and
> Decryptor
> > interface. So maybe Decryptor interface could also inherit MediaKeys
> interface.
> > Then we don't have this problem (two pointers for the same object).
> > 
> > WDYT?
> 
> The inheritance would be for convenience and not really any real relationship.
> What about exposing MediaKeys from the Decryptor? (I really like the idea of a
> Decryptor(MediaKeys*) constructor, but that may not actually be practical.
This
> is close.
> 
> See my comment in the .cc file, which I think makes an argument for exposing
it
> in this way.

Done.

[ddorwin, 2013-05-28 19:44:32]

lgtm with comments. Thanks!

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
File webkit/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.h:29: class ProxyDecryptor : public
media::MediaKeys {
On 2013/05/24 23:15:56, xhwang wrote:
> On 2013/05/24 20:39:12, ddorwin wrote:
> > We should probably rename this class in a future CL.
> 
> Any idea on the new name? How about MediaKeysManager or DecryptorManager?

I'm not sure. It still hides the separation of Decryptor/MediaKeys from WMPI.
Maybe it will fade away with the new APIs.

At the very least, it's not a Decryptor - it contains one. DecryptorProxy or
DecryptorAdapter would be a big improvement. I don't think it's worth changing
at this point since we don't know what it should be. However, you might add a
TODO here that notes we need to change the name or delete this.

https://codereview.chromium.org/15772012/diff/10001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/15772012/diff/10001/media/base/decryptor.h#ne...
media/base/decryptor.h:52: // to be valid during the Decryptor's lifetime.
I guess we'll need to destroy the Decryptor when the JS MediaKeys is detached.
That might make DVD/DAD interesting.

https://codereview.chromium.org/15772012/diff/10001/media/base/decryptor.h#ne...
media/base/decryptor.h:55: // Indicates that a new key has been added to the
MediaKey object associated
MediaKey_s_

[xhwang, 2013-05-28 23:50:39]

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
File webkit/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/15772012/diff/1/webkit/media/crypto/proxy_dec...
webkit/media/crypto/proxy_decryptor.h:29: class ProxyDecryptor : public
media::MediaKeys {
On 2013/05/28 19:44:33, ddorwin wrote:
> On 2013/05/24 23:15:56, xhwang wrote:
> > On 2013/05/24 20:39:12, ddorwin wrote:
> > > We should probably rename this class in a future CL.
> > 
> > Any idea on the new name? How about MediaKeysManager or DecryptorManager?
> 
> I'm not sure. It still hides the separation of Decryptor/MediaKeys from WMPI.
> Maybe it will fade away with the new APIs.
> 
> At the very least, it's not a Decryptor - it contains one. DecryptorProxy or
> DecryptorAdapter would be a big improvement. I don't think it's worth changing
> at this point since we don't know what it should be. However, you might add a
> TODO here that notes we need to change the name or delete this.

Added TODO.

https://codereview.chromium.org/15772012/diff/10001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/15772012/diff/10001/media/base/decryptor.h#ne...
media/base/decryptor.h:55: // Indicates that a new key has been added to the
MediaKey object associated
On 2013/05/28 19:44:33, ddorwin wrote:
> MediaKey_s_

Done.

[xhwang, 2013-05-28 23:53:11]

dmichael@: could you please do an OWNERS review on
webkit/plugins/ppapi/content_decryptor_delegate.*

The original Decryptor interface is split to two interfaces: Decryptor and
MediaKeys. The changes in webkit/plugins/ppapi/content_decryptor_delegate.*
reflects this change.

[dmichael (off chromium), 2013-05-29 21:31:22]

lgtm

[commit-bot: I haz the power, 2013-05-29 21:32:39]

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

[commit-bot: I haz the power, 2013-05-30 00:40:58]

Change committed as 203026