Eliminate MediaPlayer & MediaPlayerClient abstractions(play/pause, other APIs)

Eliminate MediaPlayer & MediaPlayerClient abstractions(play/pause, other APIs)

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

[Srirama, 2014-05-27 14:11:24]

Removed play, pause, paused, supportsSave, hasSingleSecurityOrigin and
mediaTimeForTimeValue from WebMediaPlayerClientImpl.

PTAL.

[Srirama, 2014-05-27 14:16:29]

abarth@: Owners review for web/ and platform/ changes please.

[philipj_slow, 2014-05-27 20:54:56]

A few comments, I'll leave the final say to acolwell.

https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1784: double mediaTime = webMediaPlayer()
? webMediaPlayer()->mediaTimeForTimeValue(time) : time;
This ends up looking a bit weird given the following comparison, maybe rewrite
as
    if (webMediaPlayer()) {
        double mediaTime = webMediaPlayer()->mediaTimeForTimeValue(time);
        if (time != mediaTime) {
            WTF_LOG(Media, "HTMLMediaElement::seek(%f) - media timeline
equivalent is %f", time, mediaTime);
            time = mediaTime;
        }
    }
?

https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:237: bool hasSingleSecurityOrigin() const {
return webMediaPlayer() && webMediaPlayer()->hasSingleSecurityOrigin(); }
Did you deliberately change the return value when the player is null? Because of
how WebGLRenderingContextBase::validateHTMLVideoElement is implemented it should
be impossible to hit that case here, so I think it's OK.

[Srirama, 2014-05-28 13:42:07]

Fixed review comments

[Srirama, 2014-05-28 13:46:29]

On 2014/05/27 20:54:56, philipj wrote:
> A few comments, I'll leave the final say to acolwell.
> 
>
https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
> Source/core/html/HTMLMediaElement.cpp:1784: double mediaTime =
webMediaPlayer()
> ? webMediaPlayer()->mediaTimeForTimeValue(time) : time;
> This ends up looking a bit weird given the following comparison, maybe rewrite
> as
>     if (webMediaPlayer()) {
>         double mediaTime = webMediaPlayer()->mediaTimeForTimeValue(time);
>         if (time != mediaTime) {
>             WTF_LOG(Media, "HTMLMediaElement::seek(%f) - media timeline
> equivalent is %f", time, mediaTime);
>             time = mediaTime;
>         }
>     }
> ?
> 
Yes, this makes sense, Done.

>
https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/298093004/diff/1/Source/core/html/HTMLMediaEl...
> Source/core/html/HTMLMediaElement.h:237: bool hasSingleSecurityOrigin() const
{
> return webMediaPlayer() && webMediaPlayer()->hasSingleSecurityOrigin(); }
> Did you deliberately change the return value when the player is null? Because
of
> how WebGLRenderingContextBase::validateHTMLVideoElement is implemented it
should
> be impossible to hit that case here, so I think it's OK.

Yes, i assumed that case will be unlikely and changed it.

[philipj_slow, 2014-05-28 15:01:59]

Thanks! No further comments from me, Aaron will be back next week to say yay or
nay.

[acolwell GONE FROM CHROMIUM, 2014-06-02 22:24:25]

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1783: if (webMediaPlayer()) {
Do you really need this? I don't believe it is possible for m_readyState to be >
HAVE_NOTHING and webMediaPlayer() return a null pointer. If so, I think that
might be a bug.

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2836: if (webMediaPlayer() &&
webMediaPlayer()->paused())
Remove pointer check. The call to this method comes from the WebMediaPlayer
instance. webMediaPlayer() should never return a null pointer here.

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3033: if (webMediaPlayer())
Is this really necessary? I believe the conditions in potentiallyPlaying() and
playerPaused equalling true ensure that webMediaPlayer() is non-null at this
point.

[Srirama, 2014-06-03 05:48:00]

Fixed review comments.
PTAL.

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1783: if (webMediaPlayer()) {
On 2014/06/02 22:24:26, acolwell wrote:
> Do you really need this? I don't believe it is possible for m_readyState to be
>
> HAVE_NOTHING and webMediaPlayer() return a null pointer. If so, I think that
> might be a bug.

Done.

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2836: if (webMediaPlayer() &&
webMediaPlayer()->paused())
On 2014/06/02 22:24:26, acolwell wrote:
> Remove pointer check. The call to this method comes from the WebMediaPlayer
> instance. webMediaPlayer() should never return a null pointer here.

Done.

https://codereview.chromium.org/298093004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3033: if (webMediaPlayer())
On 2014/06/02 22:24:26, acolwell wrote:
> Is this really necessary? I believe the conditions in potentiallyPlaying() and
> playerPaused equalling true ensure that webMediaPlayer() is non-null at this
> point.

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-03 17:07:57]

lgtm

[Srirama, 2014-06-04 06:02:51]

adamk@: owners review for web/ and platform/ changes please

[adamk, 2014-06-04 16:56:59]

lgtm for web and platform

[Srirama, 2014-06-04 17:21:13]

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

[commit-bot: I haz the power, 2014-06-04 17:22:10]

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

[commit-bot: I haz the power, 2014-06-04 18:54:31]

Change committed as 175508