Mailbox support for texture layers.

cc/texture_layer_unittest.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>

[danakj, 2013/01/04 15:10:35]

put this between cc/texture_layer.h and the rest of the headers (with spaces)

+
+#include "base/callback.h"

[danakj, 2013/01/04 15:10:35]

move this header to the top of the next group of headers.

 #include "cc/texture_layer.h"
 
 #include "cc/layer_tree_host.h"
 #include "cc/single_thread_proxy.h"
 #include "cc/test/fake_impl_proxy.h"
 #include "cc/test/fake_layer_tree_host_client.h"
 #include "cc/test/fake_layer_tree_host_impl.h"
+#include "cc/test/layer_tree_test_common.h"
 #include "cc/texture_layer_impl.h"
 #include "cc/thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::Mock;
 using ::testing::_;
 using ::testing::AtLeast;
 using ::testing::AnyNumber;
 
@@ skipping 71 matching lines @@
     Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
 }
 
 TEST_F(TextureLayerTest, syncImplWhenDrawing)
 {
     gfx::RectF dirtyRect(0, 0, 1, 1);
 
     scoped_refptr<TextureLayer> testLayer = TextureLayer::create(0);
     ASSERT_TRUE(testLayer);
     scoped_ptr<TextureLayerImpl> implLayer;
-    implLayer = TextureLayerImpl::create(m_hostImpl.activeTree(), 1);
+    implLayer = TextureLayerImpl::create(m_hostImpl.activeTree(), 1, false);
     ASSERT_TRUE(implLayer);
 
     EXPECT_CALL(*m_layerTreeHost, acquireLayerTextures()).Times(AnyNumber());
     EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AnyNumber());
     m_layerTreeHost->setRootLayer(testLayer);
     testLayer->setTextureId(1);
     testLayer->setIsDrawable(true);
     Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
     EXPECT_EQ(testLayer->layerTreeHost(), m_layerTreeHost.get());
 
@@ skipping 63 matching lines @@
     EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AtLeast(1));
     testLayer->setTextureId(1);
     Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
 
     EXPECT_CALL(*m_layerTreeHost, acquireLayerTextures()).Times(AtLeast(1));
     EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AtLeast(1));
     testLayer->removeFromParent();
     Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
 }
 
+class MockMailboxCallback {
+public:
+    MOCK_METHOD2(Release, void(const std::string& mailbox, unsigned syncPoint));
+};
+
+struct CommonMailboxObjects {
+    CommonMailboxObjects()
+        : m_mailbox1(64, '1')
+        , m_mailbox2(64, '2')
+    {
+        m_releaseMailbox1 = base::Bind(&MockMailboxCallback::Release,
+                                       base::Unretained(&m_mockCallback),
+                                       m_mailbox1);
+        m_releaseMailbox2 = base::Bind(&MockMailboxCallback::Release,
+                                       base::Unretained(&m_mockCallback),
+                                       m_mailbox2);
+    }
+
+    std::string m_mailbox1;
+    std::string m_mailbox2;
+    MockMailboxCallback m_mockCallback;
+    base::Callback<void(unsigned)> m_releaseMailbox1;
+    base::Callback<void(unsigned)> m_releaseMailbox2;
+};
+
+TEST_F(TextureLayerTest, testImplLayerCallbacks)
+{
+    CommonMailboxObjects m_testData;
+    {

[danakj, 2013/01/04 15:10:35]

the extra scoping/indent here doesn't seem useful?

[alexst (slow to review), 2013/01/04 16:20:48]

It's to ensure the object receiving callbacks outlives the layer since I am
testing callbacks from the destructor.

On 2013/01/04 15:10:35, danakj wrote:

[danakj, 2013/01/04 21:36:27]

I see. Can you add a quick comment here to that effect so it doesn't get
refactored into a flaky crash or something in the future?

+        scoped_ptr<TextureLayerImpl> implLayer;
+        implLayer = TextureLayerImpl::create(m_hostImpl.activeTree(), 1, true);
+        ASSERT_TRUE(implLayer);
+
+        // Test setting identical mailbox.
+        EXPECT_CALL(m_testData.m_mockCallback, Release(_, _)).Times(0);
+        implLayer->setTextureMailbox(m_testData.m_mailbox1,
+                                     m_testData.m_releaseMailbox1);
+        implLayer->setTextureMailbox(m_testData.m_mailbox1,
+                                     m_testData.m_releaseMailbox1);
+        Mock::VerifyAndClearExpectations(&m_testData.m_mockCallback);
+
+        // Test multiple commits without a draw.
+        EXPECT_CALL(m_testData.m_mockCallback,
+                    Release(m_testData.m_mailbox1, _)).Times(1);
+        implLayer->setTextureMailbox(m_testData.m_mailbox2,
+                                     m_testData.m_releaseMailbox2);
+        Mock::VerifyAndClearExpectations(&m_testData.m_mockCallback);
+
+        // Test resetting the mailbox.

[danakj, 2013/01/04 15:10:35]

How about the case when the resource from the mailbox is in use by a consumer
(the browser)? Can we test that too?

[danakj, 2013/01/04 21:36:27]

Do you think this is something we can test here? Maybe call
prepareSendToParent() with the texture, then set a new mailbox and/or delete the
layer, and make sure the callback happens on receiveFromParent()?

+        EXPECT_CALL(m_testData.m_mockCallback,
+                    Release(m_testData.m_mailbox2, _)).Times(1);
+        implLayer->setTextureMailbox(std::string(),
+                                     base::Callback<void(unsigned)>());
+        Mock::VerifyAndClearExpectations(&m_testData.m_mockCallback);
+
+        // Test destructor.
+        EXPECT_CALL(m_testData.m_mockCallback,
+                    Release(m_testData.m_mailbox1, _)).Times(1);

[danakj, 2013/01/04 15:10:35]

Since you just cleared it, how will this happen?

Without the VerifyAndClearExpectations after, is this being checked?

[alexst (slow to review), 2013/01/04 16:20:48]

This next expectation is for mailbox1 set below, that gets triggered on
destruction.

On 2013/01/04 15:10:35, danakj wrote:

+        implLayer->setTextureMailbox(m_testData.m_mailbox1,
+                                     m_testData.m_releaseMailbox1);
+    }
+}
+
+class TextureLayerWithMailboxTest : public TextureLayerTest {
+protected:
+    virtual void TearDown()
+    {
+        Mock::VerifyAndClearExpectations(&m_testData.m_mockCallback);
+        EXPECT_CALL(m_testData.m_mockCallback,
+                    Release(m_testData.m_mailbox1, _)).Times(1);
+        TextureLayerTest::TearDown();
+    }
+
+    CommonMailboxObjects m_testData;
+};
+
+TEST_F(TextureLayerWithMailboxTest, syncImplAndGetCallback)

