Separate CDM initialization from GenerateKeyRequest & remove key_system parameters.

Separate CDM initialization from GenerateKeyRequest & remove key_system parameters.

This eliminates unnecessary parameters and prepares for integration with the new API.

BUG=249976, 250048
TBR=cdn@chromium.org

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

[ddorwin, 2013-06-17 20:57:11]

I need to update the Android code after this gets reviewed.

[xhwang, 2013-06-18 00:43:42]

Looking good w/ only nits.

Will stampit after Android is updated as well.

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:634: std::string(),
media::MediaKeys::kUnknownError, 0);
nit: fit in one line?

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:664: std::string(),
media::MediaKeys::kUnknownError, 0);
ditto

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:684: std::string(),
media::MediaKeys::kUnknownError, 0);
ditto

https://codereview.chromium.org/17289006/diff/1017/webkit/renderer/media/cryp...
File webkit/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/17289006/diff/1017/webkit/renderer/media/cryp...
webkit/renderer/media/crypto/proxy_decryptor.cc:179: return
scoped_ptr<media::Decryptor>(decryptor);
Can PpapiDecryptor::Create() return scoped_ptr directly?

[ddorwin, 2013-06-18 16:46:21]

Now with Android changes.

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:634: std::string(),
media::MediaKeys::kUnknownError, 0);
On 2013/06/18 00:43:42, xhwang wrote:
> nit: fit in one line?

Done.

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:664: std::string(),
media::MediaKeys::kUnknownError, 0);
On 2013/06/18 00:43:42, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/17289006/diff/1017/webkit/plugins/ppapi/conte...
webkit/plugins/ppapi/content_decryptor_delegate.cc:684: std::string(),
media::MediaKeys::kUnknownError, 0);
On 2013/06/18 00:43:42, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/17289006/diff/1017/webkit/renderer/media/cryp...
File webkit/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/17289006/diff/1017/webkit/renderer/media/cryp...
webkit/renderer/media/crypto/proxy_decryptor.cc:179: return
scoped_ptr<media::Decryptor>(decryptor);
On 2013/06/18 00:43:42, xhwang wrote:
> Can PpapiDecryptor::Create() return scoped_ptr directly?

Done.

[qinmin, 2013-06-18 17:01:00]

https://codereview.chromium.org/17289006/diff/53001/content/common/media/medi...
File content/common/media/media_player_messages_android.h (left):

https://codereview.chromium.org/17289006/diff/53001/content/common/media/medi...
content/common/media/media_player_messages_android.h:220: std::string /*
key_system */,
If no key_system in GenerateKeyRequest, do we need a separate message for it?
I am wondering how to support systems other than widewine. I am ok removing it
from the rest of the messages, but we still need to tell the DRM bridge sth
about the key_system

[ddorwin, 2013-06-18 17:34:06]

https://codereview.chromium.org/17289006/diff/53001/content/common/media/medi...
File content/common/media/media_player_messages_android.h (left):

https://codereview.chromium.org/17289006/diff/53001/content/common/media/medi...
content/common/media/media_player_messages_android.h:220: std::string /*
key_system */,
On 2013/06/18 17:01:00, qinmin wrote:
> If no key_system in GenerateKeyRequest, do we need a separate message for it?
> I am wondering how to support systems other than widewine. I am ok removing it
> from the rest of the messages, but we still need to tell the DRM bridge sth
> about the key_system

The key system should be specified before generating a key request. This is how
the new EME API works and can be made compatible with the existing one as I have
done for desktop. We'll need to add an Initialize message similar to
https://codereview.chromium.org/17289006/diff/53001/webkit/plugins/ppapi/cont....
Since I was not working on an Android workspace and the bridge is not yet
implemented, I left that for a future CL.

[qinmin, 2013-06-18 17:58:35]

lgtm

[xhwang, 2013-06-18 18:20:12]

lgtm

[ddorwin, 2013-06-18 18:31:42]

yfriedman, please review the first two files. (Should we move these to a media/
directory or give qinmin per-file OWNERS permissions?)

dmichael, please review webkit/plugins/ppapi/.

[ddorwin, 2013-06-18 18:47:16]

jschuh, please review content/common/media/media_player_messages_android.h.

[Yaron, 2013-06-18 20:24:39]

content/browser/android lgtm

[dmichael (off chromium), 2013-06-18 21:45:31]

one comment, o/w lgtm

https://codereview.chromium.org/17289006/diff/53001/webkit/plugins/ppapi/cont...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/17289006/diff/53001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:647:
DCHECK(StringVar::FromPPVar(key_system_var)->value() == key_system_);
I think it would be better to explicitly make sure you get a valid StringVar
also. And I prefer we don't crash Chrome on bad inputs from the plugin/cdm (even
in Debug). Would a DLOG or DLOG_IF work for you?
DLOG_IF(!StringVar::FromPPVar(key_system_var) ||
        StringVar::FromPPVar(key_system_var)->value() != key_system_) <<
    "Your message here";

https://codereview.chromium.org/17289006/diff/53001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:678:
DCHECK(StringVar::FromPPVar(key_system_var)->value() == key_system_);
ditto

[ddorwin, 2013-06-18 23:58:17]

cdn, please review content/common/media/media_player_messages_android.h.

[ddorwin, 2013-06-19 00:29:27]

https://codereview.chromium.org/17289006/diff/53001/webkit/plugins/ppapi/cont...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/17289006/diff/53001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:647:
DCHECK(StringVar::FromPPVar(key_system_var)->value() == key_system_);
On 2013/06/18 21:45:31, dmichael wrote:
> I think it would be better to explicitly make sure you get a valid StringVar
> also. And I prefer we don't crash Chrome on bad inputs from the plugin/cdm
(even
> in Debug). Would a DLOG or DLOG_IF work for you?
> DLOG_IF(!StringVar::FromPPVar(key_system_var) ||
>         StringVar::FromPPVar(key_system_var)->value() != key_system_) <<
>     "Your message here";

I just removed these checks since they aren't that valuable and the parameter
will be removed (TODO added).

https://codereview.chromium.org/17289006/diff/53001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:678:
DCHECK(StringVar::FromPPVar(key_system_var)->value() == key_system_);
On 2013/06/18 21:45:31, dmichael wrote:
> ditto

Done.

[ddorwin, 2013-06-19 00:52:20]

Committed patchset #13 manually as r207131 (presubmit successful).