Separate encrypted media functions out of WebMediaPlayerClient interface

Separate encrypted media functions out of WebMediaPlayerClient interface

Added a new interface WebMediaPlayerEncryptedMediaClient which will
facilitate in removing the MediaPlayer and MediaPlayerClient abstractions.
This interface will be used to separate the encrypted media related
functions out of WebMediaPlayerClient interface.

This interface is implemented by WebMediaPlayerClient.

Added new createMediaPlayer() method to pass an extra client parameter
for WebMediaPlayerEncryptedMediaClient which then will be used on
chromium side to adapt encrypted media related events to this new client.

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

[Srirama, 2015-05-08 06:56:59]

Patchset #1 (id:1) has been deleted

[Srirama, 2015-05-08 07:55:02]

srirama.m@samsung.com changed reviewers:
+ ddorwin@chromium.org, philipj@opera.com

[Srirama, 2015-05-08 07:55:03]

PTAL

[philipj_slow, 2015-05-08 08:18:55]

Can you update the title/description to match what this specific step is doing?
You're just changing header files, so it seems like a strange thing to land in
separation.

[Srirama, 2015-05-08 10:09:28]

On 2015/05/08 08:18:55, philipj_UTC2 wrote:
> Can you update the title/description to match what this specific step is
doing?
> You're just changing header files, so it seems like a strange thing to land in
> separation.

I have updated the title and description and i have given the blink CL reference
as well.
PTAL.

[philipj_slow, 2015-05-08 11:23:35]

But this is also a Blink CL, is it part of a 3-sided patch where this is the
first and there's a Chromium CL to go in before
https://codereview.chromium.org/1055503002/ ?

[Srirama, 2015-05-08 11:56:56]

On 2015/05/08 11:23:35, philipj_UTC2 wrote:
> But this is also a Blink CL, is it part of a 3-sided patch where this is the
> first and there's a Chromium CL to go in before
> https://codereview.chromium.org/1055503002/ ?

Sorry for the confusion, it is a 3-sided patch. Let me explain the complete
steps.

To avoid EME bits from HTMLMediaElement, i am going to pass one more client
object (WebMediaPlayerEncryptedmedia implemented by
HTMLMediaElementEncryptedMedia) to createMediaPlayer function in
WebFrameClient.h which is implemented in chromium side(render_frame_impl.cc). So
i am planning to do in the following way.

1) Add new method createWebMediaPlayer to accept new parameter so that i need
not disturb the existing createMediaPlayer method. Also add the new interface
WebMediaPlayerEncryptedMediaClient which is referenced in WebFrameClient.
2) Implement the new method createWebMediaPlayer on chromium side
render_frame_impl and pass this client to webMediaPlayer_impl and
webmediaplayer_android and invoke the EME methods through this client.
3) Now do the blink side work to implement WebMediaPlayerClient in
HTMLMediaElement and WebMediaPlayerEncryptedMediaClient in
HTMLMediaElementEncyptedMedia and remove the MediaPlayer and MediaPlayerClient
interfaces, Which is being done in https://codereview.chromium.org/1055503002/.
Thought there are again some more steps here in addition to this, as i cannot do
them as part of this blink CL (https://codereview.chromium.org/1055503002/).
a) Keep empty implementations for EME methods in HTMLMediaElement
b) Remove the current invocation of these EME methods in WebMediaPlayer side
c) Then remove these methods from HTMLMediaElement and WebMediaPlayerClient
d) Also i need to remove the existing createMediaPlayer method from chromium
side (render_frame_impl) first and finally i will remove it from blink side
(WebFrameClient)

Please feel free to question me on the above steps if i have overlooked
something.

[Srirama, 2015-05-08 12:06:28]

