[Webkit-unassigned] [Bug 67418] [chromium] Fix CCLayerTreeHostTest

Thu Sep 15 13:05:21 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=67418

James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #107493|review?                     |review-
               Flag|                            |

--- Comment #9 from James Robinson <jamesr at chromium.org>  2011-09-15 13:05:21 PST ---
(From update of attachment 107493)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.