Re-Relanding 'Always notify the WebMediaPlayer of any seek' patch

Re-Relanding 'Always notify the WebMediaPlayer of any seek' patch

This change removes short circuit logic for seeking to the current position.
There are certain situations, like seeking to a truncated duration, where the
HTMLMediaElement is not in the best position to determine whether a call to
the WebMediaPlayer is necessary or not. This change allows the
WebMediaPlayer implementation to determine what the best course
of action should be. This also makes the seeking behavior more spec
compliant because even a seek to the current position should have the
seeking attribute be true until a stable state occurs. This is guaranteed
when calling into the WebMediaPlayer and allows all seeks to be treated the
same at the HTMLMediaElement level.

BUG=266631
TEST=LayoutTests/media/video-seek-to-current-position.html

Original CL: https://codereview.chromium.org/431903003/
Re-attemp: https://codereview.chromium.org/456343002/

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

[Srirama, 2015-08-03 16:25:40]

srirama.m@samsung.com changed reviewers:
+ philipj@opera.com

[Srirama, 2015-08-03 16:25:41]

PTAL

[philipj_slow, 2015-08-04 11:07:10]

You can simplify the description to only talk about WebMediaPlayer.

[Srirama, 2015-08-04 11:11:50]

On 2015/08/04 11:07:10, philipj wrote:
> You can simplify the description to only talk about WebMediaPlayer.

Thanks, removed MediaPlayer from description :)

[philipj_slow, 2015-08-04 11:16:47]

LGTM % nits

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/media-con...
File LayoutTests/media/media-controller-playbackrate.html (right):

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/media-con...
LayoutTests/media/media-controller-playbackrate.html:7: var start = function()
Please revert the style changes, it's OK that different tests have different
styles as long as they're internally consistent.

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/media-con...
LayoutTests/media/media-controller-playbackrate.html:48:
waitForEventOnce("timeupdate", timeupdate);
Please break this out into a "var playing = function()". Nesting all of the
functions would also be OK, but would make for a bigger diff.

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
File LayoutTests/media/video-seek-to-current-position.html (right):

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
LayoutTests/media/video-seek-to-current-position.html:4: <title>Test that
seeking attribute is true immediately after a seek
Can you check if this tests anything not covered by
https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded...
?

If that test is good enough, either importing it into
LayoutTests/imported/web-platform-tests/ or copying it here with a link and TODO
would be nice.

[Srirama, 2015-08-04 11:53:38]

Patchset #2 (id:20001) has been deleted

[Srirama, 2015-08-04 12:03:21]

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/media-con...
File LayoutTests/media/media-controller-playbackrate.html (right):

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/media-con...
LayoutTests/media/media-controller-playbackrate.html:7: var start = function()
On 2015/08/04 11:16:47, philipj wrote:
> Please revert the style changes, it's OK that different tests have different
> styles as long as they're internally consistent.

Done.

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/media-con...
LayoutTests/media/media-controller-playbackrate.html:48:
waitForEventOnce("timeupdate", timeupdate);
On 2015/08/04 11:16:47, philipj wrote:
> Please break this out into a "var playing = function()". Nesting all of the
> functions would also be OK, but would make for a bigger diff.

Done.

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
File LayoutTests/media/video-seek-to-current-position.html (right):

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
LayoutTests/media/video-seek-to-current-position.html:4: <title>Test that
seeking attribute is true immediately after a seek
On 2015/08/04 11:16:47, philipj wrote:
> Can you check if this tests anything not covered by
>
https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded...
> ?
> 
> If that test is good enough, either importing it into
> LayoutTests/imported/web-platform-tests/ or copying it here with a link and
TODO
> would be nice.

