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

Fri Sep 16 04:07:48 PDT 2011

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

--- Comment #10 from Iain Merrick <husky at google.com>  2011-09-16 04:07:48 PST ---
(From update of attachment 107493)
View in context: https://bugs.webkit.org/attachment.cgi?id=107493&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:45
>> +}
> 
> instead of going through the client to create the impl, I think you should just make createLayerTreeHostImpl() virtual and override it in the test

Good call, that makes much more sense. Done. (That involved moving some classes around in CCLayerTreeHostTest.cpp, because the forward declarations were getting out of hand, but I think it's an improvement.)

I hadn't realised that CCProxy calls this method. This ties pretty nicely into extracting a pure virtual interface for the CCProxy -> CCLayerTreeHost calls.

>> 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

Removed.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:189
>> +    CCMainThread::postTask(createMainThreadTask(this, &CCThreadProxy::beginFrameAndCommit, 0.0));
> 
> I don't think you want to land these

Well, I need something other than ASSERT_NOT_REACHED! I guess there's another patch I'm waiting on?

>> Source/WebKit/chromium/ChangeLog:21
>> +        (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.

Good question. The test code is actually in an anonymous namespace, which seems right. Do all these methods really need to be listed in the ChangeLog? They aren't public APIs.

The test source is in WebKit/chromium/tests. Should it be in WebCore instead? That would be a much bigger change as there isn't any existing unit test stuff in WebCore. I'd definitely be in favour of that, but maybe not in this patch. :)

>> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:-104
>> -#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)

This is only needed when the graphics context is being created for use on another thread.

Without this, WebGLLayerChromium is created on the main thread and destroyed on the CC thread. It contains a Timer member variable, and Timer has asserts against multithreaded usage.

I did it this way to limit that unusual create-on-one-thread, destroy-on-another behaviour to GraphicsContext3D.

Alternative ways to fix it:
- Lazy initialize the Timer instead
- Make the asserts in Timer less strict (risky, affects other code)

But both of those mean running more constructors/destructors with those unusual semantics.

What do you think?

>> Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:328
>> +    mutable RefPtr<WebGLLayerChromium> m_compositingLayer;
> 
> instead of making this mutable, can you make ::platformLayer() be non-const?

Yep, done.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:31
> +#include "CCThreadImpl.h"

Huh, code review tool is somehow confused by those comments and won't let me reply directly.

#include ordering fixed, unused virtual methods and parameters deleted.

>> 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?

As Nat says elsewhere, "mock" is actually not a great name for the base class. But I'll stick with that for the purposes of this patch.

CompositorMock is good, I'll use that (and maybe rename to CompositorStub later).

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:181

> 
> 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)

Done

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

Done

>> 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?

Explicit added.

For the create() function, I'm not sure what our guidelines are, but I think it probably isn't beneficial here. This is a private class used only in this test; and if we were to move it into its own header for reuse in other tests, we'd probably want to write subclasses rather than calling create().

>> 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)

Fixed

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

Fixed

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:469
>> +    virtual void commitCompleteOnCCThread(MockLayerTreeHostImpl* impl)
> 
> don't name this param

Fixed

>> 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

Fixed

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