Eliminate MediaPlayer & MediaPlayerClient abstractions

Eliminate MediaPlayer & MediaPlayerClient abstractions

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

[Srirama, 2014-05-19 12:57:01]

As suggested by Acolwell I have started working on BUG 350571 instead of 100915.
As part of this I have started with cleanup of method player() in
HTMLVideoElement.cpp.
I have replaced it with webMediaPlayer() in most of the places where it is
straightforward.

PTAL

[acolwell GONE FROM CHROMIUM, 2014-05-19 16:55:31]

Please remove the corresponding declarations from MediaPlayer and remove the
implementation of those methods from WebMediaPlayerClientImpl as well.

[acolwell GONE FROM CHROMIUM, 2014-05-19 16:57:53]

https://codereview.chromium.org/291873002/diff/1/Source/core/html/HTMLVideoEl...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/291873002/diff/1/Source/core/html/HTMLVideoEl...
Source/core/html/HTMLVideoElement.cpp:212: return webMediaPlayer()->hasVideo()
&& static_cast<MediaPlayer::ReadyState>(webMediaPlayer()->readyState()) >=
MediaPlayer::HaveCurrentData;
MediaPlayer is going away so this should be changed to use
WebMediaPlayer::ReadyStateHaveCurrentData instead of casting to the MediaPlayer
type.

[Srirama, 2014-05-20 07:44:37]

Removed unused methods as much as possible and cleaned up the code as suggested.
I will take care of the remaining methods from next patch.

PTAL.

[Srirama, 2014-05-20 13:17:14]

Removed didPassCORSAccessCheck as well which is not used now.
PTAL.

[acolwell GONE FROM CHROMIUM, 2014-05-20 16:40:20]

looks pretty good. Just a few comments

https://codereview.chromium.org/291873002/diff/40001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/291873002/diff/40001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:46: using blink::WebMediaPlayer;
nit: Please just add blink:: to the one usage below.

https://codereview.chromium.org/291873002/diff/40001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:140: IntSize playerSize =
webMediaPlayer()->naturalSize();
nit: Just inline this below since nothing else uses this temporary variable.

https://codereview.chromium.org/291873002/diff/40001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:149: return playerSize.height();
ditto

https://codereview.chromium.org/291873002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderVideo.cpp (right):

https://codereview.chromium.org/291873002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderVideo.cpp:43: using blink::WebMediaPlayer;
nit: Just use blink:: at the one location where you reference WebMediaPlayer.
There is no need for a using here.

https://codereview.chromium.org/291873002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderVideo.cpp:103: LayoutSize size =
static_cast<IntSize>(webMediaPlayer->naturalSize());
nit: If you make size an IntSize instead of a LayoutSize, does the need for a
cast go away?

https://codereview.chromium.org/291873002/diff/40001/Source/modules/imagebitm...
File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):

https://codereview.chromium.org/291873002/diff/40001/Source/modules/imagebitm...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:54: using
blink::WebMediaPlayer;
nit: no need for a using since you only reference WebMediaPlayer in one place.
Just add blink:: at that one usage.

https://codereview.chromium.org/291873002/diff/40001/Source/modules/mediasour...
File Source/modules/mediasource/HTMLVideoElementMediaSource.cpp (right):

https://codereview.chromium.org/291873002/diff/40001/Source/modules/mediasour...
Source/modules/mediasource/HTMLVideoElementMediaSource.cpp:39: using
blink::WebMediaPlayer;
nit: No need for using since there is only one instance of WebMediaPlayer here.

https://codereview.chromium.org/291873002/diff/40001/Source/platform/graphics...
File Source/platform/graphics/media/MediaPlayer.h (right):

https://codereview.chromium.org/291873002/diff/40001/Source/platform/graphics...
Source/platform/graphics/media/MediaPlayer.h:48: class IntSize;
I think you can remove this now

https://codereview.chromium.org/291873002/diff/40001/Source/platform/graphics...
Source/platform/graphics/media/MediaPlayer.h:121: virtual bool hasAudio() const
= 0;
Please remove this as well so that both hasXXX methods are removed.

https://codereview.chromium.org/291873002/diff/40001/Source/platform/graphics...
Source/platform/graphics/media/MediaPlayer.h:166: virtual unsigned
videoDecodedByteCount() const = 0;
Please remove these two as well so that all the stats methods are now removed.