On 2015/05/08 11:56:56, Srirama wrote:
> On 2015/05/08 11:23:35, philipj_UTC2 wrote:
> > But this is also a Blink CL, is it part of a 3-sided patch where this is the
> > first and there's a Chromium CL to go in before
> > https://codereview.chromium.org/1055503002/ ?
> 
> Sorry for the confusion, it is a 3-sided patch. Let me explain the complete
> steps.
> 
> To avoid EME bits from HTMLMediaElement, i am going to pass one more client
> object (WebMediaPlayerEncryptedmedia implemented by
> HTMLMediaElementEncryptedMedia) to createMediaPlayer function in
> WebFrameClient.h which is implemented in chromium side(render_frame_impl.cc).
So
> i am planning to do in the following way.
> 
> 1) Add new method createWebMediaPlayer to accept new parameter so that i need
> not disturb the existing createMediaPlayer method. Also add the new interface
> WebMediaPlayerEncryptedMediaClient which is referenced in WebFrameClient.
> 2) Implement the new method createWebMediaPlayer on chromium side
> render_frame_impl and pass this client to webMediaPlayer_impl and
> webmediaplayer_android and invoke the EME methods through this client.
> 3) Now do the blink side work to implement WebMediaPlayerClient in
> HTMLMediaElement and WebMediaPlayerEncryptedMediaClient in
> HTMLMediaElementEncyptedMedia and remove the MediaPlayer and MediaPlayerClient
> interfaces, Which is being done in
https://codereview.chromium.org/1055503002/.
> Thought there are again some more steps here in addition to this, as i cannot
do
> them as part of this blink CL (https://codereview.chromium.org/1055503002/).
> a) Keep empty implementations for EME methods in HTMLMediaElement
> b) Remove the current invocation of these EME methods in WebMediaPlayer side
> c) Then remove these methods from HTMLMediaElement and WebMediaPlayerClient
> d) Also i need to remove the existing createMediaPlayer method from chromium
> side (render_frame_impl) first and finally i will remove it from blink side
> (WebFrameClient)
> 
> Please feel free to question me on the above steps if i have overlooked
> something.

Shall i upload the chromium side patch (step 2) as well and give a referece to
it in the description?

[Srirama, 2015-05-08 12:26:58]

On 2015/05/08 12:06:28, Srirama wrote:
> On 2015/05/08 11:56:56, Srirama wrote:
> > On 2015/05/08 11:23:35, philipj_UTC2 wrote:
> > > But this is also a Blink CL, is it part of a 3-sided patch where this is
the
> > > first and there's a Chromium CL to go in before
> > > https://codereview.chromium.org/1055503002/ ?
> > 
> > Sorry for the confusion, it is a 3-sided patch. Let me explain the complete
> > steps.
> > 
> > To avoid EME bits from HTMLMediaElement, i am going to pass one more client
> > object (WebMediaPlayerEncryptedmedia implemented by
> > HTMLMediaElementEncryptedMedia) to createMediaPlayer function in
> > WebFrameClient.h which is implemented in chromium
side(render_frame_impl.cc).
> So
> > i am planning to do in the following way.
> > 
> > 1) Add new method createWebMediaPlayer to accept new parameter so that i
need
> > not disturb the existing createMediaPlayer method. Also add the new
interface
> > WebMediaPlayerEncryptedMediaClient which is referenced in WebFrameClient.
> > 2) Implement the new method createWebMediaPlayer on chromium side
> > render_frame_impl and pass this client to webMediaPlayer_impl and
> > webmediaplayer_android and invoke the EME methods through this client.
> > 3) Now do the blink side work to implement WebMediaPlayerClient in
> > HTMLMediaElement and WebMediaPlayerEncryptedMediaClient in
> > HTMLMediaElementEncyptedMedia and remove the MediaPlayer and
MediaPlayerClient
> > interfaces, Which is being done in
> https://codereview.chromium.org/1055503002/.
> > Thought there are again some more steps here in addition to this, as i
cannot
> do
> > them as part of this blink CL (https://codereview.chromium.org/1055503002/).
> > a) Keep empty implementations for EME methods in HTMLMediaElement
> > b) Remove the current invocation of these EME methods in WebMediaPlayer side
> > c) Then remove these methods from HTMLMediaElement and WebMediaPlayerClient
> > d) Also i need to remove the existing createMediaPlayer method from chromium
> > side (render_frame_impl) first and finally i will remove it from blink side
> > (WebFrameClient)
> > 
> > Please feel free to question me on the above steps if i have overlooked
> > something.
> 
> Shall i upload the chromium side patch (step 2) as well and give a referece to
> it in the description?

Sorry again, as part of step2 i need to do even more work, considering android
part.

[philipj_slow, 2015-05-08 12:49:41]

On 2015/05/08 11:56:56, Srirama wrote:
> On 2015/05/08 11:23:35, philipj_UTC2 wrote:
> > But this is also a Blink CL, is it part of a 3-sided patch where this is the
> > first and there's a Chromium CL to go in before
> > https://codereview.chromium.org/1055503002/ ?
> 
> Sorry for the confusion, it is a 3-sided patch. Let me explain the complete
> steps.
> 
> To avoid EME bits from HTMLMediaElement, i am going to pass one more client
> object (WebMediaPlayerEncryptedmedia implemented by
> HTMLMediaElementEncryptedMedia) to createMediaPlayer function in
> WebFrameClient.h which is implemented in chromium side(render_frame_impl.cc).
So
> i am planning to do in the following way.
> 
> 1) Add new method createWebMediaPlayer to accept new parameter so that i need
> not disturb the existing createMediaPlayer method. Also add the new interface
> WebMediaPlayerEncryptedMediaClient which is referenced in WebFrameClient.
> 2) Implement the new method createWebMediaPlayer on chromium side
> render_frame_impl and pass this client to webMediaPlayer_impl and
> webmediaplayer_android and invoke the EME methods through this client.

