Bug 67418 – [chromium] Fix CCLayerTreeHostTest

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nat Duca]

CCLayerTreeHosttest exists but doesn't run

[Nat Duca]

In retrospect, maybe we should fix the test first, then make any overhaul changes. :)

[Iain Merrick]

Created attachment 106759 [details]
Patch

[Iain Merrick]

Hacky patch that gets the test passing. I'll upload a tidier version ASAP.

[Iain Merrick]

Comment on attachment 106759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106759&action=review

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:123
> +#if !USE(THREADED_COMPOSITING)

This was needed because LayerTextureUpdaterSkPicture isn't compiled in -- similar #ifndef in LayerTextureUpdaterCanvas.cpp.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:43
> +PassOwnPtr<CCLayerTreeHostImpl> CCLayerTreeHostClient::createLayerTreeHostImpl(const CCSettings& settings)

Needed for the test to make its own HostImpl subclass.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:102
> +void CCLayerTreeHost::stop()

Needed this for a clean shutdown in the test:

- layerTreeHost->stop()
- RunAllPendingMessages()
- delete layerTreeHost

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:124
> +    TextureManager* textureManager = contentsTextureManager();

Needed because we don't currently create a TextureManager (should be fixable)

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:67
> +class CCLayerTreeHostClient {

Moved down because it now depends on CCSettings.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:52
> +    virtual void drawLayers();

Made this virtual so I can detect calls in the test.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:184
> +    postBeginFrameToMainThread();

Cheesy implementation of the FIXME. Should use CCMainThread.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:189
> +    if (!m_compositingLayer)

Lazy init so that  WebGLLayerChromium is created on the correct thread.

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

Blah, lazy initialization method is const!

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

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:83
> +    virtual void drawLayersOnCCThread(MockLayerTreeHostImpl* layerTreeHostImpl) { }

drawLayers() and present() now seem to be separate methods, let's ignore present() for now.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:90
> +        callOnMainThread(CCLayerTreeHostTest::dispatchSetNeedsCommit, this);

Should use CCMainThread

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:180
> +    virtual bool makeContextCurrent() { return true; }

These are the methods that the CC tree checks when setting up.

[WebKit Review Bot]

Attachment 106759 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Comment on attachment 106759 [details]
Patch

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?

[Iain Merrick]

Sorry, I let this slide while working on other stuff. https://bugs.webkit.org/show_bug.cgi?id=67440 has landed so I'll refresh this patch.

[Iain Merrick]

Created attachment 107493 [details]
Patch

[James Robinson]

Comment on attachment 107493 [details]
Patch

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

[Iain Merrick]

Comment on attachment 107493 [details]
Patch

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

[Iain Merrick]

Created attachment 107632 [details]
Patch

[Iain Merrick]

Comment on attachment 107632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107632&action=review

> Source/WebKit/chromium/ChangeLog:16
> +        (WTF::TestHooks::beginCommitOnCCThread):

Re-ran the changelog generator but these are still broken.

Should I just delete lines 16-56? They're in an anonymous namespace, not WTF.

[Nat Duca]

Recapping from a conversation offline:
- Lets remove the GraphicsContext3DChromium change, thats another CL, I can do it if it prevents the CCThreadProxy code from running, otherwise you can do it next week sometime
- Fix up the changelogs manually
- Use the create() design pattern
- Wrt the "I dont think you want to land these" changes to CCThreadProxy, lets leave those changes in and get this landed. I can rebase my CCThreadProxy changes onto this since its AM here and PM for Iain

[Iain Merrick]

Created attachment 107664 [details]
Patch

[Iain Merrick]

New patch with Nat's requested changes.

[James Robinson]

Comment on attachment 107664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107664&action=review

Good start.  I have some nits, but I can fix those on landing.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:220
> +    void doEndTest()

This function seems unreachable

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:263
> +            printf("Test timed out");
> +            FAIL() << "Test timed out";

Very weird to printf() and FAIL().  The latter should produce valid output, no?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:279
> +    Mutex m_tracesLock;
> +    Vector<std::string> m_traces;

We don't use std::string in WebKit code - we use WTF::String.  These seem completely unused, however.

[James Robinson]

Committed r95309: <http://trac.webkit.org/changeset/95309>

[David Levin]

Had to roll this out.  Sorry.
This seemed to break 80+ gpu tests.

Here's a *small* sampling:  http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2Fwebgl%2Fcanvas-test.html%2Cfast%2Fcanvas%2Fwebgl%2Fgl-pixelstorei.html%2Cfast%2Fcanvas%2Fwebgl%2Fgl-teximage.html&group=%40ToT%20GPU%20Mesa%20-%20chromium.org

[David Levin]

Note that the failures didn't happen 100% of the time. But there were a lot of them across several bots when this landed.

And then in another run many passed. It was very odd. But the coincidence was too great to call it unrelated.

[James Robinson]

Looking more closely we're pretty sure that http://src.chromium.org/viewvc/chrome?view=rev&revision=101545 caused the test failures.

[James Robinson]

Relanded http://trac.webkit.org/changeset/95320