Allow HTMLMediaElement.currentTime to be set before the transition to HAVE_METADATA

Allow HTMLMediaElement.currentTime to be set before the transition to HAVE_METADATA

Spec now allows currentTime to be set when the readyState is still HAVE_NOTHING.
This patch modifies the behaviour in accordance to the spec. The default playback
start position is set to the new currentTime and the resource plays from that point
once it is loaded.
http://www.whatwg.org/specs/web-apps/current-work/#dom-media-currenttime

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

[amogh.bihani, 2014-09-01 05:58:28]

amogh.bihani@samsung.com changed reviewers:
+ acolwell@chromium.org, philipj@opera.com

[amogh.bihani, 2014-09-01 05:58:40]

PTAL.

[philipj_slow, 2014-09-02 13:03:31]

Looks promising, thanks for working on this!

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

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1827: if (m_defaultPlaybackStartPosition >
0) {
This looks like the correct place for this logic, but per spec there's a jumped
flag that will prevent the media fragment URI handling from kicking in. We need
a test for setting currentTime to e.g. 1 on a resource with #t=2, verifying that
we end up at currentTime 1 and not 2. I don't think the existing code will do
this. Fixing that should also allow you to remove m_fragmentStartTime.

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1941: m_defaultPlaybackStartPosition =
time;
This should be moved to setCurrentTime, per "if the media element's readyState
is HAVE_NOTHING, then it must set the media element's default playback start
position to the new value"

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2086: return
m_defaultPlaybackStartPosition;
The spec says "return the media element's default playback start position,
unless that is zero" so "if (m_defaultPlaybackStartPosition) return
m_defaultPlaybackStartPosition;" would be appropriate. If that can only happen
while readyState is HAVE_NOTHING, you can assert that.

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2115:
exceptionState.throwDOMException(InvalidStateError, "There is no media
attached.");
This condition is not in the spec, why is it needed?

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

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:511: // The default playback start position
set for the player
Just "The default playback start position." (with trailing period) seems more
accurate, this is a spec concept separate from the internal player concept.

[amogh.bihani, 2014-09-02 13:39:54]

Thanks for the review :)

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

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1827: if (m_defaultPlaybackStartPosition >
0) {
On 2014/09/02 13:03:31, philipj wrote:
> This looks like the correct place for this logic, but per spec there's a
jumped
> flag that will prevent the media fragment URI handling from kicking in. We
need
> a test for setting currentTime to e.g. 1 on a resource with #t=2, verifying
that
> we end up at currentTime 1 and not 2. I don't think the existing code will do
> this. Fixing that should also allow you to remove m_fragmentStartTime.

Acknowledged.

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1941: m_defaultPlaybackStartPosition =
time;
On 2014/09/02 13:03:31, philipj wrote:
> This should be moved to setCurrentTime, per "if the media element's readyState
> is HAVE_NOTHING, then it must set the media element's default playback start
> position to the new value"

Acknowledged.

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2086: return
m_defaultPlaybackStartPosition;
On 2014/09/02 13:03:31, philipj wrote:
> The spec says "return the media element's default playback start position,
> unless that is zero" so "if (m_defaultPlaybackStartPosition) return
> m_defaultPlaybackStartPosition;" would be appropriate. If that can only happen
> while readyState is HAVE_NOTHING, you can assert that.

Ok, I'll do that. I thought that its value is set to zero in constructor, so if
it is not set to anything else, we will be returning zero.

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2115:
exceptionState.throwDOMException(InvalidStateError, "There is no media
attached.");
On 2014/09/02 13:03:31, philipj wrote:
> This condition is not in the spec, why is it needed?

Oh Sorry! I got confused with this line "Test that seeking video with no 'src'
attribute throws an INVALID_STATE_ERR exception." in the video-seek-no-src
layout test and presumed that when there is not src attribute, then
InvalidStateError has to be thrown. I'll change this.

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

https://codereview.chromium.org/517593003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:511: // The default playback start position
set for the player
On 2014/09/02 13:03:31, philipj wrote:
> Just "The default playback start position." (with trailing period) seems more
> accurate, this is a spec concept separate from the internal player concept.

Acknowledged.

[philipj_slow, 2014-09-02 15:54:48]

There are some simplifications made possible by your patch:
https://codereview.chromium.org/526413002

If you want to include those in your patch that's probably best, since otherwise
reverting may become a mess.

[amogh.bihani, 2014-09-03 13:14:33]

Thanks :)
I have merged your patch with this one and addressed the comments.

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

https://codereview.chromium.org/517593003/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1827: if (m_defaultPlaybackStartPosition >
0) {
I tested locally for fragment URI and, as you had mentioned, it wasn't working
as per the spec. So I should introduce the jumped flag with this CL or another
one?

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

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

https://codereview.chromium.org/517593003/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1827: if (m_defaultPlaybackStartPosition >
0) {
On 2014/09/03 13:14:32, amogh.bihani wrote:
> I tested locally for fragment URI and, as you had mentioned, it wasn't working
> as per the spec. So I should introduce the jumped flag with this CL or another
> one?

Whichever you prefer is OK with me, but I would like to have all changes
reviewed before landing.

[amogh.bihani, 2014-09-04 13:44:07]

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

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1983: bool noSeekRequired =
!seekableRanges->length() || (time == now && !m_defaultPlaybackStartPosition &&
displayMode() != Poster);
Because of line 1944 we get time == now and hence seek never happens. In PS1, I
had mistakenly returned m_defaultPlaybackStartPosition in line 2082, hence it
was working :P.

Sorry it took nearly the whole day to debug this and briefly go through the
Fragment URI spec, hence I could not work much on handling fragment URI. I have
created a rought patch at https://codereview.chromium.org/539103002/.
This works fine but I have one doubt in it. Please help me there.

[philipj_slow, 2014-09-04 13:53:40]

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

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1983: bool noSeekRequired =
!seekableRanges->length() || (time == now && !m_defaultPlaybackStartPosition &&
displayMode() != Poster);
On 2014/09/04 13:44:06, amogh.bihani wrote:
> Because of line 1944 we get time == now and hence seek never happens. In PS1,
I
> had mistakenly returned m_defaultPlaybackStartPosition in line 2082, hence it
> was working :P.

Ah, right, this is a bit tricky now. The spec has split the concept of current
playback position and official playback position. The currentTime() exposed to
scripts is the official playback position, but using that internally is going to
cause little issues like this.

Maybe this isn't the place, but I would like to have
[ImplementedAs=officialPlaybackPosition] attribute double currentTime in the IDL
and only officialPlaybackPosition() and currentPlaybackPosition() internally so
that each call sites makes to make an explicit choice, hopefully matching the
spec.

[amogh.bihani, 2014-09-08 10:26:38]

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

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1937: 
While working on media fragments URI it came out that when media controller has
2 resources and the second resource has #t=2 then both resources seek till t=2
but if the first resource has #t=2, then when the media controller seeks till
t=2, second resource is still at HAVE_NOTHING, because of which it does not seek
till t=2. However,if we set here
if (HAVE_NOTHING) {
  m_defaultPlaybackStartPostition = time,
  return;
}
then the other resource would come to t=2 when it reaches loadedmetadata.

I could not find anything in the spec which would suggest what is the normal
behaviour.
What should be done here?

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1983: bool noSeekRequired =
!seekableRanges->length() || (time == now && !m_defaultPlaybackStartPosition &&
displayMode() != Poster);
On 2014/09/04 13:53:40, philipj wrote:
> On 2014/09/04 13:44:06, amogh.bihani wrote:
> > Because of line 1944 we get time == now and hence seek never happens. In
PS1,
> I
> > had mistakenly returned m_defaultPlaybackStartPosition in line 2082, hence
it
> > was working :P.
> 
> Ah, right, this is a bit tricky now. The spec has split the concept of current
> playback position and official playback position. The currentTime() exposed to
> scripts is the official playback position, but using that internally is going
to
> cause little issues like this.
> 
> Maybe this isn't the place, but I would like to have
> [ImplementedAs=officialPlaybackPosition] attribute double currentTime in the
IDL
> and only officialPlaybackPosition() and currentPlaybackPosition() internally
so
> that each call sites makes to make an explicit choice, hopefully matching the
> spec.

Should I add a TODO here that this condition should be removed when official
playback position is implemented or first implement it and then land this on top
of that?

[philipj_slow, 2014-09-08 15:00:58]

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

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1937: 
On 2014/09/08 10:26:38, amogh.bihani wrote:
> While working on media fragments URI it came out that when media controller
has
> 2 resources and the second resource has #t=2 then both resources seek till t=2
> but if the first resource has #t=2, then when the media controller seeks till
> t=2, second resource is still at HAVE_NOTHING, because of which it does not
seek
> till t=2. However,if we set here
> if (HAVE_NOTHING) {
>   m_defaultPlaybackStartPostition = time,
>   return;
> }
> then the other resource would come to t=2 when it reaches loadedmetadata.
> 
> I could not find anything in the spec which would suggest what is the normal
> behaviour.
> What should be done here?

It sounds like the problem is that setting MediaController.currentTime will
invoke HTMLMediaElement.seek(), but it's the HTMLMediaElement.currentTime setter
that saves the default playback start position in the HAVE_NOTHING case? And
moving that logic to seek() "fixes" the problem?

What I don't understand is how #t=2 on one element ends up seeking both elements
attached to a MediaController. applyMediaFragmentURI() just calls seek(), and
that doesn't do anything with the MediaController. Per spec I also don't see a
connection. Please help me out :)

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1983: bool noSeekRequired =
!seekableRanges->length() || (time == now && !m_defaultPlaybackStartPosition &&
displayMode() != Poster);
On 2014/09/08 10:26:38, amogh.bihani wrote:
> On 2014/09/04 13:53:40, philipj wrote:
> > On 2014/09/04 13:44:06, amogh.bihani wrote:
> > > Because of line 1944 we get time == now and hence seek never happens. In
> PS1,
> > I
> > > had mistakenly returned m_defaultPlaybackStartPosition in line 2082, hence
> it
> > > was working :P.
> > 
> > Ah, right, this is a bit tricky now. The spec has split the concept of
current
> > playback position and official playback position. The currentTime() exposed
to
> > scripts is the official playback position, but using that internally is
going
> to
> > cause little issues like this.
> > 
> > Maybe this isn't the place, but I would like to have
> > [ImplementedAs=officialPlaybackPosition] attribute double currentTime in the
> IDL
> > and only officialPlaybackPosition() and currentPlaybackPosition() internally
> so
> > that each call sites makes to make an explicit choice, hopefully matching
the
> > spec.
> 
> Should I add a TODO here that this condition should be removed when official
> playback position is implemented or first implement it and then land this on
top
> of that?

If you want to work on official playback position that'd be nice, but it's
likely to be a bit complicated so I wouldn't block this CL on it. To fix this
problem, I would perhaps use "double now = m_cachedTime" above and add a comment
at that point saying that it's required to avoid getting the default playback
start position from currentTime().

[philipj_slow, 2014-09-08 15:03:22]

Oh, by the way, if you want to finish up this CL before everything about
https://codereview.chromium.org/539103002/ is resolved that's OK. I'm confident
the jumped flag can be implemented correctly. In this CL you can just write a
test case that verifies that the behavior is wrong, i.e. that a media fragment
start time wins over the default playback start position. The revert the pass
condition in https://codereview.chromium.org/539103002/

[amogh.bihani, 2014-09-09 05:30:52]

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

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1937: 
On 2014/09/08 15:00:58, philipj wrote:
> On 2014/09/08 10:26:38, amogh.bihani wrote:
> > While working on media fragments URI it came out that when media controller
> has
> > 2 resources and the second resource has #t=2 then both resources seek till
t=2
> > but if the first resource has #t=2, then when the media controller seeks
till
> > t=2, second resource is still at HAVE_NOTHING, because of which it does not
> seek
> > till t=2. However,if we set here
> > if (HAVE_NOTHING) {
> >   m_defaultPlaybackStartPostition = time,
> >   return;
> > }
> > then the other resource would come to t=2 when it reaches loadedmetadata.
> > 
> > I could not find anything in the spec which would suggest what is the normal
> > behaviour.
> > What should be done here?
> 
> It sounds like the problem is that setting MediaController.currentTime will
> invoke HTMLMediaElement.seek(), but it's the HTMLMediaElement.currentTime
setter
> that saves the default playback start position in the HAVE_NOTHING case? And
> moving that logic to seek() "fixes" the problem?
> 
> What I don't understand is how #t=2 on one element ends up seeking both
elements
> attached to a MediaController. applyMediaFragmentURI() just calls seek(), and
> that doesn't do anything with the MediaController. Per spec I also don't see a
> connection. Please help me out :)

