Turns out we don't have any coverage for firing a suspend event after loading has started! Feedback welcome on what else we should be covering in the test -- I'm uploading a very rudimentary test that covers http://crbug.com/139511
Created attachment 156222 [details] Patch
Testing this out on mac snow leopard right now.
eric.carlson: on my build (r124543) I actually notice that mac doesn't fire a suspend event after loadstart. Testing on Safari 5.1.7 on Snow Leopard I also notice that while progress events are fired until the entirety of the media is buffered, no suspend event is fired. Know what's up?
Comment on attachment 156222 [details] Patch Attachment 156222 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13424499 New failing tests: http/tests/media/video-load-suspend.html
Created attachment 156246 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 156222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156222&action=review > LayoutTests/http/tests/media/video-load-suspend.html:8 > + function onSuspend() { Vestigial?
Comment on attachment 156222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156222&action=review > LayoutTests/ChangeLog:8 > + There was a regression in the Chromium port (http://crbug.com/93052) and on further investigation Did you mean to point to the crbug here instead?
Comment on attachment 156222 [details] Patch Attachment 156222 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13424534 New failing tests: http/tests/media/video-load-suspend.html
Created attachment 156258 [details] Archive of layout-test-results from apple-mac-2 The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: apple-mac-2 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.7.4
Comment on attachment 156222 [details] Patch Attachment 156222 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13417772 New failing tests: http/tests/media/video-load-suspend.html
Created attachment 156259 [details] Archive of layout-test-results from apple-mac-5 The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: apple-mac-5 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.7.4
Created attachment 156399 [details] Patch
Comment on attachment 156399 [details] Patch Attachment 156399 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13419958 New failing tests: http/tests/media/video-load-suspend.html
Created attachment 156406 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
(In reply to comment #3) > eric.carlson: on my build (r124543) I actually notice that mac doesn't fire a suspend event after loadstart. Testing on Safari 5.1.7 on Snow Leopard I also notice that while progress events are fired until the entirety of the media is buffered, no suspend event is fired. > > Know what's up? There is a bug in HTMLMediaElement::setNetworkState, so we only fire 'suspend' when the networkState is set to MediaPlayer::Idle, not when it is set to MediaPlayer::Loaded. It looks like we should move the firing of the event into changeNetworkStateFromLoadingToIdle().
*** Bug 93131 has been marked as a duplicate of this bug. ***
Created attachment 157008 [details] Patch
Comment on attachment 157008 [details] Patch Thanks for the fix! Marking r+ but not cq+ as the bots have not all had a chance to chew on this yet. You can set the cq flag yourself at any point.
Thanks Eric!
Comment on attachment 157008 [details] Patch Attachment 157008 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13455260 New failing tests: media/event-attributes.html
Created attachment 157018 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 157040 [details] Patch
looks like I had to update media/event-attributes.html expectations as it listens for a suspend event
Comment on attachment 157040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157040&action=review > Source/WebCore/ChangeLog:12 > + Fire suspend event whenever network state is set to NETWORK_IDLE. > + https://bugs.webkit.org/show_bug.cgi?id=93052 > + > + Reviewed by NOBODY (OOPS!). > + > + There was a regression in the Chromium port (http://crbug.com/139511) that revealed we didn't > + have a layout test for suspend events. Upon further investigation it appeared we also had a bug > + where we didn't fire the suspend event when a media engine reported they had completely loaded > + the media. > + You should mention the new test. > LayoutTests/ChangeLog:15 > + * http/tests/media/video-load-suspend-expected.txt: Added. > + * http/tests/media/video-load-suspend.html: Added. > + You forgot to log the changes to media/event-attributes-expected.txt. > LayoutTests/http/tests/media/video-load-suspend.html:10 > + function init() { > + findMediaElement(); > + run("video.src = file"); Nit: A function's opening brace should be on a new line.
Addressed all comments. Committed as http://trac.webkit.org/changeset/125054