Is there any reason that the new method can't also be called createMediaPlayer
just with different arguments? That would be more in line with the other create*
methods in WebFrameClient.h

> 3) Now do the blink side work to implement WebMediaPlayerClient in
> HTMLMediaElement and WebMediaPlayerEncryptedMediaClient in
> HTMLMediaElementEncyptedMedia and remove the MediaPlayer and MediaPlayerClient
> interfaces, Which is being done in
https://codereview.chromium.org/1055503002/.
> Thought there are again some more steps here in addition to this, as i cannot
do
> them as part of this blink CL (https://codereview.chromium.org/1055503002/).
> a) Keep empty implementations for EME methods in HTMLMediaElement
> b) Remove the current invocation of these EME methods in WebMediaPlayer side
> c) Then remove these methods from HTMLMediaElement and WebMediaPlayerClient
> d) Also i need to remove the existing createMediaPlayer method from chromium
> side (render_frame_impl) first and finally i will remove it from blink side
> (WebFrameClient)
> 
> Please feel free to question me on the above steps if i have overlooked
> something.

If you're not yet entirely sure of how everything will fit together, you could
try to write the change as one step without regard for the Chromium-Blink split,
then figure out how to actually land it in 2 or 3 steps. In any event, it would
be easier to review the whole change as a unit, regardless or how it lands.

[Srirama, 2015-05-08 13:58:08]

On 2015/05/08 12:49:41, philipj_UTC2 wrote:
> On 2015/05/08 11:56:56, Srirama wrote:
> > On 2015/05/08 11:23:35, philipj_UTC2 wrote:
> > > But this is also a Blink CL, is it part of a 3-sided patch where this is
the
> > > first and there's a Chromium CL to go in before
> > > https://codereview.chromium.org/1055503002/ ?
> > 
> > Sorry for the confusion, it is a 3-sided patch. Let me explain the complete
> > steps.
> > 
> > To avoid EME bits from HTMLMediaElement, i am going to pass one more client
> > object (WebMediaPlayerEncryptedmedia implemented by
> > HTMLMediaElementEncryptedMedia) to createMediaPlayer function in
> > WebFrameClient.h which is implemented in chromium
side(render_frame_impl.cc).
> So
> > i am planning to do in the following way.
> > 
> > 1) Add new method createWebMediaPlayer to accept new parameter so that i
need
> > not disturb the existing createMediaPlayer method. Also add the new
interface
> > WebMediaPlayerEncryptedMediaClient which is referenced in WebFrameClient.
> > 2) Implement the new method createWebMediaPlayer on chromium side
> > render_frame_impl and pass this client to webMediaPlayer_impl and
> > webmediaplayer_android and invoke the EME methods through this client.
> 
> Is there any reason that the new method can't also be called createMediaPlayer
> just with different arguments? That would be more in line with the other
create*
> methods in WebFrameClient.h
> 
> > 3) Now do the blink side work to implement WebMediaPlayerClient in
> > HTMLMediaElement and WebMediaPlayerEncryptedMediaClient in
> > HTMLMediaElementEncyptedMedia and remove the MediaPlayer and
MediaPlayerClient
> > interfaces, Which is being done in
> https://codereview.chromium.org/1055503002/.
> > Thought there are again some more steps here in addition to this, as i
cannot
> do
> > them as part of this blink CL (https://codereview.chromium.org/1055503002/).
> > a) Keep empty implementations for EME methods in HTMLMediaElement
> > b) Remove the current invocation of these EME methods in WebMediaPlayer side
> > c) Then remove these methods from HTMLMediaElement and WebMediaPlayerClient
> > d) Also i need to remove the existing createMediaPlayer method from chromium
> > side (render_frame_impl) first and finally i will remove it from blink side
> > (WebFrameClient)
> > 
> > Please feel free to question me on the above steps if i have overlooked
> > something.
> 
> If you're not yet entirely sure of how everything will fit together, you could
> try to write the change as one step without regard for the Chromium-Blink
split,
> then figure out how to actually land it in 2 or 3 steps. In any event, it
would
> be easier to review the whole change as a unit, regardless or how it lands.