Yes applyMediaFragmentURI seeks only the corresponding media but when we add
another media to the media group or MediaController then we call
addMediaElement() which internally calls bringElementUpToSpeed(). Now this calls
seek() for the new media attached. So if the first element has t=2, second will
also seek to that position (but the problem is that it is still in HAVE_NOTHING
state :/)

In step 13, it says that the media controller should seek to the media element's
initial playback position. So when second media has t=2, even the first moves to
that position.

Even in first scenario when we start the playback, the second media seeks to t=2
and then both start playing together, so I guess seeking it before hand would
look good and make both the scenarios uniform. Plus with this we won't be
fetching the resource which wont be played and save some data and power
(especially in hand held devices).

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1983: bool noSeekRequired =
!seekableRanges->length() || (time == now && !m_defaultPlaybackStartPosition &&
displayMode() != Poster);
On 2014/09/08 15:00:58, philipj wrote:
> On 2014/09/08 10:26:38, amogh.bihani wrote:
> > On 2014/09/04 13:53:40, philipj wrote:
> > > On 2014/09/04 13:44:06, amogh.bihani wrote:
> > > > Because of line 1944 we get time == now and hence seek never happens. In
> > PS1,
> > > I
> > > > had mistakenly returned m_defaultPlaybackStartPosition in line 2082,
hence
> > it
> > > > was working :P.
> > > 
> > > Ah, right, this is a bit tricky now. The spec has split the concept of
> current
> > > playback position and official playback position. The currentTime()
exposed
> to
> > > scripts is the official playback position, but using that internally is
> going
> > to
> > > cause little issues like this.
> > > 
> > > Maybe this isn't the place, but I would like to have
> > > [ImplementedAs=officialPlaybackPosition] attribute double currentTime in
the
> > IDL
> > > and only officialPlaybackPosition() and currentPlaybackPosition()
internally
> > so
> > > that each call sites makes to make an explicit choice, hopefully matching
> the
> > > spec.
> > 
> > Should I add a TODO here that this condition should be removed when official
> > playback position is implemented or first implement it and then land this on
> top
> > of that?
> 
> If you want to work on official playback position that'd be nice, but it's
> likely to be a bit complicated so I wouldn't block this CL on it. To fix this
> problem, I would perhaps use "double now = m_cachedTime" above and add a
comment
> at that point saying that it's required to avoid getting the default playback
> start position from currentTime().

