[webkit-reviews] review denied: [Bug 74882] implement layout tests for <video> with media stream : [Attachment 121889] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied wjia at chromium.org's request for
review:
Bug 74882: implement layout tests for <video> with media stream
https://bugs.webkit.org/show_bug.cgi?id=74882

Attachment 121889: Patch
https://bugs.webkit.org/attachment.cgi?id=121889&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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)