Bug 74882

Summary: implement layout tests for <video> with media stream
Product: WebKit Reporter: wjia
Component: Tools / TestsAssignee: wjia
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, harald, tkent, tommyw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 74930    
Bug Blocks: 56587    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description wjia 2011-12-19 13:31:20 PST
Implement the needed code to facilitate integration test for <video> with src from media stream.
Comment 1 wjia 2011-12-19 14:25:21 PST
Created attachment 119919 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-19 14:34:33 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Kent Tamura 2011-12-19 17:23:37 PST
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.
Comment 4 wjia 2011-12-19 18:45:14 PST
Created attachment 119969 [details]
Patch
Comment 5 wjia 2011-12-19 18:47:34 PST
please take another look. The TestMediaStreamClient has been moved into webkit/support in chromium (http://codereview.chromium.org/8887004/).
Comment 6 Tommy Widenflycht 2011-12-20 07:22:06 PST
This patch must be split into two to keep the build green at all times, so please wait with the review.
Comment 7 WebKit Review Bot 2011-12-20 13:04:39 PST
Comment on attachment 119969 [details]
Patch

Attachment 119969 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10991359
Comment 8 Tommy Widenflycht 2011-12-21 02:31:33 PST
Created attachment 120163 [details]
Patch
Comment 9 wjia 2012-01-05 15:40:37 PST
Created attachment 121353 [details]
Patch
Comment 10 wjia 2012-01-05 15:46:08 PST
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/.
Comment 11 WebKit Review Bot 2012-01-05 20:57:08 PST
Comment on attachment 121353 [details]
Patch

Attachment 121353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11122209
Comment 12 wjia 2012-01-06 15:59:55 PST
The chromium patch has been landed as http://src.chromium.org/viewvc/chrome?view=rev&revision=116749. With chromium r116749, this patch should work.
Comment 13 Kent Tamura 2012-01-09 20:16:34 PST
Would you post the same patch again please? I'd like to see EWS bot results.
Comment 14 wjia 2012-01-10 12:10:17 PST
Created attachment 121889 [details]
Patch
Comment 15 Kent Tamura 2012-01-10 16:54:16 PST
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)
Comment 16 wjia 2012-01-11 11:53:31 PST
Created attachment 122064 [details]
Patch
Comment 17 wjia 2012-01-11 11:55:44 PST
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.
Comment 18 Kent Tamura 2012-01-11 15:24:03 PST
Comment on attachment 122064 [details]
Patch

LGTM
Comment 19 WebKit Review Bot 2012-01-11 16:27:12 PST
Comment on attachment 122064 [details]
Patch

Clearing flags on attachment: 122064

Committed r104764: <http://trac.webkit.org/changeset/104764>
Comment 20 WebKit Review Bot 2012-01-11 16:27:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 wjia 2012-01-12 10:24:42 PST
*** Bug 74930 has been marked as a duplicate of this bug. ***