Acknowledged.

[amogh.bihani, 2014-09-09 06:47:56]

Patchset #4 (id:60001) has been deleted

[philipj_slow, 2014-09-09 08:39:37]

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

https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1937: 
On 2014/09/09 05:30:52, amogh.bihani wrote:
> On 2014/09/08 15:00:58, philipj wrote:
> > On 2014/09/08 10:26:38, amogh.bihani wrote:
> > > While working on media fragments URI it came out that when media
controller
> > has
> > > 2 resources and the second resource has #t=2 then both resources seek till
> t=2
> > > but if the first resource has #t=2, then when the media controller seeks
> till
> > > t=2, second resource is still at HAVE_NOTHING, because of which it does
not
> > seek
> > > till t=2. However,if we set here
> > > if (HAVE_NOTHING) {
> > >   m_defaultPlaybackStartPostition = time,
> > >   return;
> > > }
> > > then the other resource would come to t=2 when it reaches loadedmetadata.
> > > 
> > > I could not find anything in the spec which would suggest what is the
normal
> > > behaviour.
> > > What should be done here?
> > 
> > It sounds like the problem is that setting MediaController.currentTime will
> > invoke HTMLMediaElement.seek(), but it's the HTMLMediaElement.currentTime
> setter
> > that saves the default playback start position in the HAVE_NOTHING case? And
> > moving that logic to seek() "fixes" the problem?
> > 
> > What I don't understand is how #t=2 on one element ends up seeking both
> elements
> > attached to a MediaController. applyMediaFragmentURI() just calls seek(),
and
> > that doesn't do anything with the MediaController. Per spec I also don't see
a
> > connection. Please help me out :)
> 
> Yes applyMediaFragmentURI seeks only the corresponding media but when we add
> another media to the media group or MediaController then we call
> addMediaElement() which internally calls bringElementUpToSpeed(). Now this
calls
> seek() for the new media attached. So if the first element has t=2, second
will
> also seek to that position (but the problem is that it is still in
HAVE_NOTHING
> state :/)
> 
> In step 13, it says that the media controller should seek to the media
element's
> initial playback position. So when second media has t=2, even the first moves
to
> that position.
> 
> Even in first scenario when we start the playback, the second media seeks to
t=2
> and then both start playing together, so I guess seeking it before hand would
> look good and make both the scenarios uniform. Plus with this we won't be
> fetching the resource which wont be played and save some data and power
> (especially in hand held devices).

