Move OnNeedKey() to WebMediaPlayerImpl.

Move OnNeedKey() to WebMediaPlayerImpl.

OnNeedKey() is needed for both prefixed and unprefixed EME. Move it out of EncryptedMediaPlayerSupport so that EncryptedMediaPlayerSupport is for prefixed EME only.

BUG=249976
Committed: https://crrev.com/bab66f5a4aac5b611ea0008f322391e0bb25eaff
Cr-Commit-Position: refs/heads/master@{#306493}

[xhwang, 2014-12-02 18:08:17]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org

[xhwang, 2014-12-02 18:08:18]

xhwang@chromium.org changed required reviewers:
+ ddorwin@chromium.org

[xhwang, 2014-12-02 18:08:18]

PTAL

[ddorwin, 2014-12-02 20:49:49]

LG overall. Some minor comments.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:661: // TODO(xhwang): We assume all streams
are from the same container (thus have
Note: YouTube is currently violating this assumption.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:662: // the same "type") for now. In the
future, the "type" should be passed down
ditto

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:706: void WebMediaPlayerImpl::OnNeedKey(const
std::string& type,
i_d_t

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:708: // Do not fire NeedKey event if
encrypted media is not enabled.
I know this was copied, but should client_ be handling this instead? Maybe a
future CL.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:716: DCHECK(init_data_type_.empty() ||
type.empty() || type == init_data_type_);
Can type be empty? The demuxers that call this always know their IDT.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:716: DCHECK(init_data_type_.empty() ||
type.empty() || type == init_data_type_);
YouTube currently violates this without problems in opt builds. Since the
condition does not indicate a code bug, maybe we should replace it with a
DLOG_IF() or NOTSUPPORTED(), assuming that just logs.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:318: // Temporary for EME v0.1. In the future
the init data type should be passed
Future? Isn't this already implemented? Can we drop the second sentence?

A better solution might be to add a setIDT() to the WMPS interface. What do you
think?

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:320: std::string init_data_type_;
Let's name it prefixed_eme_... or something like that to avoid confusion.

[xhwang, 2014-12-02 21:57:39]

comments addressed

[xhwang, 2014-12-02 21:57:47]

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:661: // TODO(xhwang): We assume all streams
are from the same container (thus have
On 2014/12/02 20:49:49, ddorwin wrote:
> Note: YouTube is currently violating this assumption.

Acknowledged.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:662: // the same "type") for now. In the
future, the "type" should be passed down
On 2014/12/02 20:49:49, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:706: void WebMediaPlayerImpl::OnNeedKey(const
std::string& type,
On 2014/12/02 20:49:49, ddorwin wrote:
> i_d_t

Done.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:708: // Do not fire NeedKey event if
encrypted media is not enabled.
On 2014/12/02 20:49:49, ddorwin wrote:
> I know this was copied, but should client_ be handling this instead? Maybe a
> future CL.

Added TODO.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:716: DCHECK(init_data_type_.empty() ||
type.empty() || type == init_data_type_);
On 2014/12/02 20:49:49, ddorwin wrote:
> Can type be empty? The demuxers that call this always know their IDT.

Good catch. Done.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:716: DCHECK(init_data_type_.empty() ||
type.empty() || type == init_data_type_);
On 2014/12/02 20:49:49, ddorwin wrote:
> YouTube currently violates this without problems in opt builds. Since the
> condition does not indicate a code bug, maybe we should replace it with a
> DLOG_IF() or NOTSUPPORTED(), assuming that just logs.

Done.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:318: // Temporary for EME v0.1. In the future
the init data type should be passed
On 2014/12/02 20:49:49, ddorwin wrote:
> Future? Isn't this already implemented? Can we drop the second sentence?
> 
> A better solution might be to add a setIDT() to the WMPS interface. What do
you
> think?

Done.

https://codereview.chromium.org/759933003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.h:320: std::string init_data_type_;
On 2014/12/02 20:49:49, ddorwin wrote:
> Let's name it prefixed_eme_... or something like that to avoid confusion.

Not needed since init_data_type_ is moved back to EMPS, which is prefixed EME
only.

[ddorwin, 2014-12-02 22:06:38]

LGTM with comments. Thanks!

https://codereview.chromium.org/759933003/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/759933003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:1746:
DCHECK(init_data_type_.empty() || init_data_type.empty() ||
We should log instead of check here too.

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
File media/blink/encrypted_media_player_support.cc (right):

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
media/blink/encrypted_media_player_support.cc:186: if
(!proxy_decryptor_->GenerateKeyRequest(init_data_type, init_data,
nit: IMO when grouping, we should group pointers with their length.

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
File media/blink/encrypted_media_player_support.h (right):

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
media/blink/encrypted_media_player_support.h:66: void SetInitDataType(const
std::string& init_data_type);
nit: It would reduce diffs if this was on the same line as the removed function.
See the diff against base.

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
media/blink/encrypted_media_player_support.h:108: // We assume all streams are
from the same container, thus have the same
Ditto, move above 103.

[xhwang, 2014-12-02 22:31:10]

comments addressed

[xhwang, 2014-12-02 22:31:27]

https://codereview.chromium.org/759933003/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/759933003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:1746:
DCHECK(init_data_type_.empty() || init_data_type.empty() ||
On 2014/12/02 22:06:38, ddorwin wrote:
> We should log instead of check here too.

Done.

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
File media/blink/encrypted_media_player_support.cc (right):

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
media/blink/encrypted_media_player_support.cc:186: if
(!proxy_decryptor_->GenerateKeyRequest(init_data_type, init_data,
On 2014/12/02 22:06:38, ddorwin wrote:
> nit: IMO when grouping, we should group pointers with their length.

I'am now totally on clang format and not doing any manual tweak for
style/format...

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
File media/blink/encrypted_media_player_support.h (right):

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
media/blink/encrypted_media_player_support.h:66: void SetInitDataType(const
std::string& init_data_type);
On 2014/12/02 22:06:38, ddorwin wrote:
> nit: It would reduce diffs if this was on the same line as the removed
function.
> See the diff against base.

Done.

https://codereview.chromium.org/759933003/diff/20001/media/blink/encrypted_me...
media/blink/encrypted_media_player_support.h:108: // We assume all streams are
from the same container, thus have the same
On 2014/12/02 22:06:38, ddorwin wrote:
> Ditto, move above 103.

Done.

[xhwang, 2014-12-02 23:25:22]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-12-02 23:26:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759933003/40001

[commit-bot: I haz the power, 2014-12-02 23:49:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-02 23:50:31]

Patchset 3 (id:??) landed as
https://crrev.com/bab66f5a4aac5b611ea0008f322391e0bb25eaff
Cr-Commit-Position: refs/heads/master@{#306493}