Set state of a new audio stream to paused until it starts playing.

Set state of a new audio stream to Paused until it start to play. Also added media layout tests in ui tests.
BUG=39825
TEST=sound is still playing (with and without autoplay).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56953

[Sergey Ulanov, 2010-08-19 00:09:16]

This is a first change to fix http://crbug.com/39825 .

[Alpha Left Google, 2010-08-19 06:42:07]

LGTM if it passes layout tests.

[scherkus (not reviewing), 2010-08-19 06:45:16]

hold on -- test_shell uses NullAudioRenderer and not AudioRendererImpl...

I'm fairly certain we lack an integration test that goes through
AudioRendererImpl path (other than the UI tests, but they're not testing for
this stuff)

[Alpha Left Google, 2010-08-19 07:11:55]

One simpler way to solve this is to the layout tests as UI tests, I've seen app
cache or maybe database using this trick to test their chrome implementation.

[Sergey Ulanov, 2010-08-20 18:08:43]

On 2010/08/19 07:11:55, Alpha wrote:
> One simpler way to solve this is to the layout tests as UI tests, I've seen
app
> cache or maybe database using this trick to test their chrome implementation.
I've added layout UI tests for media. The test is disabled because they fail
with chromium (test expects mp4 while ogv is played).

[Paweł Hajdan Jr., 2010-08-20 18:12:23]

Drive-by with test comments.

http://codereview.chromium.org/3160027/diff/7001/8002
File chrome/browser/media_uitest.cc (right):

http://codereview.chromium.org/3160027/diff/7001/8002#newcode59
chrome/browser/media_uitest.cc:59: TEST_F(UILayoutTest,
DISABLED_MediaUILayoutTest) {
Every disabled test should have a bug with Tests-Disabled label and the link to
the bug should be in a comment above the test.

Like // Disabled, http://crbug.com/1234.

http://codereview.chromium.org/3160027/diff/7001/8002#newcode86
chrome/browser/media_uitest.cc:86: RunLayoutTest(kMediaTests[i], kNoHttpPort);
I suggest splitting the test to more precisely identify failures.

[Alpha Left Google, 2010-08-20 18:13:04]

On 2010/08/20 18:08:43, sergeyu wrote:
> On 2010/08/19 07:11:55, Alpha wrote:
> > One simpler way to solve this is to the layout tests as UI tests, I've seen
> app
> > cache or maybe database using this trick to test their chrome
implementation.
> I've added layout UI tests for media. The test is disabled because they fail
> with chromium (test expects mp4 while ogv is played).

Are the test html files uploaded? I don't see them in this review.

Also is it possible to modify the test so that they take ogv instead of mp4?

To expand this test harness we can test different file formats based on the
build, but this will be a separate patch. We do want to test webm and other
formats and run through the whole system.

[Sergey Ulanov, 2010-08-20 18:19:02]

On Fri, Aug 20, 2010 at 11:13 AM, <hclam@chromium.org> wrote:

> On 2010/08/20 18:08:43, sergeyu wrote:
>
>> On 2010/08/19 07:11:55, Alpha wrote:
>> > One simpler way to solve this is to the layout tests as UI tests, I've
>> seen
>> app
>> > cache or maybe database using this trick to test their chrome
>>
> implementation.
>
>> I've added layout UI tests for media. The test is disabled because they
>> fail
>> with chromium (test expects mp4 while ogv is played).
>>
>
> Are the test html files uploaded? I don't see them in this review.
>
see the DEPS file. Basically I pull the tests from webkit.


>
> Also is it possible to modify the test so that they take ogv instead of
> mp4?
>
Looks like it has been fixed for video-loop yesterday:
https://bugs.webkit.org/show_bug.cgi?id=33671
Will try to enable test, run it on trybots.


>
> To expand this test harness we can test different file formats based on the
> build, but this will be a separate patch. We do want to test webm and other
> formats and run through the whole system.
>
>
>
> http://codereview.chromium.org/3160027/show
>

[Alpha Left Google, 2010-08-20 18:20:45]

sounds good. LGTM.

[Sergey Ulanov, 2010-08-21 00:37:11]

http://codereview.chromium.org/3160027/diff/7001/8002
File chrome/browser/media_uitest.cc (right):

http://codereview.chromium.org/3160027/diff/7001/8002#newcode59
chrome/browser/media_uitest.cc:59: TEST_F(UILayoutTest,
DISABLED_MediaUILayoutTest) {
On 2010/08/20 18:12:23, Paweł Hajdan Jr. wrote:
> Every disabled test should have a bug with Tests-Disabled label and the link
to
> the bug should be in a comment above the test.
> 
> Like // Disabled, http://crbug.com/1234.

Enabled the test, but some of the test cases are disabled.

http://codereview.chromium.org/3160027/diff/7001/8002#newcode86
chrome/browser/media_uitest.cc:86: RunLayoutTest(kMediaTests[i], kNoHttpPort);
On 2010/08/20 18:12:23, Paweł Hajdan Jr. wrote:
> I suggest splitting the test to more precisely identify failures.
It would slow down the tests significantly because the video files would have to
be copied to the temp directory every time.