[webkit-reviews] review denied: [Bug 67418] [chromium] Fix CCLayerTreeHostTest : [Attachment 107493] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Iain Merrick
<husky at google.com>'s request for review:
Bug 67418: [chromium] Fix CCLayerTreeHostTest
https://bugs.webkit.org/show_bug.cgi?id=67418

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107493&action=review


Some issues, but I think we're getting there.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:45
> +PassOwnPtr<CCLayerTreeHostImpl>
CCLayerTreeHostClient::createLayerTreeHostImpl(const CCSettings& settings)
> +{
> +    return CCLayerTreeHostImpl::create(settings);
> +}

instead of going through the client to create the impl, I think you should just
make createLayerTreeHostImpl() virtual and override it in the test

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:70
> +    virtual PassOwnPtr<CCLayerTreeHostImpl> createLayerTreeHostImpl(const
CCSettings&);

this is wrong.	The client (person who's using CCLayerTreeHost) should have no
knowledge of *Impl stuff

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:189
> +    // TEMP HACK so we can exercise this code in unit tests.
> +    CCMainThread::postTask(createMainThreadTask(this,
&CCThreadProxy::beginFrameAndCommit, 0.0));

I don't think you want to land these

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:250
> +    // TEMP HACK so we can exercise this code in unit tests.
> +    CCMainThread::postTask(createMainThreadTask(this,
&CCThreadProxy::beginFrameAndCommit, 0.0));

Same here, don't think you want to land

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:257
> +    // TEMP HACK so we can exercise this code in unit tests.
> +    drawLayersOnCCThread();

Don't land

> Source/WebKit/chromium/ChangeLog:21
> +	   (WTF::CCLayerTreeHostTest::beginCommitOnMainThread):
> +	   (WTF::CCLayerTreeHostTest::drawLayersOnCCThread):

is the script confused?  These aren't really in the WTF namespace, are they?
They probably should be in the WebCore namespace.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:-104
> -#if USE(ACCELERATED_COMPOSITING)
> -    m_compositingLayer = WebGLLayerChromium::create(0);
> -#endif

Why are you moving this? (I don't think it's necessarily a bad thing to do, but
I don't understand how it relates to the rest of this patch)

> Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:328
> +    mutable RefPtr<WebGLLayerChromium> m_compositingLayer;

instead of making this mutable, can you make ::platformLayer() be non-const?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:29
>  #include "cc/CCLayerTreeHost.h"

i think this #include should be sorted in the normal order with everything else


> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:79
>      virtual void animateAndLayout(MockLayerTreeHostClient* layerTreeHost,
double frameBeginTime) { }
>      virtual void beginCommitOnCCThread(MockLayerTreeHostImpl*
layerTreeHostImpl) { }
>      virtual void beginCommitOnMainThread(MockLayerTreeHost* layerTreeHost) {
}

looks like we don't have any implementations of this that do anything. Do we
plan to grow some?

For now, since the impl doesn't use the params don't name them.

This class has a protected RefPtr<MockLayerTreeHost> member, so I don't
understand why we would want to pass a MockLayerTreeHost* to member functions.
Passing the impl* seems like it could make sense sometimes.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:80
>      virtual void commitCompleteOnCCThread(MockLayerTreeHostImpl*
layerTreeHostImpl) { }

why does this function take a layerTreeHostImpl* ? I don't think any
implementations actually use the parameter

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:81
>      virtual void commitCompleteOnMainThread(MockLayerTreeHost*
layerTreeHost) { }

passing the MockLayerTreeHost* makes no sense

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:83
>      virtual void updateLayers(MockLayerTreeHost* layerTreeHost) { }

passing the MockLayerTreeHost* makes no sense

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:176
> +class MyMockContext : public MockWebGraphicsContext3D {

This isn't a good class name. it looks like this is a MockWebGraphicsContext3D
that's used for compositor tests, so maybe something like
CompositorMockWebGraphicsContext3D?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:181
> +    virtual void getShaderiv(WebGLId shader, WGC3Denum pname, WGC3Dint*
value)

don't provide names for the parameters that are not used in the function or you
will run in to unused variable warnings. IOW write this definition as:

virtual void getShaderiv(WebGLId, WGC3Denum, WGC3Dint* value)

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:185
> +    virtual void getProgramiv(WebGLId program, WGC3Denum pname, WGC3Dint*
value)

same here

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:228
> +    MockLayerTreeHostClient(CCLayerTreeHostTest* test) : m_test(test) { }

explicit. Why doesn't this class have a static public PassOwnPtr<> create()
function and a private c'tor, like we normally do?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:258
> +    virtual void didRecreateGraphicsContext(bool success)

don't name the param if you aren't using it in the body (you can name it in a
comment if you like)

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:426
> +    virtual void commitCompleteOnCCThread(MockLayerTreeHostImpl* impl)

don't name param

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:469
> +    virtual void commitCompleteOnCCThread(MockLayerTreeHostImpl* impl)

don't name this param

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:508
> +	       postSetNeedsRedrawToMainThread(); // redraw again to verify that
the second redraw doesnt commit.

please capitalize the start of this sentence

doesnt -> doesn't

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:514
> +    virtual void commitCompleteOnCCThread(MockLayerTreeHostImpl* impl)

don't name param