Bug 66414 - Tell media player to prepareToPlay() at end of HTMLMediaElement::load()
Summary: Tell media player to prepareToPlay() at end of HTMLMediaElement::load()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Victoria Kirst
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-17 14:26 PDT by Victoria Kirst
Modified: 2011-08-19 14:12 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.16 KB, patch)
2011-08-17 14:27 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff
add layout test (3.45 KB, patch)
2011-08-17 15:44 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff
layout test polish (3.39 KB, patch)
2011-08-17 15:56 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff
load after delay (3.64 KB, patch)
2011-08-18 14:30 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff
Check readyState in test (3.89 KB, patch)
2011-08-19 13:31 PDT, Victoria Kirst
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victoria Kirst 2011-08-17 14:26:22 PDT
Tell media player to prepareToPlay() at end of HTMLMediaElement::load()
Comment 1 Victoria Kirst 2011-08-17 14:27:01 PDT
Created attachment 104243 [details]
Patch
Comment 2 Victoria Kirst 2011-08-17 14:30:16 PDT
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.
Comment 3 Eric Carlson 2011-08-17 14:38:47 PDT
Comment on attachment 104243 [details]
Patch

This looks fine, but it needs a layout test.
Comment 4 Victoria Kirst 2011-08-17 15:44:48 PDT
Created attachment 104258 [details]
add layout test
Comment 5 Victoria Kirst 2011-08-17 15:51:46 PDT
(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!
Comment 6 Victoria Kirst 2011-08-17 15:56:51 PDT
Created attachment 104261 [details]
layout test polish
Comment 7 Eric Carlson 2011-08-18 06:16:00 PDT
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.
Comment 8 Victoria Kirst 2011-08-18 14:30:40 PDT
Created attachment 104399 [details]
load after delay
Comment 9 Victoria Kirst 2011-08-18 14:38:07 PDT
(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.
Comment 10 Eric Carlson 2011-08-18 14:43:48 PDT
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.
Comment 11 Victoria Kirst 2011-08-19 13:31:08 PDT
Created attachment 104554 [details]
Check readyState in test
Comment 12 WebKit Review Bot 2011-08-19 14:12:40 PDT
Comment on attachment 104554 [details]
Check readyState in test

Clearing flags on attachment: 104554

Committed r93437: <http://trac.webkit.org/changeset/93437>
Comment 13 WebKit Review Bot 2011-08-19 14:12:45 PDT
All reviewed patches have been landed.  Closing bug.