Summary: | timeupdate events stop firing after suspend event fires | ||
---|---|---|---|
Product: | WebKit | Reporter: | Andrew Scherkus <scherkus> |
Component: | Media | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | eric.carlson, eric, ojan |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | OS X 10.5 | ||
Attachments: |
Description
Andrew Scherkus
2010-04-01 19:01:17 PDT
Created attachment 52367 [details] 2010-04-01 Andrew Scherkus <scherkus@chromium.org> Looks like you're using good old post-commits. :) You might want to try out "webkit-patch upload" it will do slightly smarter things with patch names at least. Comment on attachment 52367 [details] 2010-04-01 Andrew Scherkus <scherkus@chromium.org> 63 # WebKit doesn't fire suspend events. Is a confusing comment in all the Skipped tests. You shoudl link to this bug or mention the platform name, or indicate if hte platofrm plans to fix that. :) Created attachment 52387 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org> Does webkit-patch upload work with git? It seems I need to have the changes uncomitted, which scares me a little :s Comment on attachment 52387 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org> Skipping in "mac/" will skip on all mac platforms. no need to skip for Leopard and Tiger individually. ojan is your man. He's making webkit-patch upload better. Just wait another day and it will be more git-friendly. Created attachment 52391 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org> (In reply to comment #6) > (From update of attachment 52387 [details]) > Skipping in "mac/" will skip on all mac platforms. no need to skip for Leopard > and Tiger individually. Perfect! I thought this might be the case, but not having access to a Tiger machine I wasn't really able to test out the theory. I noticed there were some duplicated Skipped entries between mac platforms, so I assumed that it might be per-platform. PS ojan if you're reading this... thank you :) Comment on attachment 52391 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org> > +++ b/LayoutTests/http/tests/media/video-play-suspend.html > @@ -0,0 +1,26 @@ > +<video></video> > +<p>Test that timeupdate events are sent when media loading suspends itself.</p> > +<script src=../../../media/media-file.js></script> > +<script src=../../../media/video-test.js></script> > +<script> > + var suspendCount = 0; > + > + // Make sure we've at least reached NETWORK_LOADING before waiting for suspend. > + waitForEvent('loadstart', function() { > + // suspend event means the user agent has intentionally paused network usage, > + // however we should still receive timeupdate events. > + mediaElement.addEventListener('suspend', function () { > + // timeupdate events are fired as playback progresses so only verify that at least one > + // event is fired > + ++suspendCount; > + if (suspendCount == 1) { > + consoleWrite("EVENT(suspend)"); > + waitForEventAndEnd('timeupdate'); > + } > + } ); > + } ); > + > + var mediaFile = findMediaFile("video", "resources/test"); > + video.src = "http://127.0.0.1:8000/media/" + mediaFile; > + run("video.play()"); > +</script> I really prefer to see tests as whole html documents rather than fragments - include <html> and <body>, put the <script> in a function in <head>, trigger the script on 'load', etc. I think it makes the tests *much* easier to read and understand for someone else down the line. > diff --git a/LayoutTests/platform/gtk/Skipped b/LayoutTests/platform/gtk/Skipped > index d086452..4c4bd1f 100644 > --- a/LayoutTests/platform/gtk/Skipped > +++ b/LayoutTests/platform/gtk/Skipped > @@ -3424,6 +3424,7 @@ http/tests/local/send-sliced-dragged-file.html > http/tests/local/send-form-data.html > http/tests/media/video-play-stall-seek.html > http/tests/media/video-play-stall.html > +http/tests/media/video-play-suspend.html > http/tests/media/video-seekable-stall.html > http/tests/mime/standard-mode-loads-stylesheet-with-empty-content-type.html > http/tests/mime/standard-mode-loads-stylesheet-with-text-css-and-invalid-type.html GTK port doesn't get a comment about why the test is skipped? > diff --git a/LayoutTests/platform/mac/Skipped b/LayoutTests/platform/mac/Skipped > index 7a6a7f7..1eb9fc1 100644 > --- a/LayoutTests/platform/mac/Skipped > +++ b/LayoutTests/platform/mac/Skipped > @@ -60,6 +60,10 @@ svg/hixie/perf/002.xml > # <rdar://problem/6710625> Test media/video-error-abort.html doesn't work > http/tests/media/video-error-abort.html > > +# Mac uses QuickTime, which currently doesn't fire a suspend event for audio/video elements. > +# https://bugs.webkit.org/show_bug.cgi?id=37003 > +http/tests/media/video-play-suspend.html > + A bugzilla url in a skipped file should refer to the bug that must be fixed in order to un-skip the test. Please write bugs for each of the ports that don't fire 'suspend' and reference them in the skipped files, or omit the url. Are you certain that the state is not set to 'idle' because of QuickTime, or is it a bug in the WebCore code that uses QuickTime? > +# Windows uses QuickTime, which currently doesn't fire a suspend event for audio/video elements. > +# https://bugs.webkit.org/show_bug.cgi?id=37003 > +http/tests/media/video-play-suspend.html > + Ditto. r=me with these minor changes. Created attachment 52434 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org> Thanks for the review Eric! I've created the following bugs tracking the NETWORK_IDLE/suspend work: https://bugs.webkit.org/show_bug.cgi?id=37034 https://bugs.webkit.org/show_bug.cgi?id=37035 https://bugs.webkit.org/show_bug.cgi?id=37036 Whoops I uploaded and clobbered the review+ Having addressed Eric's concerns I'll go ahead and land the patch. Committed as http://trac.webkit.org/changeset/57023 Comment on attachment 52434 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org> Clearing r? since this bug is closed. |