I think the root problem is that, regardless of media fragments and the default
playback start position, attaching a media element with readyState HAVE_NOTHING
to a media controller will try to seek the media element but fail. If only later
calls play() it'll seek again, but if left paused it will stay in the
out-of-sync state.

If you want to file a spec bug about this, do so at http://whatwg.org/newbug and
post the bug number here.

Unless this CL exposes a problem that couldn't happen before, which doesn't seem
to be the case, I'm OK with leaving the problem unsolved.

[amogh.bihani, 2014-09-09 11:50:00]

Patchset #5 (id:100001) has been deleted

[amogh.bihani, 2014-09-09 13:48:09]

On 2014/09/09 08:39:37, philipj wrote:
>
https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/517593003/diff/40001/Source/core/html/HTMLMed...
> Source/core/html/HTMLMediaElement.cpp:1937: 
> On 2014/09/09 05:30:52, amogh.bihani wrote:
> > On 2014/09/08 15:00:58, philipj wrote:
> > > On 2014/09/08 10:26:38, amogh.bihani wrote:
> > > > While working on media fragments URI it came out that when media
> controller
> > > has
> > > > 2 resources and the second resource has #t=2 then both resources seek
till
> > t=2
> > > > but if the first resource has #t=2, then when the media controller seeks
> > till
> > > > t=2, second resource is still at HAVE_NOTHING, because of which it does
> not
> > > seek
> > > > till t=2. However,if we set here
> > > > if (HAVE_NOTHING) {
> > > >   m_defaultPlaybackStartPostition = time,
> > > >   return;
> > > > }
> > > > then the other resource would come to t=2 when it reaches
loadedmetadata.
> > > > 
> > > > I could not find anything in the spec which would suggest what is the
> normal
> > > > behaviour.
> > > > What should be done here?
> > > 
> > > It sounds like the problem is that setting MediaController.currentTime
will
> > > invoke HTMLMediaElement.seek(), but it's the HTMLMediaElement.currentTime
> > setter
> > > that saves the default playback start position in the HAVE_NOTHING case?
And
> > > moving that logic to seek() "fixes" the problem?
> > > 
> > > What I don't understand is how #t=2 on one element ends up seeking both
> > elements
> > > attached to a MediaController. applyMediaFragmentURI() just calls seek(),
> and
> > > that doesn't do anything with the MediaController. Per spec I also don't
see
> a
> > > connection. Please help me out :)
> > 
> > Yes applyMediaFragmentURI seeks only the corresponding media but when we add
> > another media to the media group or MediaController then we call
> > addMediaElement() which internally calls bringElementUpToSpeed(). Now this
> calls
> > seek() for the new media attached. So if the first element has t=2, second
> will
> > also seek to that position (but the problem is that it is still in
> HAVE_NOTHING
> > state :/)
> > 
> > In step 13, it says that the media controller should seek to the media
> element's
> > initial playback position. So when second media has t=2, even the first
moves
> to
> > that position.
> > 
> > Even in first scenario when we start the playback, the second media seeks to
> t=2
> > and then both start playing together, so I guess seeking it before hand
would
> > look good and make both the scenarios uniform. Plus with this we won't be
> > fetching the resource which wont be played and save some data and power
> > (especially in hand held devices).
> 
> I think the root problem is that, regardless of media fragments and the
default
> playback start position, attaching a media element with readyState
HAVE_NOTHING
> to a media controller will try to seek the media element but fail. If only
later
> calls play() it'll seek again, but if left paused it will stay in the
> out-of-sync state.
> 
> If you want to file a spec bug about this, do so at http://whatwg.org/newbug
and
> post the bug number here.
> 
> Unless this CL exposes a problem that couldn't happen before, which doesn't
seem
> to be the case, I'm OK with leaving the problem unsolved.

