Add test to verify durationchange event if the actual duration is wrong

Add test to verify durationchange event if the actual duration is wrong

If the initially reported duration value is less than the actual
file content, there is a durationchange event fired just before
the ended event to correct the duration.

This should land after
https://codereview.chromium.org/740663002/

TEST: LayoutTests/media/video-durationchange-on-ended.html

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

[Srirama, 2014-12-10 14:20:11]

Patchset #1 (id:1) has been deleted

[Srirama, 2014-12-10 14:22:38]

Patchset #1 (id:20001) has been deleted

[Srirama, 2014-12-10 14:25:36]

srirama.m@samsung.com changed reviewers:
+ dalecurtis@chromium.org, philipj@opera.com

[Srirama, 2014-12-10 14:25:37]

Added test case to verify durationchange event when the actual duration is less
than the reported duration.
PTAL.

[philipj_slow, 2014-12-10 14:42:13]

Try to make truncated.webm as small as possible while still serving its current
purpose, so that the test runs as fast as possible.

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
File LayoutTests/media/video-durationchange-on-ended.html (right):

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:4: <script
src=media-file.js></script>
media-file.js probably isn't needed.

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:9: video =
document.getElementById('video');
You can simplify the test a bit by moving this script to body, creating the
video element in script and running these steps directly. As little boilerplate
as possible :)

[Srirama, 2014-12-10 14:45:21]

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
File LayoutTests/media/video-durationchange-on-ended.html (right):

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:4: <script
src=media-file.js></script>
On 2014/12/10 14:42:13, philipj wrote:
> media-file.js probably isn't needed.

Acknowledged.

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:9: video =
document.getElementById('video');
On 2014/12/10 14:42:13, philipj wrote:
> You can simplify the test a bit by moving this script to body, creating the
> video element in script and running these steps directly. As little
boilerplate
> as possible :)

I have followed the html structure and put all the tags in proper place, so do i
need to move the script inside body? Is it ok to break the structure?

[philipj_slow, 2014-12-10 15:05:41]

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
File LayoutTests/media/video-durationchange-on-ended.html (right):

https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:9: video =
document.getElementById('video');
On 2014/12/10 14:45:20, Srirama wrote:
> On 2014/12/10 14:42:13, philipj wrote:
> > You can simplify the test a bit by moving this script to body, creating the
> > video element in script and running these steps directly. As little
> boilerplate
> > as possible :)
> 
> I have followed the html structure and put all the tags in proper place, so do
i
> need to move the script inside body? Is it ok to break the structure?

It's valid to have scripts in body, and in this case it removes the need for an
extra event handler.

[Srirama, 2014-12-10 15:14:10]

Patchset #2 (id:60001) has been deleted

[Srirama, 2014-12-10 15:23:08]

On 2014/12/10 15:05:41, philipj wrote:
>
https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
> File LayoutTests/media/video-durationchange-on-ended.html (right):
> 
>
https://codereview.chromium.org/790153002/diff/40001/LayoutTests/media/video-...
> LayoutTests/media/video-durationchange-on-ended.html:9: video =
> document.getElementById('video');
> On 2014/12/10 14:45:20, Srirama wrote:
> > On 2014/12/10 14:42:13, philipj wrote:
> > > You can simplify the test a bit by moving this script to body, creating
the
> > > video element in script and running these steps directly. As little
> > boilerplate
> > > as possible :)
> > 
> > I have followed the html structure and put all the tags in proper place, so
do
> i
> > need to move the script inside body? Is it ok to break the structure?
> 
> It's valid to have scripts in body, and in this case it removes the need for
an
> extra event handler.

Done. Reduced truncated.webm size to 33kb (a bit more than 1s). Moved script to
body. I have to hardcode the video.src value as findMediaFile() is looking for
ogg file on linux. Is the video creation part fine or can it be done better?

[philipj_slow, 2014-12-10 22:59:14]

LGTM with nits.

https://codereview.chromium.org/790153002/diff/80001/LayoutTests/media/video-...
File LayoutTests/media/video-durationchange-on-ended.html (right):

https://codereview.chromium.org/790153002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:10:
waitForEvent('durationchange');
Put these in the end, that seems more obvious and if the events are fired before
play() has returned there's a bug that would be good to expose.

Can you also assert what the duration is before and after this event, so that
it's clear that it's really shrinking as the description claims?

https://codereview.chromium.org/790153002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:12: video =
document.getElementById('video');
This is OK, but you can just document.createElement('video') since being in a
document isn't important to the test. As you see fit.

[Srirama, 2014-12-11 06:43:57]

PTAL one final time.

https://codereview.chromium.org/790153002/diff/80001/LayoutTests/media/video-...
File LayoutTests/media/video-durationchange-on-ended.html (right):

https://codereview.chromium.org/790153002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:10:
waitForEvent('durationchange');
On 2014/12/10 22:59:14, philipj wrote:
> Put these in the end, that seems more obvious and if the events are fired
before
> play() has returned there's a bug that would be good to expose.
> 
> Can you also assert what the duration is before and after this event, so that
> it's clear that it's really shrinking as the description claims?

Moved the events down.
Verified duration change with a condition, instead of printing the values as the
final duration changes sometimes in the order of milliseconds

https://codereview.chromium.org/790153002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-durationchange-on-ended.html:12: video =
document.getElementById('video');
On 2014/12/10 22:59:14, philipj wrote:
> This is OK, but you can just document.createElement('video') since being in a
> document isn't important to the test. As you see fit.

Done.

[philipj_slow, 2014-12-11 08:15:42]

lgtm

[Srirama, 2014-12-12 02:50:20]

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

[commit-bot: I haz the power, 2014-12-12 02:50:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/790153002/100001

[commit-bot: I haz the power, 2014-12-12 03:02:30]

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