[danakj, 2013/01/04 15:10:35]

I think this test name is wrong, it is not doing anything with impl in this test
at all AFAICT.

I think it's more like replaceMailboxOnMainThreadBeforeCommit.

[alexst (slow to review), 2013/01/04 16:20:48]

Done.

+{
+    scoped_refptr<TextureLayer> testLayer = TextureLayer::createForMailbox();
+    ASSERT_TRUE(testLayer);
+
+    EXPECT_CALL(*m_layerTreeHost, acquireLayerTextures()).Times(0);
+    EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AnyNumber());
+    m_layerTreeHost->setRootLayer(testLayer);
+    Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
+
+    EXPECT_CALL(*m_layerTreeHost, acquireLayerTextures()).Times(0);
+    EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AtLeast(1));
+    EXPECT_CALL(m_testData.m_mockCallback,
+                Release(m_testData.m_mailbox1, _)).Times(1);

[danakj, 2013/01/04 15:10:35]

Seems like this EXPECT should move to line 287?

[alexst (slow to review), 2013/01/04 16:20:48]

Done.

+    testLayer->setTextureMailbox(m_testData.m_mailbox1,
+                                 m_testData.m_releaseMailbox1);
+    Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
+
+    EXPECT_CALL(*m_layerTreeHost, acquireLayerTextures()).Times(0);
+    EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AtLeast(1));
+
+    testLayer->setTextureMailbox(m_testData.m_mailbox2,
+                                 m_testData.m_releaseMailbox2);
+    Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
+    Mock::VerifyAndClearExpectations(&m_testData.m_mockCallback);
+
+    EXPECT_CALL(*m_layerTreeHost, acquireLayerTextures()).Times(0);
+    EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AtLeast(1));
+    EXPECT_CALL(m_testData.m_mockCallback,
+                Release(m_testData.m_mailbox2, _)).Times(1);
+    testLayer->setTextureMailbox(std::string(),
+                                 base::Callback<void(unsigned)>());
+    Mock::VerifyAndClearExpectations(m_layerTreeHost.get());
+    Mock::VerifyAndClearExpectations(&m_testData.m_mockCallback);
+
+    // Test destructor.
+    EXPECT_CALL(*m_layerTreeHost, setNeedsCommit()).Times(AtLeast(1));
+    testLayer->setTextureMailbox(m_testData.m_mailbox1,
+                                 m_testData.m_releaseMailbox1);
+}
+
+class TextureLayerImplWithMailbox : public ThreadedTest {
+public:
+    TextureLayerImplWithMailbox()
+        : m_resetMailbox(false)
+    {
+    }
+
+    // Make sure callback is received on main and doesn't block the impl thread.
+    void releaseCallback(unsigned syncPoint) {
+        EXPECT_EQ(proxy()->isMainThread(), true);

[danakj, 2013/01/04 15:10:35]

nit: (expected value, actual value)

Otherwise the output reads backward when it fails.

+        endTest();
+    }
+
+    virtual void beginTest() OVERRIDE
+    {
+        m_layerTreeHost->initializeRendererIfNeeded();

[danakj, 2013/01/04 15:10:35]

I don't think you should need this line?

[alexst (slow to review), 2013/01/04 16:20:48]

Done.

+
+        m_layer = TextureLayer::createForMailbox();
+        m_layer->setIsDrawable(true);
+        m_layerTreeHost->setRootLayer(m_layer);
+        m_layer->setTextureMailbox(
+            std::string(64, '1'),
+            base::Bind(&TextureLayerImplWithMailbox::releaseCallback,
+                base::Unretained(this)));
+        postSetNeedsCommitToMainThread();
+    }
+
+    virtual void didCommit() OVERRIDE {

[danakj, 2013/01/04 15:10:35]

nit: { on new line

[alexst (slow to review), 2013/01/04 16:20:48]

Done.

+        if (m_resetMailbox)
+            return;
+
+        m_layer->setTextureMailbox(std::string(),
+                                   base::Callback<void(unsigned)>());
+        m_resetMailbox = true;
+    }
+
+    virtual void afterTest() OVERRIDE {

[danakj, 2013/01/04 15:10:35]

nit: { on new line

[alexst (slow to review), 2013/01/04 16:20:48]

Done.

+    }
+
+private:
+    bool m_resetMailbox;
+    scoped_refptr<TextureLayer> m_layer;
+};
+
+MULTI_THREAD_TEST_F(TextureLayerImplWithMailbox);

[danakj, 2013/01/04 15:10:35]

would this work as SINGLE_AND_MULTI_THREAD_TEST_F? If so I prefer that.

[alexst (slow to review), 2013/01/04 16:20:48]

Done.

+
 }  // namespace
 }  // namespace cc