ok, i will make the complete patch first and upload the changes (both blink and
chromium), after the review i will split it as required for landing.

[Srirama, 2015-05-08 14:04:54]

On 2015/05/08 13:58:08, Srirama wrote:
> On 2015/05/08 12:49:41, philipj_UTC2 wrote:
> > On 2015/05/08 11:56:56, Srirama wrote:
> > > On 2015/05/08 11:23:35, philipj_UTC2 wrote:
> > > > But this is also a Blink CL, is it part of a 3-sided patch where this is
> the
> > > > first and there's a Chromium CL to go in before
> > > > https://codereview.chromium.org/1055503002/ ?
> > > 
> > > Sorry for the confusion, it is a 3-sided patch. Let me explain the
complete
> > > steps.
> > > 
> > > To avoid EME bits from HTMLMediaElement, i am going to pass one more
client
> > > object (WebMediaPlayerEncryptedmedia implemented by
> > > HTMLMediaElementEncryptedMedia) to createMediaPlayer function in
> > > WebFrameClient.h which is implemented in chromium
> side(render_frame_impl.cc).
> > So
> > > i am planning to do in the following way.
> > > 
> > > 1) Add new method createWebMediaPlayer to accept new parameter so that i
> need
> > > not disturb the existing createMediaPlayer method. Also add the new
> interface
> > > WebMediaPlayerEncryptedMediaClient which is referenced in WebFrameClient.
> > > 2) Implement the new method createWebMediaPlayer on chromium side
> > > render_frame_impl and pass this client to webMediaPlayer_impl and
> > > webmediaplayer_android and invoke the EME methods through this client.
> > 
> > Is there any reason that the new method can't also be called
createMediaPlayer
> > just with different arguments? That would be more in line with the other
> create*
> > methods in WebFrameClient.h
> > 
> > > 3) Now do the blink side work to implement WebMediaPlayerClient in
> > > HTMLMediaElement and WebMediaPlayerEncryptedMediaClient in
> > > HTMLMediaElementEncyptedMedia and remove the MediaPlayer and
> MediaPlayerClient
> > > interfaces, Which is being done in
> > https://codereview.chromium.org/1055503002/.
> > > Thought there are again some more steps here in addition to this, as i
> cannot
> > do
> > > them as part of this blink CL
(https://codereview.chromium.org/1055503002/).
> > > a) Keep empty implementations for EME methods in HTMLMediaElement
> > > b) Remove the current invocation of these EME methods in WebMediaPlayer
side
> > > c) Then remove these methods from HTMLMediaElement and
WebMediaPlayerClient
> > > d) Also i need to remove the existing createMediaPlayer method from
chromium
> > > side (render_frame_impl) first and finally i will remove it from blink
side
> > > (WebFrameClient)
> > > 
> > > Please feel free to question me on the above steps if i have overlooked
> > > something.
> > 
> > If you're not yet entirely sure of how everything will fit together, you
could
> > try to write the change as one step without regard for the Chromium-Blink
> split,
> > then figure out how to actually land it in 2 or 3 steps. In any event, it
> would
> > be easier to review the whole change as a unit, regardless or how it lands.
> 
> ok, i will make the complete patch first and upload the changes (both blink
and
> chromium), after the review i will split it as required for landing.

And regarding the method name, there is already a overloaded function in
render_frame_impl which is giving me error, so temporarily i kept different name
which i thought will change it back later. It will not be an issue now as i will
make the direct patch.

[Srirama, 2015-05-29 14:41:40]

Patchset #2 (id:40001) has been deleted

[Srirama, 2015-05-29 14:50:37]

Patchset #2 (id:60001) has been deleted

[Srirama, 2015-05-29 16:44:29]

Implemented WebMediaPlayerEncryptedMediaClient in WebMediaPlayerClient and added
new createMediaPlayer() method to accept new interface client.
PTAL

[ddorwin, 2015-05-29 19:53:14]

https://codereview.chromium.org/1125353004/diff/80001/public/platform/WebMedi...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/1125353004/diff/80001/public/platform/WebMedi...
public/platform/WebMediaPlayerClient.h:34: #include "WebEncryptedMediaTypes.h"
Why is this necessary?

[Srirama, 2015-05-29 20:06:26]

https://codereview.chromium.org/1125353004/diff/80001/public/platform/WebMedi...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/1125353004/diff/80001/public/platform/WebMedi...
public/platform/WebMediaPlayerClient.h:34: #include "WebEncryptedMediaTypes.h"
On 2015/05/29 19:53:14, ddorwin wrote:
> Why is this necessary?