PTAL.

I have raised a bug for discussion at
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26764.
Please edit it if I have made any mistake.

[philipj_slow, 2014-09-09 15:27:55]

On 2014/09/09 13:48:09, amogh.bihani wrote:

> I have raised a bug for discussion at
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=26764.
> Please edit it if I have made any mistake.

It looks like you've filed a bug on W3C instead of the WHATWG. It's Ian Hickson,
the WHATWG editor, who's in the best position to read and fix the bug. I'm not
sure how to move a bug, so I'd recommend filing a new bug at
http://whatwg.org/newbug and closing the W3C one.

[amogh.bihani, 2014-09-10 03:21:17]

On 2014/09/09 15:27:55, philipj wrote:
> On 2014/09/09 13:48:09, amogh.bihani wrote:
> 
> > I have raised a bug for discussion at
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26764.
> > Please edit it if I have made any mistake.
> 
> It looks like you've filed a bug on W3C instead of the WHATWG. It's Ian
Hickson,
> the WHATWG editor, who's in the best position to read and fix the bug. I'm not
> sure how to move a bug, so I'd recommend filing a new bug at
> http://whatwg.org/newbug and closing the W3C one.

Oops! Sorry! Filed a bug in WHATWG at
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26768

[philipj_slow, 2014-09-10 09:57:06]