[Srirama, 2014-05-21 03:21:56]

Updated according to the review comments.
PTAL

[Srirama, 2014-05-21 03:40:21]

Forgot to remove declarations of stats methods. Its done now.
PTAL

[acolwell GONE FROM CHROMIUM, 2014-05-22 01:06:06]

lgtm % comments

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:129: if (!webMediaPlayer())
Doesn't this cause a subtle script visible change in behavior right now since we
can have a player() but no webMediaPlayer()?

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:139: return
static_cast<IntSize>(webMediaPlayer()->naturalSize()).width();
Why do you need the cast to IntSize here?

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:146: return
static_cast<IntSize>(webMediaPlayer()->naturalSize()).height();
Why do you need the cast to IntSize here?

[Srirama, 2014-05-22 06:06:03]

PTAL

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:129: if (!webMediaPlayer())
You are right, there will be a difference if we access this function between
creation of player() and webmediaplayer(). I will use player() till we change
the webmediaplayer() creation logic.

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:139: return
static_cast<IntSize>(webMediaPlayer()->naturalSize()).width();
This gives me error without cast, because webMediaPlayer()->naturalSize()
returns blink::WebSize which doesn't have width, height utility functions.

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:146: return
static_cast<IntSize>(webMediaPlayer()->naturalSize()).height();
ditto

[Srirama, 2014-05-22 06:35:18]

PTAL

[acolwell GONE FROM CHROMIUM, 2014-05-22 14:47:45]

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:139: return
static_cast<IntSize>(webMediaPlayer()->naturalSize()).width();
On 2014/05/22 06:06:04, Srirama wrote:
> This gives me error without cast, because webMediaPlayer()->naturalSize()
> returns blink::WebSize which doesn't have width, height utility functions.

WebSize is a struct with public width & height members. You should be able to
directly access the members.

[Srirama, 2014-05-22 15:06:32]

On 2014/05/22 14:47:45, acolwell wrote:
>
https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
> File Source/core/html/HTMLVideoElement.cpp (right):
> 
>
https://codereview.chromium.org/291873002/diff/80001/Source/core/html/HTMLVid...
> Source/core/html/HTMLVideoElement.cpp:139: return
> static_cast<IntSize>(webMediaPlayer()->naturalSize()).width();
> On 2014/05/22 06:06:04, Srirama wrote:
> > This gives me error without cast, because webMediaPlayer()->naturalSize()
> > returns blink::WebSize which doesn't have width, height utility functions.
> 
> WebSize is a struct with public width & height members. You should be able to
> directly access the members.

Thanks, I got it now. I will use the member variables directly.

[Srirama, 2014-05-23 03:50:03]

Used width and height members to avoid unnecessary casting as suggested.

[Srirama, 2014-05-23 03:50:17]

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

[commit-bot: I haz the power, 2014-05-23 03:50:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/srirama.m@samsung.com/291873002/120001

[commit-bot: I haz the power, 2014-05-23 05:08:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6065)

[commit-bot: I haz the power, 2014-05-23 05:22:55]

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

[commit-bot: I haz the power, 2014-05-23 05:22:55]

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

[Srirama, 2014-05-23 09:33:18]

abarth@chromium.org: Please review changes in 

web/ platform/ and modules/

[Srirama, 2014-05-26 13:20:49]

On 2014/05/23 09:33:18, Srirama wrote:
> mailto:abarth@chromium.org: Please review changes in 
> 
> web/ platform/ and modules/

Ping

[abarth-chromium, 2014-05-26 19:25:34]

lgtm

https://codereview.chromium.org/291873002/diff/120001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/291873002/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:1829: return webMediaPlayer() ?
webMediaPlayer()->hasAudio() : false;
return webMediaPlayer() && webMediaPlayer()->hasAudio();

[Srirama, 2014-05-27 06:19:21]

Thanks, updated as per the comment.

[Srirama, 2014-05-27 06:21:21]

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

[commit-bot: I haz the power, 2014-05-27 06:21:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/srirama.m@samsung.com/291873002/140001

[commit-bot: I haz the power, 2014-05-27 07:39:19]

Change committed as 174841