cc: Adding DidFinishImplFrame to LTHI.

cc/trees/layer_tree_host_unittest.cc

Index: cc/trees/layer_tree_host_unittest.cc
diff --git a/cc/trees/layer_tree_host_unittest.cc b/cc/trees/layer_tree_host_unittest.cc
index be879af1384d097e5b7d9ef53ec4ea1603c10248..a1676124ba25e769813e004e3acdfe202412b718 100644
--- a/cc/trees/layer_tree_host_unittest.cc
+++ b/cc/trees/layer_tree_host_unittest.cc
@@ -2283,7 +2283,8 @@ class LayerTreeHostTestDeferCommits : public LayerTreeHostTest {
 
   void BeginTest() override { PostSetNeedsCommitToMainThread(); }
 
-  void WillBeginImplFrame(const BeginFrameArgs& args) override {
+  void WillBeginImplFrameOnThread(LayerTreeHostImpl* host_impl,
+                                  const BeginFrameArgs& args) override {
     num_will_begin_impl_frame_++;
     switch (num_will_begin_impl_frame_) {
       case 1:
@@ -5493,6 +5494,72 @@ class LayerTreeHostTestContinuousPainting : public LayerTreeHostTest {
 
 MULTI_THREAD_TEST_F(LayerTreeHostTestContinuousPainting);
 
+class LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame
+    : public LayerTreeHostTest {
+ public:
+  enum { kExpectedNumImplFrames = 10 };
+
+  LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame()
+      : threaded_(false),
+        will_begin_impl_frame_count_(0),
+        did_finish_impl_frame_count_(0) {}
+
+  void RunTest(bool threaded,
+               bool delegating_renderer,
+               bool impl_side_painting) override {
+    threaded_ = threaded;
+    LayerTreeHostTest::RunTest(threaded, delegating_renderer,
+                               impl_side_painting);
+  }
+
+  void BeginTest() override {
+    // Kick off the test with a commit.
+    PostSetNeedsCommitToMainThread();
+  }
+
+  void WillBeginImplFrameOnThread(LayerTreeHostImpl* host_impl,
+                                  const BeginFrameArgs& args) override {
+    EXPECT_EQ(will_begin_impl_frame_count_, did_finish_impl_frame_count_);
+    EXPECT_FALSE(TestEnded());
+    will_begin_impl_frame_count_++;
+  }
+
+  void DidFinishImplFrameOnThread(LayerTreeHostImpl* host_impl) override {
+    did_finish_impl_frame_count_++;
+    EXPECT_EQ(will_begin_impl_frame_count_, did_finish_impl_frame_count_);
+
+    // Request a number of commits to cause multiple impl frames. We expect to
+    // get one more impl frames than the number of commits requested because
+    // after a commit it takes one frame to become idle.
+    if (did_finish_impl_frame_count_ < kExpectedNumImplFrames - 1)
+      PostSetNeedsCommitToMainThread();
+  }
+
+  void SendBeginMainFrameNotExpectedSoon() override { EndTest(); }
+
+  void AfterTest() override {
+    EXPECT_GT(will_begin_impl_frame_count_, 0);
+    EXPECT_GT(did_finish_impl_frame_count_, 0);
+    EXPECT_EQ(will_begin_impl_frame_count_, did_finish_impl_frame_count_);
+
+    // TODO(mithro): Figure out why the multithread version of this test
+    // sometimes has one more frame then expected. Possibly related to
+    // http://crbug.com/443185
+    if (!threaded_) {

[danakj, 2015/05/04 17:39:19]

You can just do HasImplThread() there's a function on LayerTreeTest. No need to
store threaded_ in this class.

[mithro-old, 2015/05/05 04:13:13]

I tried to use HasImplThread() here but it seems to mean something different
then the threaded_ argument (which seems to determine when the extra frame
occurs).

I added a check that EXPECT_EQ(threaded_, HasImplThread()) and got the following
errors;
------------------------------------------------------------
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_MainThreadPaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_MainThreadPaint
(55 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_ImplSidePaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_ImplSidePaint
(56 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_MainThreadPaint
[       OK ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_MainThreadPaint
(52 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_ImplSidePaint
[       OK ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_ImplSidePaint
(50 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_MainThreadPaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_MainThreadPaint
(50 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_ImplSidePaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_ImplSidePaint
(56 ms)
------------------------------------------------------------

Looking at the code in LayerTreeTest::RunTest, it does a;
------------------------------------------------------------
  if (threaded) {
    impl_thread_.reset(new base::Thread("Compositor"));
    ASSERT_TRUE(impl_thread_->Start());
  }
------------------------------------------------------------

While HasImplThread() has the following definition;
------------------------------------------------------------
  bool HasImplThread() { return proxy() ? proxy()->HasImplThread() : false; }

  bool Proxy::HasImplThread() const { return !!impl_task_runner_.get(); }
------------------------------------------------------------

Also looking at the macro definitions;

threaded == false
------------------------------------------------------------------
#define SINGLE_THREAD_DIRECT_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunSingleThread_DirectRenderer_MainThreadPaint) { \
    RunTest(false, false, false);                                             \
  }                                                                           \
  class SingleThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define SINGLE_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunSingleThread_DirectRenderer_ImplSidePaint) { \
    RunTest(false, false, true);                                            \
  }                                                                         \
  class SingleThreadDirectImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define SINGLE_THREAD_DELEGATING_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME) \
  TEST_F(TEST_FIXTURE_NAME,                                                \
         RunSingleThread_DelegatingRenderer_MainThreadPaint) {             \
    RunTest(false, true, false);                                           \
  }                                                                        \
  class SingleThreadDelegatingNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define SINGLE_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME) \
  TEST_F(TEST_FIXTURE_NAME,                                              \
         RunSingleThread_DelegatingRenderer_ImplSidePaint) {             \
    RunTest(false, true, true);                                          \
  }                                                                      \
  class SingleThreadDelegatingImplNeedsSemicolon##TEST_FIXTURE_NAME {}

------------------------------------------------------------------


threaded == true
------------------------------------------------------------------
#define MULTI_THREAD_DIRECT_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DirectRenderer_MainThreadPaint) { \
    RunTest(true, false, false);                                             \
  }                                                                          \
  class MultiThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define MULTI_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DirectRenderer_ImplSidePaint) { \
    RunTest(true, false, true);                                            \
  }                                                                        \
  class MultiThreadDirectImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define MULTI_THREAD_DELEGATING_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME) \
  TEST_F(TEST_FIXTURE_NAME,                                               \
         RunMultiThread_DelegatingRenderer_MainThreadPaint) {             \
    RunTest(true, true, false);                                           \
  }                                                                       \
  class MultiThreadDelegatingNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define MULTI_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DelegatingRenderer_ImplSidePaint) { \
    RunTest(true, true, true);                                                 \
  }                                                                            \
  class MultiThreadDelegatingImplNeedsSemicolon##TEST_FIXTURE_NAME {}
------------------------------------------------------------------

[danakj, 2015/05/05 18:42:00]

It looks to me that your threaded_ is always false. They match for
RunSingleThread, and for RunMultiThread there is a thread (HasImplThread() is
true) but your threaded_ variable is false?
When there's a thread, a task runner is given to the proxy. HasImplThread()
means the test is threaded.

[mithro-old, 2015/05/06 02:14:11]

I tracked down the issue and uploaded a patch at
https://codereview.chromium.org/1126973002

tl;dr - HasImplThread() function returned the incorrect value in AfterTest().

+      EXPECT_EQ(will_begin_impl_frame_count_, kExpectedNumImplFrames);
+      EXPECT_EQ(did_finish_impl_frame_count_, kExpectedNumImplFrames);
+    }
+  }
+
+ private:
+  bool threaded_;
+  int will_begin_impl_frame_count_;
+  int did_finish_impl_frame_count_;
+};
+
+SINGLE_AND_MULTI_THREAD_TEST_F(
+    LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame);
+
 class LayerTreeHostTestSendBeginFramesToChildren : public LayerTreeHostTest {
  public:
   LayerTreeHostTestSendBeginFramesToChildren()