Tell media player to prepareToPlay() at end of HTMLMediaElement::load()

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

Tell media player to prepareToPlay() at end of HTMLMediaElement::load()

[Victoria Kirst]

Created attachment 104243 [details]
Patch

[Victoria Kirst]

This will fix problem where a media element doesn't respond to an explicit call to load() via JS. 

There is a bug filed for this in Chromium:
http://code.google.com/p/chromium/issues/detail?id=81477

But it doesn't appear Chromium-specific so I left off that label from the subject.

[Eric Carlson]

Comment on attachment 104243 [details]
Patch

This looks fine, but it needs a layout test.

[Victoria Kirst]

Created attachment 104258 [details]
add layout test

[Victoria Kirst]

(In reply to comment #3)
> (From update of attachment 104243 [details])
> This looks fine, but it needs a layout test.

Good point! Added a layout test that tests for this. 

In the layout test, listening for the "loadedmetadata" event seemed like the most appropriate thing to do to confirm the call to load() did what we intended. Please advise if you disagree!

[Victoria Kirst]

Created attachment 104261 [details]
layout test polish

[Eric Carlson]

Comment on attachment 104261 [details]
layout test polish

View in context: https://bugs.webkit.org/attachment.cgi?id=104261&action=review

> LayoutTests/media/video-load-preload-none.html:14
> +            function start()
> +            {
> +                findMediaElement();
> +                video.src = findMediaFile("video", "content/test");
> +
> +                testExpected("video.preload", "none");
> +                waitForEventAndEnd('loadedmetadata');
> +                run("video.load()");
> +            }

This test can "succeed"  even if preload=none is ignored. While this tests isn't strictly about that, I think it would be a better test if you add the event listener and set the src attribute in one function, and call load() after a delay to you can ensure that the event handler isn't triggered by just setting the src. We don't like to use setTimeout() in layout tests, but sometimes it can't really be avoided.

[Victoria Kirst]

Created attachment 104399 [details]
load after delay

[Victoria Kirst]

(In reply to comment #7)
> This test can "succeed"  even if preload=none is ignored. While this tests isn't strictly about that, I think it would be a better test if you add the event listener and set the src attribute in one function, and call load() after a delay to you can ensure that the event handler isn't triggered by just setting the src. We don't like to use setTimeout() in layout tests, but sometimes it can't really be avoided.

Thanks for the suggestion! Updated the test to load() after a 250ms delay.

[Eric Carlson]

Comment on attachment 104399 [details]
load after delay

View in context: https://bugs.webkit.org/attachment.cgi?id=104399&action=review

Marking r+ but please change the test before landing to make sure it will catch data loading in spite of preload=none.


> LayoutTests/media/video-load-preload-none.html:18
> +            function load() {

Nit: The opening brace for the function should be on the next line.


> LayoutTests/media/video-load-preload-none.html:20
> +                run("video.load()");
> +            }

The test results could be exactly the same if the load is triggered automatically. You can verify that the load doesn't trigger automatically by checking readyState and networkState in start() and here.

[Victoria Kirst]

Created attachment 104554 [details]
Check readyState in test

[WebKit Review Bot]

Comment on attachment 104554 [details]
Check readyState in test

Clearing flags on attachment: 104554

Committed r93437: <http://trac.webkit.org/changeset/93437>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.