HTMLMediaElement shouldn't call currentTime() when we HAVE_NOTHING.

Tighten up preconditions for calling WebMediaPlayer::currentTime().

HTMLMediaElement shouldn't call WebMediaPlayer::currentTime() when
we HAVE_NOTHING. Doing so leaks spec implementation details into
lower levels of the stack.

Also remove some old crufty time-related code.

BUG=349543, 370634
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181329

[scherkus (not reviewing), 2014-09-02 23:42:33]

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

[acolwell GONE FROM CHROMIUM, 2014-09-03 00:21:42]

Just a few minor comments.

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

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:795: refreshCachedTime();
I think you should be able to remove this call now.

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2053: m_cachedTime =
webMediaPlayer()->currentTime();
nit: Add a comment indicating that this only contains a valid value when
m_readyState >= HAVE_METADATA?

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2054: m_cachedTimeWallClockUpdateTime =
WTF::currentTime();
Mind removing this while you are in here? The value appears to never be read.

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2066:
m_minimumWallClockTimeToCacheMediaTime = WTF::currentTime() +
minimumTimePlayingBeforeCacheSnapshot;
ditto

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2074: static const double
minCachedDeltaForWarning = 0.01;
move this into the block below so we only have 1 #ifdef or remove the code
altogether?

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3419: updateMediaController();
Add invalidateCachedTime() call here just to be consistent?

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

Patchset #2 (id:20001) has been deleted

[scherkus (not reviewing), 2014-09-03 03:27:23]

Patchset #2 (id:40001) has been deleted

[scherkus (not reviewing), 2014-09-03 03:27:45]

PTAL

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

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:795: refreshCachedTime();
On 2014/09/03 00:21:42, acolwell wrote:
> I think you should be able to remove this call now.

Done ... it's tough to figure out the original intent of this code but I believe
my updated FIXME is accurate. LMKWYT.

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2053: m_cachedTime =
webMediaPlayer()->currentTime();
On 2014/09/03 00:21:42, acolwell wrote:
> nit: Add a comment indicating that this only contains a valid value when
> m_readyState >= HAVE_METADATA?

Done in .h

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2054: m_cachedTimeWallClockUpdateTime =
WTF::currentTime();
On 2014/09/03 00:21:42, acolwell wrote:
> Mind removing this while you are in here? The value appears to never be read.

Done.

I actually think we can nuke all this cached time business as I don't think it
affects Chromium's media engines, but that can be a future CL.

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2066:
m_minimumWallClockTimeToCacheMediaTime = WTF::currentTime() +
minimumTimePlayingBeforeCacheSnapshot;
On 2014/09/03 00:21:42, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2074: static const double
minCachedDeltaForWarning = 0.01;
On 2014/09/03 00:21:42, acolwell wrote:
> move this into the block below so we only have 1 #ifdef or remove the code
> altogether?

Done.

https://codereview.chromium.org/535703002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3419: updateMediaController();
On 2014/09/03 00:21:42, acolwell wrote:
> Add invalidateCachedTime() call here just to be consistent?

Done.

[philipj_slow, 2014-09-03 09:06:06]

LGTM, but can you qualify the commit message to say
WebMediaPlayer::currentTime()? I was expecting HTMLMediaElement::currentTime()
but that wasn't the case.

[philipj_slow, 2014-09-03 09:11:00]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2014-09-03 09:11:01]

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

https://codereview.chromium.org/535703002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2046: if (!webMediaPlayer() ||
m_readyState < HAVE_METADATA)
This reminds me of http://crbug.com/382721 which was intended to guarantee that
m_readyState >= HAVE_METADATA implies webMediaPlayer(), i.e. !webMediaPlayer()
implies m_readyState < HAVE_METADATA.

Is the "m_readyState < HAVE_METADATA" still reachable, or could it simply be
asserted?

[scherkus (not reviewing), 2014-09-03 17:03:45]

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

https://codereview.chromium.org/535703002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2046: if (!webMediaPlayer() ||
m_readyState < HAVE_METADATA)
On 2014/09/03 09:11:00, philipj wrote:
> This reminds me of http://crbug.com/382721 which was intended to guarantee
that
> m_readyState >= HAVE_METADATA implies webMediaPlayer(), i.e. !webMediaPlayer()
> implies m_readyState < HAVE_METADATA.
> 
> Is the "m_readyState < HAVE_METADATA" still reachable, or could it simply be
> asserted?

While m_readyState >= HAVE_METADATA certainly implies webMediaPlayer(), the
converse isn't necessarily true: we can still HAVE_NOTHING and be
NETWORK_LOADING as we're currently trying to fetch the initial data to advance
to the HAVE_METADATA state

As a side, it'd be nice to tighten up the WebMediaPlayer contract to imply that
that they start in the HAVE_NOTHING state so that implementations don't have to
do stuff like:
https://code.google.com/p/chromium/codesearch#search/&q=SetReadyState%5C(WebM...

[scherkus (not reviewing), 2014-09-03 17:04:00]

On 2014/09/03 09:06:06, philipj wrote:
> LGTM, but can you qualify the commit message to say
> WebMediaPlayer::currentTime()? I was expecting HTMLMediaElement::currentTime()
> but that wasn't the case.

Ah good call! CL description updated.

[scherkus (not reviewing), 2014-09-03 17:06:10]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-03 17:06:45]

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

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

Committed patchset #2 (id:60001) as 181329