Looks good with some minor nits. If you fix those up I'll CQ it for you.

For new tests I prefer using testharness.js, so that it's easier to share tests
with web-platform-tests, but since you've already written the test it's OK.

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
File LayoutTests/media/video-currentTime-before-have-metadata.html (right):

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-currentTime-before-have-metadata.html:13:
video.currentTime = 1;
Add testExpected("video.currentTime", 1) after this line.

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-currentTime-before-have-metadata.html:20:
waitForEvent('seeked', function()
This can be simplified to waitForEventAndEnd("seeked").

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
File LayoutTests/media/video-seek-no-src.html (right):

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-seek-no-src.html:14: setTimeout(endTest(), 200);
I think the timeout isn't needed, there's nothing that can happen that would
cause the test to fail during the wait since there are no event listeners.

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

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:1824: m_defaultPlaybackStartPosition = 0;
Either move this out of the if clause, like the spec does, or
ASSERT(m_defaultPlaybackStartPosition == 0) afterwards, so that it's clear that
it can't be a negative number, NaN or anything like that afterwards.

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:1931: // 4.7.14.9 Seeking
Now it's "4.8.14.9 Seeking" in the spec. The section numbers change often, so I
think you can just remove this comment and the one in finishSeek().

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2010: // 11 - Set the current playback
position to the given new playback position
Can you add the missing period here?

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2013: // 12-17 are handled, if necessary,
when the engine signals a readystate change or otherwise
It's step 14-17 that's handled in finishSeek(). Can you update the numbering
there as well?

[amogh.bihani, 2014-09-10 11:18:33]

Thanks :)

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
File LayoutTests/media/video-currentTime-before-have-metadata.html (right):

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-currentTime-before-have-metadata.html:13:
video.currentTime = 1;
On 2014/09/10 09:57:05, philipj wrote:
> Add testExpected("video.currentTime", 1) after this line.

