Eliminate MediaPlayer & MediaPlayerClient abstractions

Eliminate MediaPlayer & MediaPlayerClient abstractions

This is a chromium side patch to help eliminate MediaPlayer and
MediaPlayerClient interfaces. This helps HTMLMediaElement to
directly interact with WebMediaPlayer.

BUG=350571, 310466
Blink CL: https://codereview.chromium.org/1055503002/

Committed: https://crrev.com/26f864d0a0671225b734141d63a31a36880b9976
Cr-Commit-Position: refs/heads/master@{#338641}

[Srirama, 2015-05-19 07:36:30]

Patchset #1 (id:1) has been deleted

[Srirama, 2015-05-19 10:42:25]

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

[Srirama, 2015-05-19 10:42:25]

PTAL.

[ddorwin, 2015-05-19 17:09:19]

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

[ddorwin, 2015-05-19 17:09:20]

https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:357: virtual blink::WebMediaPlayer*
createMediaPlayer(
Because Blink is independent of Chromium, you need to support both signatures in
Chromium before landing the Blink side. (For example, the signature above is the
old one before this new one was added. It is long overdue for deletion, but feel
free to update it for your purposes.

https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:361:
blink::WebMediaPlayerEncryptedMediaClient* encrypted_client,
You need to update components/html_viewer/html_document.h too.

[Srirama, 2015-05-19 18:20:35]

https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:357: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/05/19 17:09:20, ddorwin wrote:
> Because Blink is independent of Chromium, you need to support both signatures
in
> Chromium before landing the Blink side. (For example, the signature above is
the
> old one before this new one was added. It is long overdue for deletion, but
feel
> free to update it for your purposes.

I will definitely use this. Infact i need to do a lot more work before landing
this CL and blink CL. There will be multiple patches as i have noted at
https://codereview.chromium.org/1125353004/. As per Philip's suggestion i have
uploaded the changes first so that review will be easier. If you don't mind
please check the steps i have mentioned at
https://codereview.chromium.org/1125353004/#msg7 and let me know if it is fine
or suggest me if there is a better way.

https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:361:
blink::WebMediaPlayerEncryptedMediaClient* encrypted_client,
On 2015/05/19 17:09:20, ddorwin wrote:
> You need to update components/html_viewer/html_document.h too.

Acknowledged.

[ddorwin, 2015-05-19 22:53:41]

On 2015/05/19 18:20:35, Srirama wrote:
>
https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
> File content/renderer/render_frame_impl.h (right):
> 
>
https://codereview.chromium.org/1133033003/diff/20001/content/renderer/render...
> content/renderer/render_frame_impl.h:357: virtual blink::WebMediaPlayer*
> createMediaPlayer(
> On 2015/05/19 17:09:20, ddorwin wrote:
> > Because Blink is independent of Chromium, you need to support both
signatures
> in
> > Chromium before landing the Blink side. (For example, the signature above is
> the
> > old one before this new one was added. It is long overdue for deletion, but
> feel
> > free to update it for your purposes.
> 
> I will definitely use this. Infact i need to do a lot more work before landing
> this CL and blink CL. There will be multiple patches as i have noted at
> https://codereview.chromium.org/1125353004/. As per Philip's suggestion i have
> uploaded the changes first so that review will be easier. If you don't mind
> please check the steps i have mentioned at
> https://codereview.chromium.org/1125353004/#msg7 and let me know if it is fine
> or suggest me if there is a better way.

Okay. Yes, this is going to be complex because you need to support both paths on
one of the sides. Step #2 can't switch to the new client like this CL does.

[Srirama, 2015-05-26 14:30:02]

I have modified the existing unused createMediaPlayer method in
render_frame_impl and added a new path which can be used on blink side so that
moving to the new path will be smooth without compilation issues.

[ddorwin, 2015-05-26 21:43:07]

I don't understand this approach (along with concerns in the Blink CL).

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
components/html_viewer/html_document.cc:244: return createMediaPlayer(frame,
url, client, nullptr, nullptr);
need to pass initial_cdm

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
components/html_viewer/html_document.h:86: virtual blink::WebMediaPlayer*
createMediaPlayer(
// TODO(srirama): Remove this method once blink updated.

https://codereview.chromium.org/1133033003/diff/40001/media/blink/encrypted_m...
File media/blink/encrypted_media_player_support.cc (right):

https://codereview.chromium.org/1133033003/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:119: WebMediaPlayerClient* client,
I don't understand why we're passing this in.

If you need to adapt, do it at the higher levels (i.e. with a pass-through
adapter).

https://codereview.chromium.org/1133033003/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:289: client_->keyAdded(
This would result in two events if the encrypted_client_ is passed.
Also, what's the path to eliminating client_?

https://codereview.chromium.org/1133033003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:109: base::WeakPtr<WebMediaPlayerDelegate>
delegate,
We've never had to duplicate this constructor before. You shouldn't need to do
this.

[Srirama, 2015-05-27 14:48:50]

This is what i am trying to do, i may be doing a bit more than what is needed.
Please correct/suggest a better approach if required.
The problem what i see is, to knock out WebMediaPlayerClientImpl completely,
first i need to separate the encrypted media functions out of
WebMediaPlayerClient. So i need to set up the WebMediaPlayerEncryptedMediaClient
path on chromium side before i can remove them from WebMediaPlayerClient on
blink side. While setting up WebMediaPlayerEncryptedMediaClient path i cannot
remove WebMediaPlayerClient as both actions are one different side, so at one
point both clients will be active so i have a dummy implementation on
HTMLMediaElement which will be removed once WebMediaPlayerEncryptedMediaClient
is active.

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
components/html_viewer/html_document.cc:244: return createMediaPlayer(frame,
url, client, nullptr, nullptr);
On 2015/05/26 21:43:07, ddorwin wrote:
> need to pass initial_cdm

Acknowledged.

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1133033003/diff/40001/components/html_viewer/...
components/html_viewer/html_document.h:86: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/05/26 21:43:07, ddorwin wrote:
> // TODO(srirama): Remove this method once blink updated.

Acknowledged.

https://codereview.chromium.org/1133033003/diff/40001/media/blink/encrypted_m...
File media/blink/encrypted_media_player_support.cc (right):

https://codereview.chromium.org/1133033003/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:119: WebMediaPlayerClient* client,
On 2015/05/26 21:43:07, ddorwin wrote:
> I don't understand why we're passing this in.
To move encrypted related functions from WebMediaPlayerClient to
WebMediaPlayerEncryptedMediaClient, I need to remove them from
WebMediaPlayerClient on blink side and remove their invokation here, which will
be more than one step. So i am just trying to set up the path for
WebMediaPlayerEncryptedMediaClient for these functions before i can remove them
from WebMediaPlayerClient. So currently WebMediaPlayerEncryptedMediaClient will
be NULL which will be active when i land blink CL. At this point both client_
and encrypted_client_ will invoke these functions but client invokation is dummy
as i have kept empty functions in HTMLMediaElement. Now i will remove the client
invokation here and then i will remove blink side code.
> If you need to adapt, do it at the higher levels (i.e. with a pass-through
> adapter).
Can you please elaborate a bit on what do you mean by pass-through. And whatever
problem i have explained above will be solved with pass-through approach you are
suggesting?

https://codereview.chromium.org/1133033003/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:289: client_->keyAdded(
On 2015/05/26 21:43:07, ddorwin wrote:
> This would result in two events if the encrypted_client_ is passed.
> Also, what's the path to eliminating client_?

The client_ event will be dummy as i have kept empty definitions in
HTMLMediaElement and i will eliminate this in two steps as i have explained
above. First i will remove the invokation here once encrypted_client_ is passed
and then remove them from HTMLMediaElement and WebMediaPlayerClient.

https://codereview.chromium.org/1133033003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:109: base::WeakPtr<WebMediaPlayerDelegate>
delegate,
On 2015/05/26 21:43:07, ddorwin wrote:
> We've never had to duplicate this constructor before. You shouldn't need to do
> this.

I need to setup the path for encrypted_client related functions before i can
remove them from WebMediaPlayerClient on blink side. We need both clients
because finally we need to invoke encrypted related functions alone with
"encrypted_client_" and keep other things as it is with "client_".

[Srirama, 2015-05-27 17:20:12]

I have explained one more approach in my Blink CL, probably that may address
your concerns. Please check if that is a way to avoid all unnecessary stuff that
i am doing in this CL.

[Srirama, 2015-06-09 13:50:15]

Patchset #3 (id:60001) has been deleted

[Srirama, 2015-06-09 14:26:24]

PTAL.
Done all the changes required without worrying about two approaches. Once i get
approval for this, i will start preparing the CLs for landing.

[Srirama, 2015-06-23 12:33:00]

Patchset #4 (id:100001) has been deleted

[Srirama, 2015-06-23 12:38:21]

Gentle reminder

[philipj_slow, 2015-07-01 13:52:02]

Looks like this is entirely a question of getting the EME bits right, but let me
know if there's any part that I should be taking a closer look at.

[Srirama, 2015-07-01 14:06:02]

On 2015/07/01 13:52:02, philipj wrote:
> Looks like this is entirely a question of getting the EME bits right, but let
me
> know if there's any part that I should be taking a closer look at.

You are right, this is more about EME. Lets ddorwin give his opinion.

[ddorwin, 2015-07-06 20:44:30]

LG2M once the Blink interface change dance is handled. See
https://codereview.chromium.org/1055503002/.

https://codereview.chromium.org/1133033003/diff/140001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1133033003/diff/140001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:33: #include
"third_party/WebKit/public/platform/WebMediaPlayerEncryptedMediaClient.h"
Can forward declare.

[Srirama, 2015-07-08 17:43:42]

https://codereview.chromium.org/1133033003/diff/140001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1133033003/diff/140001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:33: #include
"third_party/WebKit/public/platform/WebMediaPlayerEncryptedMediaClient.h"
On 2015/07/06 20:44:29, ddorwin wrote:
> Can forward declare.

I added here so that it will be along with WebMediaPlayerClient.h.
Anyway i will cleanup other includes also while landing.

[Srirama, 2015-07-10 15:29:30]

PTAL.
I have added path for new createMediaPlayer() method.
This should land after https://codereview.chromium.org/1125353004/

https://codereview.chromium.org/1133033003/diff/140001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1133033003/diff/140001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:33: #include
"third_party/WebKit/public/platform/WebMediaPlayerEncryptedMediaClient.h"
On 2015/07/06 20:44:29, ddorwin wrote:
> Can forward declare.

Done.

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/frame.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/frame.h:136: virtual blink::WebMediaPlayer*
createMediaPlayer(
I have checked the code and couldn't find the use for this method. So i haven't
added the 2nd method here. Correct me if i am wrong.

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/html_document.h:107: virtual blink::WebMediaPlayer*
createMediaPlayer(
I have checked the code and couldn't find the use for this method. So i haven't
added the 2nd method here. Correct me if i am wrong.

[ddorwin, 2015-07-10 17:56:57]

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/frame.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/frame.h:136: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/07/10 15:29:30, Srirama wrote:
> I have checked the code and couldn't find the use for this method. So i
haven't
> added the 2nd method here. Correct me if i am wrong.

I'm not sure what you mean. This is the WebFrameClient for the Mojo HTML Viewer
app. Blink would call this for media in HTML Viewer.
As-is, I think this would break media in the viewer.

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/html_document.h:107: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/07/10 15:29:30, Srirama wrote:
> I have checked the code and couldn't find the use for this method. So i
haven't
> added the 2nd method here. Correct me if i am wrong.

Ditto.

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:1702:
encrypted_client_->keyAdded(
Must #include the definition

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:119: #include
"third_party/WebKit/public/platform/WebMediaPlayerClient.h"
Do we need the definition in this file? It's fwd decl'd at 210 and doesn't
appear to be used.

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:710: encrypted_client_->encrypted(
Must #include the definition

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:36: class WebMediaPlayer;
This is a base class and must be #included.

[Srirama, 2015-07-10 18:36:03]

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/frame.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/frame.h:136: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/07/10 17:56:56, ddorwin wrote:
> On 2015/07/10 15:29:30, Srirama wrote:
> > I have checked the code and couldn't find the use for this method. So i
> haven't
> > added the 2nd method here. Correct me if i am wrong.
> 
> I'm not sure what you mean. This is the WebFrameClient for the Mojo HTML
Viewer
> app. Blink would call this for media in HTML Viewer.
> As-is, I think this would break media in the viewer.

I mean, when i changed this function prototype to include new parameter, i
didn't get any build error, i am verifying the components changes with "ninja -C
out/Debug mandoline:all". So i thought it is not invoked anywhere.

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/html_document.h:107: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/07/10 17:56:56, ddorwin wrote:
> On 2015/07/10 15:29:30, Srirama wrote:
> > I have checked the code and couldn't find the use for this method. So i
> haven't
> > added the 2nd method here. Correct me if i am wrong.
> 
> Ditto.

Please refer to my comment in frame.h

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:1702:
encrypted_client_->keyAdded(
On 2015/07/10 17:56:56, ddorwin wrote:
> Must #include the definition

It is indirectly included from WebMediaPlayerClient(line#210 in this file). Do
you still want me to include it, so that we won't get issues when we remove
WebMediaPlayerEncryptedMediaClient from WebMediaPlayerClient?

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:119: #include
"third_party/WebKit/public/platform/WebMediaPlayerClient.h"
On 2015/07/10 17:56:56, ddorwin wrote:
> Do we need the definition in this file? It's fwd decl'd at 210 and doesn't
> appear to be used.

I will verify and remove it.

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:710: encrypted_client_->encrypted(
On 2015/07/10 17:56:56, ddorwin wrote:
> Must #include the definition

Please refer to my comment in webmediaplayer_android.cc

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:36: class WebMediaPlayer;
On 2015/07/10 17:56:56, ddorwin wrote:
> This is a base class and must be #included.

Acknowledged.

[ddorwin, 2015-07-10 18:47:43]

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/frame.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/frame.h:136: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/07/10 18:36:02, Srirama wrote:
> On 2015/07/10 17:56:56, ddorwin wrote:
> > On 2015/07/10 15:29:30, Srirama wrote:
> > > I have checked the code and couldn't find the use for this method. So i
> > haven't
> > > added the 2nd method here. Correct me if i am wrong.
> > 
> > I'm not sure what you mean. This is the WebFrameClient for the Mojo HTML
> Viewer
> > app. Blink would call this for media in HTML Viewer.
> > As-is, I think this would break media in the viewer.
> 
> I mean, when i changed this function prototype to include new parameter, i
> didn't get any build error, i am verifying the components changes with "ninja
-C
> out/Debug mandoline:all". So i thought it is not invoked anywhere.

That will only catch compile failures. Since WebFrameClient.h has default
implementations for all functions, the compile will succeed. I expect any
media-using test would fail, though.

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:1702:
encrypted_client_->keyAdded(
On 2015/07/10 18:36:03, Srirama wrote:
> On 2015/07/10 17:56:56, ddorwin wrote:
> > Must #include the definition
> 
> It is indirectly included from WebMediaPlayerClient(line#210 in this file). Do
> you still want me to include it, so that we won't get issues when we remove
> WebMediaPlayerEncryptedMediaClient from WebMediaPlayerClient?

Yes. Also, IWYU (Include What You Use).

[Srirama, 2015-07-11 08:44:10]

Addressed all the review comments.
PTAL.

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/frame.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/frame.h:136: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/07/10 18:47:43, ddorwin wrote:
> On 2015/07/10 18:36:02, Srirama wrote:
> > On 2015/07/10 17:56:56, ddorwin wrote:
> > > On 2015/07/10 15:29:30, Srirama wrote:
> > > > I have checked the code and couldn't find the use for this method. So i
> > > haven't
> > > > added the 2nd method here. Correct me if i am wrong.
> > > 
> > > I'm not sure what you mean. This is the WebFrameClient for the Mojo HTML
> > Viewer
> > > app. Blink would call this for media in HTML Viewer.
> > > As-is, I think this would break media in the viewer.
> > 
> > I mean, when i changed this function prototype to include new parameter, i
> > didn't get any build error, i am verifying the components changes with
"ninja
> -C
> > out/Debug mandoline:all". So i thought it is not invoked anywhere.
> 
> That will only catch compile failures. Since WebFrameClient.h has default
> implementations for all functions, the compile will succeed. I expect any
> media-using test would fail, though.

Done.

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1133033003/diff/160001/components/html_viewer...
components/html_viewer/html_document.h:107: virtual blink::WebMediaPlayer*
createMediaPlayer(
On 2015/07/10 18:36:02, Srirama wrote:
> On 2015/07/10 17:56:56, ddorwin wrote:
> > On 2015/07/10 15:29:30, Srirama wrote:
> > > I have checked the code and couldn't find the use for this method. So i
> > haven't
> > > added the 2nd method here. Correct me if i am wrong.
> > 
> > Ditto.
> 
> Please refer to my comment in frame.h

Done.

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:1702:
encrypted_client_->keyAdded(
On 2015/07/10 18:47:43, ddorwin wrote:
> On 2015/07/10 18:36:03, Srirama wrote:
> > On 2015/07/10 17:56:56, ddorwin wrote:
> > > Must #include the definition
> > 
> > It is indirectly included from WebMediaPlayerClient(line#210 in this file).
Do
> > you still want me to include it, so that we won't get issues when we remove
> > WebMediaPlayerEncryptedMediaClient from WebMediaPlayerClient?
> 
> Yes. Also, IWYU (Include What You Use).

Done.

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:119: #include
"third_party/WebKit/public/platform/WebMediaPlayerClient.h"
On 2015/07/10 18:36:03, Srirama wrote:
> On 2015/07/10 17:56:56, ddorwin wrote:
> > Do we need the definition in this file? It's fwd decl'd at 210 and doesn't
> > appear to be used.
> 
> I will verify and remove it.

It is needed because we are doing implicit cast from WebMediaPlayerClient to
WebMediaPlayerEncryptedMediaClient in the first createMediaPlayer() function

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:710: encrypted_client_->encrypted(
On 2015/07/10 18:36:03, Srirama wrote:
> On 2015/07/10 17:56:56, ddorwin wrote:
> > Must #include the definition
> 
> Please refer to my comment in webmediaplayer_android.cc

Done.

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1133033003/diff/160001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:36: class WebMediaPlayer;
On 2015/07/10 18:36:03, Srirama wrote:
> On 2015/07/10 17:56:56, ddorwin wrote:
> > This is a base class and must be #included.
> 
> Acknowledged.

Done.

[ddorwin, 2015-07-13 17:58:19]

LGTM.

You'll need OWNERS review for components/html_viewer/ and content/renderer/.

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:119: #include
"third_party/WebKit/public/platform/WebMediaPlayerClient.h"
On 2015/07/11 08:44:10, Srirama wrote:
> On 2015/07/10 18:36:03, Srirama wrote:
> > On 2015/07/10 17:56:56, ddorwin wrote:
> > > Do we need the definition in this file? It's fwd decl'd at 210 and doesn't
> > > appear to be used.
> > 
> > I will verify and remove it.
> 
> It is needed because we are doing implicit cast from WebMediaPlayerClient to
> WebMediaPlayerEncryptedMediaClient in the first createMediaPlayer() function

Okay. Let's remember to remove it from these three files later.

[Srirama, 2015-07-13 18:33:01]

srirama.m@samsung.com changed reviewers:
+ jam@chromium.org, sky@chromium.org

[Srirama, 2015-07-13 18:33:03]

Thanks ddorwin.

sky@: Please review changes in components/.

jam@: Please review changes in content/renderer/.

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:119: #include
"third_party/WebKit/public/platform/WebMediaPlayerClient.h"
On 2015/07/13 17:58:19, ddorwin wrote:
> On 2015/07/11 08:44:10, Srirama wrote:
> > On 2015/07/10 18:36:03, Srirama wrote:
> > > On 2015/07/10 17:56:56, ddorwin wrote:
> > > > Do we need the definition in this file? It's fwd decl'd at 210 and
doesn't
> > > > appear to be used.
> > > 
> > > I will verify and remove it.
> > 
> > It is needed because we are doing implicit cast from WebMediaPlayerClient to
> > WebMediaPlayerEncryptedMediaClient in the first createMediaPlayer() function
> 
> Okay. Let's remember to remove it from these three files later.

Sure, I will definitely cleanup the things.

[Srirama, 2015-07-13 18:41:19]

On 2015/07/13 18:33:03, Srirama wrote:
> Thanks ddorwin.
> 
> sky@: Please review changes in components/.
> 
> jam@: Please review changes in content/renderer/.
> 
>
https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1133033003/diff/160001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:119: #include
> "third_party/WebKit/public/platform/WebMediaPlayerClient.h"
> On 2015/07/13 17:58:19, ddorwin wrote:
> > On 2015/07/11 08:44:10, Srirama wrote:
> > > On 2015/07/10 18:36:03, Srirama wrote:
> > > > On 2015/07/10 17:56:56, ddorwin wrote:
> > > > > Do we need the definition in this file? It's fwd decl'd at 210 and
> doesn't
> > > > > appear to be used.
> > > > 
> > > > I will verify and remove it.
> > > 
> > > It is needed because we are doing implicit cast from WebMediaPlayerClient
to
> > > WebMediaPlayerEncryptedMediaClient in the first createMediaPlayer()
function
> > 
> > Okay. Let's remember to remove it from these three files later.
> 
> Sure, I will definitely cleanup the things.

jam@: Looks like sky@ is OOO, can you please do onwers review for components/ as
well.

[jam, 2015-07-13 19:00:05]

lgtm

please add background to the bug or the cl description about the motivation for
removing this interface; i couldn't find this information.

[Srirama, 2015-07-14 04:30:33]

On 2015/07/13 19:00:05, jam wrote:
> lgtm
> 
> please add background to the bug or the cl description about the motivation
for
> removing this interface; i couldn't find this information.

Thanks  @jam.
Added 310466(which has more info about why we are removing this layer)in the bug
list in the commit message.

[Srirama, 2015-07-14 04:31:13]

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

[commit-bot: I haz the power, 2015-07-14 04:31:35]

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

[commit-bot: I haz the power, 2015-07-14 05:22:04]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-07-14 05:23:03]

Patchset 7 (id:??) landed as
https://crrev.com/26f864d0a0671225b734141d63a31a36880b9976
Cr-Commit-Position: refs/heads/master@{#338641}