The purpose of this test is to check if the seeking attribute is true during
seeking.
That check is intentionally ignored in that test due to an issue with seeking
behaviour that time(https://www.w3.org/Bugs/Public/show_bug.cgi?id=24774).
So would you like me to modify and import it and ignore this test case?
And can you please tell me what do you mean by "copying it here with a link and
TODO"?

[philipj_slow, 2015-08-04 13:04:45]

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
File LayoutTests/media/video-seek-to-current-position.html (right):

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
LayoutTests/media/video-seek-to-current-position.html:4: <title>Test that
seeking attribute is true immediately after a seek
On 2015/08/04 12:03:21, Srirama wrote:
> On 2015/08/04 11:16:47, philipj wrote:
> > Can you check if this tests anything not covered by
> >
>
https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded...
> > ?
> > 
> > If that test is good enough, either importing it into
> > LayoutTests/imported/web-platform-tests/ or copying it here with a link and
> TODO
> > would be nice.
> 
> The purpose of this test is to check if the seeking attribute is true during
> seeking.
> That check is intentionally ignored in that test due to an issue with seeking
> behaviour that time(https://www.w3.org/Bugs/Public/show_bug.cgi?id=24774).
> So would you like me to modify and import it and ignore this test case?
> And can you please tell me what do you mean by "copying it here with a link
and
> TODO"?

That issue I think affects Blink, that's how I noticed, but in any cast both
this and the web-platform-tests test asserts that seeking is false in the seeked
event, so I think just the web-platform-tests test is OK.

If you don't want to get involved with the import procedure, just copy it and
leave a comment like in LayoutTests/media/autoplay.html and
LayoutTests/media/track/opera/interfaces/TextTrackList/onaddtrack.html

https://codereview.chromium.org/1267953003/diff/40001/LayoutTests/media/media...
File LayoutTests/media/media-controller-effective-playback-rate.html (right):

https://codereview.chromium.org/1267953003/diff/40001/LayoutTests/media/media...
LayoutTests/media/media-controller-effective-playback-rate.html:44: }
Semicolon here.

[Srirama, 2015-08-04 14:56:29]

Patchset #3 (id:60001) has been deleted

[Srirama, 2015-08-04 15:02:57]

One final review please before landing.

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
File LayoutTests/media/video-seek-to-current-position.html (right):

https://codereview.chromium.org/1267953003/diff/1/LayoutTests/media/video-see...
LayoutTests/media/video-seek-to-current-position.html:4: <title>Test that
seeking attribute is true immediately after a seek
On 2015/08/04 13:04:45, philipj wrote:
> On 2015/08/04 12:03:21, Srirama wrote:
> > On 2015/08/04 11:16:47, philipj wrote:
> > > Can you check if this tests anything not covered by
> > >
> >
>
https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded...
> > > ?
> > > 
> > > If that test is good enough, either importing it into
> > > LayoutTests/imported/web-platform-tests/ or copying it here with a link
and
> > TODO
> > > would be nice.
> > 
> > The purpose of this test is to check if the seeking attribute is true during
> > seeking.
> > That check is intentionally ignored in that test due to an issue with
seeking
> > behaviour that time(https://www.w3.org/Bugs/Public/show_bug.cgi?id=24774).
> > So would you like me to modify and import it and ignore this test case?
> > And can you please tell me what do you mean by "copying it here with a link
> and
> > TODO"?
> 
> That issue I think affects Blink, that's how I noticed, but in any cast both
> this and the web-platform-tests test asserts that seeking is false in the
seeked
> event, so I think just the web-platform-tests test is OK.
> 
> If you don't want to get involved with the import procedure, just copy it and
> leave a comment like in LayoutTests/media/autoplay.html and
> LayoutTests/media/track/opera/interfaces/TextTrackList/onaddtrack.html

Done.

https://codereview.chromium.org/1267953003/diff/40001/LayoutTests/media/media...
File LayoutTests/media/media-controller-effective-playback-rate.html (right):

https://codereview.chromium.org/1267953003/diff/40001/LayoutTests/media/media...
LayoutTests/media/media-controller-effective-playback-rate.html:44: }
On 2015/08/04 13:04:45, philipj wrote:
> Semicolon here.

Done.

[philipj_slow, 2015-08-04 15:07:28]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-08-04 15:07:28]

lgtm

[commit-bot: I haz the power, 2015-08-04 15:07:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267953003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267953003/80001

[commit-bot: I haz the power, 2015-08-04 16:06:06]

Committed patchset #3 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199977