Done.

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-currentTime-before-have-metadata.html:20:
waitForEvent('seeked', function()
On 2014/09/10 09:57:05, philipj wrote:
> This can be simplified to waitForEventAndEnd("seeked").

Done.

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
File LayoutTests/media/video-seek-no-src.html (right):

https://codereview.chromium.org/517593003/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-seek-no-src.html:14: setTimeout(endTest(), 200);
On 2014/09/10 09:57:05, philipj wrote:
> I think the timeout isn't needed, there's nothing that can happen that would
> cause the test to fail during the wait since there are no event listeners.

Done.

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

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:1824: m_defaultPlaybackStartPosition = 0;
On 2014/09/10 09:57:05, philipj wrote:
> Either move this out of the if clause, like the spec does, or
> ASSERT(m_defaultPlaybackStartPosition == 0) afterwards, so that it's clear
that
> it can't be a negative number, NaN or anything like that afterwards.

Done.

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:1931: // 4.7.14.9 Seeking
On 2014/09/10 09:57:05, philipj wrote:
> Now it's "4.8.14.9 Seeking" in the spec. The section numbers change often, so
I
> think you can just remove this comment and the one in finishSeek().

Done.

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2010: // 11 - Set the current playback
position to the given new playback position
On 2014/09/10 09:57:06, philipj wrote:
> Can you add the missing period here?

Done.

https://codereview.chromium.org/517593003/diff/120001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2013: // 12-17 are handled, if necessary,
when the engine signals a readystate change or otherwise
On 2014/09/10 09:57:06, philipj wrote:
> It's step 14-17 that's handled in finishSeek(). Can you update the numbering
> there as well?

Done.

[philipj_slow, 2014-09-10 13:35:53]

LGTM with a tiny nit. I wish I could just edit directly in the review, but
you'll have to do it and then CQ.

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

https://codereview.chromium.org/517593003/diff/140001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2010: // 12-17 are handled, if necessary,
when the engine signals a readystate change or otherwise
Oops, forgot to update the numbers here :)

[amogh.bihani, 2014-09-10 13:44:21]

On 2014/09/10 13:35:53, philipj wrote:
> LGTM with a tiny nit. I wish I could just edit directly in the review, but
> you'll have to do it and then CQ.
> 
>
https://codereview.chromium.org/517593003/diff/140001/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/517593003/diff/140001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:2010: // 12-17 are handled, if
necessary,
> when the engine signals a readystate change or otherwise
> Oops, forgot to update the numbers here :)

Thanks for the review :)

[amogh.bihani, 2014-09-10 13:44:26]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-09-10 13:44:53]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/517593003/160001

[commit-bot: I haz the power, 2014-09-10 14:49:09]

Committed patchset #7 (id:160001) as 181736