Eliminate MediaPlayer abstraction(seek, duration, poster APIs)

Eliminate MediaPlayer abstraction(seek, duration, poster APIs)

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

[Srirama, 2014-07-11 15:03:40]

Removed duration, currentTime, seek, seeking and setPoster APIs from
WebMediaPlayerClientImpl.
PTAL.

[philipj_slow, 2014-07-13 22:12:49]

Looks good, but I'd like acolwell to take a look as well.

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

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1982: webMediaPlayer()->seek(time);
I missed in this in the previous review, but the m_player check in this function
doesn't guarantee that webMediaPlayer() is not null, or at least not obviously
so. What makes it safe?

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2049: if (!m_player || !webMediaPlayer())
!m_player implies !webMediaPlayer(), so just the latter check here is enough.
m_player is made unused in this function anyway.

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2098: return webMediaPlayer()->duration();
Change the !m_player check at the top to match?

[Srirama, 2014-07-14 07:50:54]

Updated as per comments.
PTAL

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

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1982: webMediaPlayer()->seek(time);
The check at the top if (m_readyState == HAVE_NOTHING) return; ensures that
webmediaplayer is not null(this is how we arrived at the bug 382721 and solved
before landing these patches).
On 2014/07/13 22:12:49, philipj wrote:
> I missed in this in the previous review, but the m_player check in this
function
> doesn't guarantee that webMediaPlayer() is not null, or at least not obviously
> so. What makes it safe?

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2049: if (!m_player || !webMediaPlayer())
On 2014/07/13 22:12:48, philipj wrote:
> !m_player implies !webMediaPlayer(), so just the latter check here is enough.
> m_player is made unused in this function anyway.

Done.

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2098: return webMediaPlayer()->duration();
Removed !m_player check but !webmediaplayer() check is not requried as the other
check (m_readyState < HAVE_METADATA) makes sure that webmediaplayer is not null.
On 2014/07/13 22:12:48, philipj wrote:
> Change the !m_player check at the top to match?

[philipj_slow, 2014-07-14 10:54:57]

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

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1982: webMediaPlayer()->seek(time);
On 2014/07/14 07:50:54, Srirama wrote:
> The check at the top if (m_readyState == HAVE_NOTHING) return; ensures that
> webmediaplayer is not null(this is how we arrived at the bug 382721 and solved
> before landing these patches).

OK. So since m_readyState >= HAVE_METADATA now implies that webMediaPlayer() is
not null, the check at the top is confusing. The !m_player part of the
expression never matters and can be removed.

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2098: return webMediaPlayer()->duration();
On 2014/07/14 07:50:54, Srirama wrote:
> Removed !m_player check but !webmediaplayer() check is not requried as the
other
> check (m_readyState < HAVE_METADATA) makes sure that webmediaplayer is not
null.

The check at the top is equivalent to the one in seek, so maybe simplify it in
the same way? Both of these can be done in a separate CL if you think they'd
pollute this one, though.

[philipj_slow, 2014-07-14 11:06:56]

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

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2098: return webMediaPlayer()->duration();
On 2014/07/14 10:54:57, philipj wrote:
> On 2014/07/14 07:50:54, Srirama wrote:
> > Removed !m_player check but !webmediaplayer() check is not requried as the
> other
> > check (m_readyState < HAVE_METADATA) makes sure that webmediaplayer is not
> null.
> 
> The check at the top is equivalent to the one in seek, so maybe simplify it in
> the same way? Both of these can be done in a separate CL if you think they'd
> pollute this one, though.

My bad, you already fixed this one.

[Srirama, 2014-07-14 12:18:21]

Done.
PTAL

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

https://codereview.chromium.org/388803005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1982: webMediaPlayer()->seek(time);
On 2014/07/14 10:54:57, philipj wrote:
> On 2014/07/14 07:50:54, Srirama wrote:
> > The check at the top if (m_readyState == HAVE_NOTHING) return; ensures that
> > webmediaplayer is not null(this is how we arrived at the bug 382721 and
solved
> > before landing these patches).
> 
> OK. So since m_readyState >= HAVE_METADATA now implies that webMediaPlayer()
is
> not null, the check at the top is confusing. The !m_player part of the
> expression never matters and can be removed.

Done.

[philipj_slow, 2014-07-14 12:24:32]

LGTM, but please wait for acolwell's review before landing.

[acolwell GONE FROM CHROMIUM, 2014-07-14 18:58:08]

lgtm

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

https://codereview.chromium.org/388803005/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2051: if (!webMediaPlayer())
Should this really be in terms of webMediaPlayer()? ISTM that this should
actually be m_readyState == HAVE_NOTHING or m_readyState < HAVE_METADATA.

This roughly maps to the "official playback position" and "default playback
start position" parts of the spec eventhough we don't properly support those
concepts in the code yet.

[Srirama, 2014-07-15 01:05:50]

abarth@: owners review for web/ and platform/ please

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

https://codereview.chromium.org/388803005/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2051: if (!webMediaPlayer())
On 2014/07/14 18:58:07, acolwell wrote:
> Should this really be in terms of webMediaPlayer()? ISTM that this should
> actually be m_readyState == HAVE_NOTHING or m_readyState < HAVE_METADATA.
> 
> This roughly maps to the "official playback position" and "default playback
> start position" parts of the spec eventhough we don't properly support those
> concepts in the code yet.

Done.

[abarth-chromium, 2014-07-15 01:23:40]

lgtm

[commit-bot: I haz the power, 2014-07-15 01:24:01]

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

[commit-bot: I haz the power, 2014-07-15 02:06:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15444)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16887)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/28554)

[commit-bot: I haz the power, 2014-07-15 03:09:17]

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

[commit-bot: I haz the power, 2014-07-15 03:09:18]

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

[Srirama, 2014-07-15 04:21:50]

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

[commit-bot: I haz the power, 2014-07-15 04:22:33]

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

[commit-bot: I haz the power, 2014-07-15 04:23:22]

Change committed as 178135