Not required, i will remove it.

[Srirama, 2015-06-01 04:57:24]

Patchset #3 (id:100001) has been deleted

[Srirama, 2015-06-01 06:18:34]

Cleaned up the unused code.
PTAL

https://codereview.chromium.org/1125353004/diff/80001/public/platform/WebMedi...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/1125353004/diff/80001/public/platform/WebMedi...
public/platform/WebMediaPlayerClient.h:34: #include "WebEncryptedMediaTypes.h"
On 2015/05/29 20:06:25, Srirama wrote:
> On 2015/05/29 19:53:14, ddorwin wrote:
> > Why is this necessary?
> 
> Not required, i will remove it.

Done.

[ddorwin, 2015-06-04 20:17:07]

This LG to me with comment. We should probably make sure reviewers are happy
with the other CLs before landing, though.

https://codereview.chromium.org/1125353004/diff/120001/public/platform/WebMed...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/1125353004/diff/120001/public/platform/WebMed...
public/platform/WebMediaPlayerClient.h:43: class WebMediaPlayerClient : public
WebMediaPlayerEncryptedMediaClient {
Add a TODO to remove this inheritance when <something (bug is fixed?)>.

[Srirama, 2015-06-05 04:59:56]

On 2015/06/04 20:17:07, ddorwin wrote:
> This LG to me with comment. We should probably make sure reviewers are happy
> with the other CLs before landing, though.
> 
>
https://codereview.chromium.org/1125353004/diff/120001/public/platform/WebMed...
> File public/platform/WebMediaPlayerClient.h (right):
> 
>
https://codereview.chromium.org/1125353004/diff/120001/public/platform/WebMed...
> public/platform/WebMediaPlayerClient.h:43: class WebMediaPlayerClient : public
> WebMediaPlayerEncryptedMediaClient {
> Add a TODO to remove this inheritance when <something (bug is fixed?)>.

Thanks ddorwin, i will add the TODO comment here and start working on other CLs.

[ddorwin, 2015-07-09 18:37:50]

https://codereview.chromium.org/1125353004/diff/140001/public/platform/WebMed...
File public/platform/WebMediaPlayerEncryptedMediaClient.h (right):

https://codereview.chromium.org/1125353004/diff/140001/public/platform/WebMed...
public/platform/WebMediaPlayerEncryptedMediaClient.h:2: * Copyright (C) 2009
Google Inc. All rights reserved.
2012

And any other changes from https://codereview.chromium.org/1055503002/.

https://codereview.chromium.org/1125353004/diff/140001/public/web/WebFrameCli...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1125353004/diff/140001/public/web/WebFrameCli...
public/web/WebFrameClient.h:111: // TODO(srirama): Remove this method once blink
updated.
blink or Chromium?

[Srirama, 2015-07-10 15:17:44]

PTAL.

https://codereview.chromium.org/1125353004/diff/140001/public/platform/WebMed...
File public/platform/WebMediaPlayerEncryptedMediaClient.h (right):

https://codereview.chromium.org/1125353004/diff/140001/public/platform/WebMed...
public/platform/WebMediaPlayerEncryptedMediaClient.h:2: * Copyright (C) 2009
Google Inc. All rights reserved.
On 2015/07/09 18:37:50, ddorwin wrote:
> 2012
> 
> And any other changes from https://codereview.chromium.org/1055503002/.

Done. No other changes were present.

https://codereview.chromium.org/1125353004/diff/140001/public/web/WebFrameCli...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1125353004/diff/140001/public/web/WebFrameCli...
public/web/WebFrameClient.h:111: // TODO(srirama): Remove this method once blink
updated.
On 2015/07/09 18:37:50, ddorwin wrote:
> blink or Chromium?

Done.

[ddorwin, 2015-07-10 16:35:23]

lgtm

[Srirama, 2015-07-10 17:01:17]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2015-07-10 17:01:45]

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

[commit-bot: I haz the power, 2015-07-10 17:08:09]

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

[commit-bot: I haz the power, 2015-07-10 17:08:10]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[Srirama, 2015-07-10 17:16:54]

On 2015/07/10 17:08:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

sorry, my mistake.
@philip, owners review please.

[philipj_slow, 2015-07-10 18:13:26]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-07-10 18:13:27]

lgtm

[commit-bot: I haz the power, 2015-07-10 18:13:44]

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

[commit-bot: I haz the power, 2015-07-10 18:17:38]

Committed patchset #5 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198701