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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Nat Duca <nduca at chromium.org>  2011-09-08 12:49:28 PST ---
(From update of attachment 106759)
View in context: https://bugs.webkit.org/attachment.cgi?id=106759&action=review

Let me prefix the following comments with a big sign saying "I suck, this is all my fault, not yours." Moreover, my comments below might be better treated as a series of followon patches to this patch, but one that we should probably do above any other efforts to expand the test coverage.

One big problem with this test is that the actual TEST_ cases only test the proxy, but it instantiates the entire CClayerTreeHost and CCLayerTreeHostImpl universe in order to do so. Moreover, what call mocks aren't really that --- for example, we mock CCLayerTreeHost not really because we need its implementation, but rather because we need to hook CCProxy's calls to the host. So, our mock classes are more like wrappers or shims.

I propose we fix this, first by renaming this file to CCProxyTest. And then, by doing proper mock testing by making pure-virtual base classes for LTH and LTHI, e.g. CCLayerTreeHostBase, CCLayerTreeHostImplBase. Then, modify all the appropriate CCProxy subclasses to use only these base classes.

With that, we can actually make MockLayerTreeHost and MockLayerTreeHostImpl classes true mocks (rather than being wrappers now. This allows us to test the proxy in isolation of the rest of the system. I think this is a good thing...

A lot of the MockLayerTreeHostClient is still useful --- we need a real test of the CCLayerTreeHost and for that we will need a MockLayerTreeHostClient. So, lets take that code and put it in the CCLayerTreeHostTest.cpp file [after moving the existing file to CCProxyTest].

Similarly, we need a CCLayerTreeHostImplTest. The MockGraphicsContext stuff you have done to get the CCLayerTreeImpl going is key for that test, so lets move that to a CCLayerTreeHostImplTest.cpp.

Thoughts?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:103
> +{

Didn't CCLayerTreeHost already have a stop method? I might be getting my patches confused at this point...

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:124
>> +    TextureManager* textureManager = contentsTextureManager();
> 
> Needed because we don't currently create a TextureManager (should be fixable)

This is getting cleaned up by jamesr, https://bugs.webkit.org/show_bug.cgi?id=67440

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:206
>> +        m_proxy->setNeedsCommitAndRedraw();
> 
> Again, needed for clean test shutdown. If stop() was called, m_proxy is null.

I actually think this should be a ASSERT(m_proxy) and whoever was calling setNeedsCommit should be more careful.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:216
> +    if (m_proxy)

Same comment as above.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:184
>> +    postBeginFrameToMainThread();
> 
> Cheesy implementation of the FIXME. Should use CCMainThread.

I think I can land my CCThreadProxy changes. That way you don't have to change this file, I hope.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:188
> +#if USE(ACCELERATED_COMPOSITING)

Oh, ahha, this is actually safe you probably dont need to do this.

WebGL contexs get their own GraphicsContext3D --- totally separate from the context from the compositor's graphicscontext. A compositor context will never call this method... (though you could put an ASSERT(isMainThread()) if you want to be safe.

>> Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:317

> 
> Blah, lazy initialization method is const!

Probably dont need this once you get rid of the lazy init guard.

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:32
>> +#include <wtf/HashTraits.h>
> 
> Had to #include these directly as there's some weird problem with the header order in GraphicsContext3DPrivate.h. It ends up including StringHash.h first, which breaks the HashTraits<String> template.

Can we fix that GraphicsContext3DPrivate header? I dont think we can commit this.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:123
> +      CCLayerTreeHostTest* test = static_cast<CCLayerTreeHostTest*>(self);

Guard against stop() here so you dont have to make m_layerTreeHost->setNeedsCommitAndRedraw to be safely callable after stop()?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:132
> +      CCLayerTreeHostTest* test = static_cast<CCLayerTreeHostTest*>(self);

Guard against stop() here...

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:180
>> +    virtual bool makeContextCurrent() { return true; }
> 
> These are the methods that the CC tree checks when setting up.

We're going to use this class a LOT of future tests.

MyMockContext->MockGraphicsContextForLayerRenderer or something inane like that and pull it out to a standalone .h?

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