Bug 93052 – Fire suspend event when loaded + add layout test

• Component: Media
• Status: RESOLVED FIXED

[Andrew Scherkus]

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

[Andrew Scherkus]

Created attachment 156222 [details]
Patch

[Andrew Scherkus]

Testing this out on mac snow leopard right now.

[Andrew Scherkus]

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?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ami Fischman]

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?

[Ami Fischman]

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?

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Andrew Scherkus]

Created attachment 156399 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eric Carlson]

(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().

[Andrew Scherkus]

*** Bug 93131 has been marked as a duplicate of this bug. ***

[Andrew Scherkus]

Created attachment 157008 [details]
Patch

[Eric Carlson]

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.

[Andrew Scherkus]

Thanks Eric!

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Andrew Scherkus]

Created attachment 157040 [details]
Patch

[Andrew Scherkus]

looks like I had to update media/event-attributes.html expectations as it listens for a suspend event

[Eric Carlson]

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.

[Andrew Scherkus]

Addressed all comments.

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