Bug 74882 – implement layout tests for <video> with media stream

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

Implement the needed code to facilitate integration test for <video> with src from media stream.

Created attachment 119919 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Kent Tamura]

Comment on attachment 119919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119919&action=review

> Tools/DumpRenderTree/chromium/TestMediaStreamClient.cpp:38
> +#include "media/base/message_loop_factory.h"
> +#include "media/base/pipeline.h"
> +#include "media/video/capture/fake_capture_video_decoder.h"

We have to avoid using Chromium code other than webkit/support/ in DRT.

> Tools/DumpRenderTree/chromium/TestMediaStreamClient.cpp:46
> +namespace {
> +
> +static const int kVideoCaptureWidth = 352;
> +static const int kVideoCaptureHeight = 288;
> +static const int kVideoCaptureFramePerSecond = 30;
> +
> +} // namespace

Do not use anonymous namespace and k-prefix in WebKit code.

> Tools/DumpRenderTree/chromium/TestMediaStreamClient.h:38
> +#include "webkit/media/media_stream_client.h"

We have to avoid using Chromium code other than webkit/support/ in DRT.

Created attachment 119969 [details]
Patch

please take another look. The TestMediaStreamClient has been moved into webkit/support in chromium (http://codereview.chromium.org/8887004/).

[Tommy Widenflycht]

This patch must be split into two to keep the build green at all times, so please wait with the review.

[WebKit Review Bot]

Comment on attachment 119969 [details]
Patch

Attachment 119969 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10991359

[Tommy Widenflycht]

Created attachment 120163 [details]
Patch

Created attachment 121353 [details]
Patch

Please take another look.

patch (id=121353) moved WebUserMediaClientMock from Source/WebKit/chromium to Tools/DumpRenderTree/chromium. Now there is no need to have 2 patches (the other is in bug 74930). This patch only depends on chromium patch http://codereview.chromium.org/8887004/.

[WebKit Review Bot]

Comment on attachment 121353 [details]
Patch

Attachment 121353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11122209

The chromium patch has been landed as http://src.chromium.org/viewvc/chrome?view=rev&revision=116749. With chromium r116749, this patch should work.

[Kent Tamura]

Would you post the same patch again please? I'd like to see EWS bot results.

Created attachment 121889 [details]
Patch

[Kent Tamura]

Comment on attachment 121889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121889&action=review

Some style nits and a test question.

> Tools/DumpRenderTree/chromium/WebUserMediaClientMock.cpp:45
> +WebUserMediaClientMock* WebUserMediaClientMock::create()
> +{
> +    return new WebUserMediaClientMock();

This should be:

PassOwnPtr<WebUserMediaClientMock> WebUserMediaClientMock::create()
{
    return adoptPtr(new WebuserMediaClientMock());

> Tools/DumpRenderTree/chromium/WebUserMediaClientMock.cpp:71
> +    size_t size = 1;

nit: This should be 'const size_t'.

> Tools/DumpRenderTree/chromium/WebUserMediaClientMock.h:52
> +};

We had better have

private:
      WebUserMediaClientMock();

> LayoutTests/platform/chromium/media/video-capture-preview.html:11
> +var preview_url = "";
> +var localStream = null;

We usually use camelCase for variable names though we don't have an official JavaScript style.
preview_url -> previewURL

> LayoutTests/platform/chromium/media/video-capture-preview.html:18
> +function gotStream(s)

One-letter variable name is not good.

> LayoutTests/platform/chromium/media/video-capture-preview.html:33
> +    var milliseconds = 2000;
> +    var replayIfTimeIsReached = function ()
> +    {
> +        startPreview();
> +        consoleWrite("restart preview");
> +        setTimeout(endTest, 2500);
> +    }
> +
> +    setTimeout(replayIfTimeIsReached, milliseconds + 100);

What do these magic numbers mean?
 - milliseconds = 2000
 - setTimeout(..., 2500)
 - setTimeout(..., milliseconds + 100)

Created attachment 122064 [details]
Patch

Thanks, Kent! The magic numbers in LayoutTests/platform/chromium/media/video-capture-preview.html are play time. In the new patch, I have consolidated them to one number. Please take another look.

[Kent Tamura]

Comment on attachment 122064 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 122064 [details]
Patch

Clearing flags on attachment: 122064

Committed r104764: <http://trac.webkit.org/changeset/104764>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

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