Have WebMediaPlayerImpl track ended state.

Have WebMediaPlayerImpl track ended state.

Until we can rework the guts of HTMLMediaElement and its interaction
with WebMediaPlayer, have WebMediaPlayerImpl track an ended state so it
can return the duration HTMLMediaElement expects to fire the ended
event.

This makes it easier to change how Pipeline's audio/video time is
implemented without worrying about the pecularities of HTMLMediaElement.

BUG=349543, 370634
Committed: https://crrev.com/d2c745b13c93ecff5108bed57d8e052126715492
Cr-Commit-Position: refs/heads/master@{#293263}

[scherkus (not reviewing), 2014-08-30 00:07:17]

scherkus@chromium.org changed reviewers:
+ acolwell@chromium.org

[scherkus (not reviewing), 2014-08-30 00:07:18]

assuming we're cool with this I can rip out a bunch of other silly code in
Pipeline that's tricky to maintain, such as:

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/rend...

[acolwell GONE FROM CHROMIUM, 2014-08-30 01:00:28]

https://codereview.chromium.org/526443003/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/526443003/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:451: 
nit: Add a DCHECK here to make sure you are never called in the HAVE_NOTHING
ready state since duration is invalid in that case. You should have cover from
HTMLMediaElement above, but it doesn't hurt to have this precondition explicit.

https://codereview.chromium.org/526443003/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:712: ended_ = true;
I don't think you can blindly update it here. At a minimum I think you need to
return w/o updating if seeking_ is true. Otherwise I think you can get in a
situation where the ended event and the seek request pass eachother and ended_
gets in the wrong state.

[scherkus (not reviewing), 2014-09-02 20:44:53]

Patchset #2 (id:20001) has been deleted

[scherkus (not reviewing), 2014-09-02 20:55:13]

Patchset #2 (id:40001) has been deleted

[scherkus (not reviewing), 2014-09-02 20:58:25]

https://codereview.chromium.org/526443003/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/526443003/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:451: 
On 2014/08/30 01:00:28, acolwell wrote:
> nit: Add a DCHECK here to make sure you are never called in the HAVE_NOTHING
> ready state since duration is invalid in that case. You should have cover from
> HTMLMediaElement above, but it doesn't hurt to have this precondition
explicit.

Sadly Blink does call this when we HAVE_NOTHING due to line 252 :\

#2 0x7fcf451534a6 content::WebMediaPlayerImpl::currentTime()
#3 0x7fcf41928e97 blink::HTMLMediaElement::refreshCachedTime()
#4 0x7fcf4192eb23 blink::HTMLMediaElement::updatePlayState()
#5 0x7fcf4192cd3e blink::HTMLMediaElement::setReadyState()
#6 0x7fcf4192e138 blink::HTMLMediaElement::mediaPlayerReadyStateChanged()
#7 0x7fcf4192e15c blink::HTMLMediaElement::mediaPlayerReadyStateChanged()
#8 0x7fcf3ffc56ba blink::WebMediaPlayerClientImpl::readyStateChanged()
#9 0x7fcf3ffc56dc blink::WebMediaPlayerClientImpl::readyStateChanged()
#10 0x7fcf45150201 content::WebMediaPlayerImpl::SetReadyState()
#11 0x7fcf4514fa41 content::WebMediaPlayerImpl::DoLoad()

I've replaced with a check from the spec text that returns 0 when we're in that
state.

https://codereview.chromium.org/526443003/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:712: ended_ = true;
On 2014/08/30 01:00:28, acolwell wrote:
> I don't think you can blindly update it here. At a minimum I think you need to
> return w/o updating if seeking_ is true. Otherwise I think you can get in a
> situation where the ended event and the seek request pass eachother and ended_
> gets in the wrong state.

Good catch -- done. We have a similar check in OnBufferingStateChanged().

[acolwell GONE FROM CHROMIUM, 2014-09-02 22:33:21]

lgtm % nit

https://codereview.chromium.org/526443003/diff/80001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/526443003/diff/80001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:450: if (ready_state_ ==
WebMediaPlayer::ReadyStateHaveNothing)
nit: I'd really prefer to see this logic in Blink and only have a DCHECK here.
This type of spec enforcement shouldn't be in here if we want to ever trim down
the WMP interface.

[scherkus (not reviewing), 2014-09-02 23:43:58]

https://codereview.chromium.org/526443003/diff/80001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/526443003/diff/80001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:450: if (ready_state_ ==
WebMediaPlayer::ReadyStateHaveNothing)
On 2014/09/02 22:33:21, acolwell wrote:
> nit: I'd really prefer to see this logic in Blink and only have a DCHECK here.
> This type of spec enforcement shouldn't be in here if we want to ever trim
down
> the WMP interface.

Yeah I thought about some more while driving. Agreed and made the change:
https://codereview.chromium.org/535703002

[scherkus (not reviewing), 2014-09-04 01:01:26]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-04 01:05:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/526443003/120001

[commit-bot: I haz the power, 2014-09-04 03:03:23]

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

[commit-bot: I haz the power, 2014-09-04 03:03:24]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[scherkus (not reviewing), 2014-09-04 03:26:36]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-04 03:28:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/526443003/120001

[commit-bot: I haz the power, 2014-09-04 04:17:12]

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

[commit-bot: I haz the power, 2014-09-04 04:17:13]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[scherkus (not reviewing), 2014-09-04 04:23:44]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-04 04:29:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/526443003/120001

[commit-bot: I haz the power, 2014-09-04 05:05:39]

Committed patchset #5 (id:120001) as 288f63d802cfb27f8b5c60f983a74ade00f01da3

[commit-bot: I haz the power, 2014-09-10 03:29:58]

Patchset 5 (id:??) landed as
https://crrev.com/d2c745b13c93ecff5108bed57d8e052126715492
Cr-Commit-Position: refs/heads/master@{#293263}