Bug 93052 - Fire suspend event when loaded + add layout test
Summary: Fire suspend event when loaded + add layout test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrew Scherkus
URL:
Keywords:
: 93131 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-08-02 17:58 PDT by Andrew Scherkus
Modified: 2012-08-08 11:26 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.49 KB, patch)
2012-08-02 18:01 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (316.67 KB, application/zip)
2012-08-02 19:48 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from apple-mac-2 (333.57 KB, application/zip)
2012-08-02 21:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from apple-mac-5 (458.44 KB, application/zip)
2012-08-02 21:59 PDT, Build Bot
no flags Details
Patch (3.17 KB, patch)
2012-08-03 09:39 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (591.34 KB, application/zip)
2012-08-03 10:18 PDT, WebKit Review Bot
no flags Details
Patch (4.70 KB, patch)
2012-08-07 14:35 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (373.15 KB, application/zip)
2012-08-07 15:13 PDT, WebKit Review Bot
no flags Details
Patch (5.12 KB, patch)
2012-08-07 16:44 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2012-08-02 17:58:53 PDT
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
Comment 1 Andrew Scherkus 2012-08-02 18:01:39 PDT
Created attachment 156222 [details]
Patch
Comment 2 Andrew Scherkus 2012-08-02 18:02:26 PDT
Testing this out on mac snow leopard right now.
Comment 3 Andrew Scherkus 2012-08-02 18:38:53 PDT
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 4 WebKit Review Bot 2012-08-02 19:48:34 PDT
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
Comment 5 WebKit Review Bot 2012-08-02 19:48:38 PDT
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 6 Ami Fischman 2012-08-02 21:25:30 PDT
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 7 Ami Fischman 2012-08-02 21:27:17 PDT
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 8 Build Bot 2012-08-02 21:58:48 PDT
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
Comment 9 Build Bot 2012-08-02 21:58:51 PDT
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 10 Build Bot 2012-08-02 21:58:57 PDT
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
Comment 11 Build Bot 2012-08-02 21:59:00 PDT
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
Comment 12 Andrew Scherkus 2012-08-03 09:39:41 PDT
Created attachment 156399 [details]
Patch
Comment 13 WebKit Review Bot 2012-08-03 10:18:23 PDT
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
Comment 14 WebKit Review Bot 2012-08-03 10:18:27 PDT
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
Comment 15 Eric Carlson 2012-08-04 10:16:43 PDT
(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().
Comment 16 Andrew Scherkus 2012-08-07 14:33:41 PDT
*** Bug 93131 has been marked as a duplicate of this bug. ***
Comment 17 Andrew Scherkus 2012-08-07 14:35:47 PDT
Created attachment 157008 [details]
Patch
Comment 18 Eric Carlson 2012-08-07 14:52:10 PDT
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.
Comment 19 Andrew Scherkus 2012-08-07 14:54:33 PDT
Thanks Eric!
Comment 20 WebKit Review Bot 2012-08-07 15:13:37 PDT
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
Comment 21 WebKit Review Bot 2012-08-07 15:13:41 PDT
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
Comment 22 Andrew Scherkus 2012-08-07 16:44:28 PDT
Created attachment 157040 [details]
Patch
Comment 23 Andrew Scherkus 2012-08-07 16:44:58 PDT
looks like I had to update media/event-attributes.html expectations as it listens for a suspend event
Comment 24 Eric Carlson 2012-08-07 19:38:24 PDT
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.
Comment 25 Andrew Scherkus 2012-08-08 11:26:43 PDT
Addressed all comments.

Committed as http://trac.webkit.org/changeset/125054