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..902281943c058cc67c78e1433ac43974ee24c980 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,60 @@ class LayerTreeHostTestContinuousPainting : public LayerTreeHostTest {
 
 MULTI_THREAD_TEST_F(LayerTreeHostTestContinuousPainting);
 
+class LayerTreeHostTestWillBeginImplFrameHasDidBeginImplFrameDeadline
+    : public LayerTreeHostTest {
+ public:
+  enum { kExpectedNumImplFrames = 10 };
+
+  LayerTreeHostTestWillBeginImplFrameHasDidBeginImplFrameDeadline()
+      : will_begin_impl_frame_count_(0),
+        did_begin_impl_frame_deadline_count_(0) {}
+
+  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_begin_impl_frame_deadline_count_);
+    EXPECT_FALSE(TestEnded());
+    will_begin_impl_frame_count_++;
+  }
+
+  void DidBeginImplFrameDeadlineOnThread(
+      LayerTreeHostImpl* host_impl) override {
+    EXPECT_EQ(will_begin_impl_frame_count_,
+              did_begin_impl_frame_deadline_count_ + 1);

[danakj, 2015/04/30 18:43:38]

nit: reverse the order of the increment with this comparison and you can just
compare EQ instead of EQ+1 which is a bit nicer to read

[mithro-old, 2015/05/01 02:52:35]

Done. I was thinking of it like "do the checks first, then do any updates".

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

[danakj, 2015/04/30 18:43:38]

can you say why?

[mithro-old, 2015/05/01 02:52:35]

Done.

+    if (did_begin_impl_frame_deadline_count_ < kExpectedNumImplFrames - 1)

[danakj, 2015/04/30 18:43:38]

else EndTest()?

[mithro-old, 2015/05/01 02:52:35]

Putting the EndTest inside DidBeginImplFrameDeadline makes the final AfterCheck
begin/end equality test impossible because there becomes a race condition
between ending the test and starting another frame. For the same reason checking
the actual number of frames processed becomes flaky too.

[danakj, 2015/05/04 17:42:04]

Oh, we should make LayerTreeTest::RealEndTest not end the test while an impl
frame is still in progress, the same way it doesn't while a commit is in
progress/requested.

Can you leave a TODO here to fix that and move the EndTest() here?

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

I added the TODO inside LayerTreeTest::RealEndTest() method.

I actually think using the SendBeginMainFrameNotExpectedSoon is the right thing
here because it means this code is checking that no extra impl frames are
triggered after we stop requesting commits.

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

I disagree, I think the test should EndTest() when it is logically done, and the
harness should not exit until there are no more impl frames happening. This way
we see extra impl frames clearly in tests instead of just hiding them and
waiting for them like this.

We have the same expectations/behaviour around commits and catching extra
commits in tests has been very useful and prevented regressions.

I think SendBeginMainFrameNotExpectedSoon() probably doesn't need to be exposed
to the tests at all.

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

I agree that the test should call EndTest() when it is logically done. I think
we disagree with when "logically done" occurs.

This test is "logically done" when all impl frames are finished and no more are
going to occur. **Not** when we have the expected number of impl frames.
I believe this is impossible to detect 100% correctly and especially impossible
to detect without intimate knowledge of how impl frames work. If you want to try
write some logic, I bet that I can come up with a scenario where it will exit
too early, missing some real impl frames that would have occurred.
I'm not hiding anything here. In the AfterTest() we clearly check that the
number of frames we received was the number of frames we expected.

The fact these checks don't work for the multithread version is showing that
there is a real race condition or other bug in the system somewhere.
I agree that checking the number of commits expected it useful. 

I'm not confident that you are catching all the issues as expected with your
current method. For example, looking at EndTest() it is currently dependent on
only Scheduler::MainFrameForTestingWillHappen which knows nothing about what
operations are currently in the message queue. These pending actions **could**
make MainFrameForTestingWillHappen become true again but we have already exited
the test. You can't make this reliable by adding a test to check if the
MessageQueue is empty either because there are plenty of cases where it won't be
empty.
I think that SendBeginMainFrameNotExpectedSoon is the best compromise because it
is a signal from the scheduler that it thinks it is finished and detects the
following issues;
  * If this message occurs before the expected number of frames, then it's a bug
in the scheduler logic. 
  * If there are more than the expected number of frames, then it is again a
bug. 

The only bug it misses is where the scheduler calls
SendBeginMainFrameNotExpectedSoon at exactly the right number of frames while
there are still frames pending. Due to the async nature of the system this would
make the test flaky because there is no reliable way to make this situation
occur even if we wanted too.

+      PostSetNeedsCommitToMainThread();
+  }
+
+  void SendBeginMainFrameNotExpectedSoon() override { EndTest(); }

[danakj, 2015/04/30 18:43:38]

prefer to do this as else to PostSNC

[mithro-old, 2015/05/01 02:52:35]

Using SendBeginMainFrameNotExpectedSoon guarantees that we are not going to get
any more ImplFrames and that the state is now idle. As there is no way in the
test to get out of the idle state, this removes the race condition problem.

+
+  void AfterTest() override {
+    EXPECT_EQ(will_begin_impl_frame_count_,
+              did_begin_impl_frame_deadline_count_);
+    // TODO(mithro): Figure out why the multithread version of this test has

[sunnyps, 2015/05/01 00:31:43]

I've had issue before too - I think it has something to do with activation and
the scheduler's deadline racing in the multithread version i.e. activation
doesn't happen in time for the deadline.

In any case, instead of expecting a fixed number of frames why not wait until a
full frame is drawn and then check if the number of WillBeginImplFrames is equal
to DidBeginImplFrameDeadlines and not worry about the total number of frames.

[mithro-old, 2015/05/01 02:52:35]

The number of WillBeginImplFrames equal to the DidBeginImplFrameDeadlines check
is on line 5534.

The reason I want to check the final count is because the number of frames
should be deterministic. The fact it is behaves differently on the multithread
verse single thread proxy. This is a real bug somewhere (either in our test
framework, or in real code).

+    // one more frame then expected.Possibly related to

[danakj, 2015/04/30 18:43:38]

spacing

[mithro-old, 2015/05/01 02:52:35]

Done.

+    // http://crbug.com/443185
+    // EXPECT_EQ(will_begin_impl_frame_count_, kExpectedNumImplFrames);

[danakj, 2015/04/30 18:43:38]

You can ask if there's a thread and compare against different values here
instead of just not checking

[mithro-old, 2015/05/01 02:52:35]

Done. I'm now checking the value where it is correct. I'm leaving the TODO and
not checking the value on the threaded version because the value is flaky.

+    // EXPECT_EQ(did_begin_impl_frame_deadline_count_, kExpectedNumImplFrames);
+  }
+
+ private:
+  int will_begin_impl_frame_count_;
+  int did_begin_impl_frame_deadline_count_;
+};
+
+SINGLE_AND_MULTI_THREAD_TEST_F(
+    LayerTreeHostTestWillBeginImplFrameHasDidBeginImplFrameDeadline);
+
 class LayerTreeHostTestSendBeginFramesToChildren : public LayerTreeHostTest {
  public:
   LayerTreeHostTestSendBeginFramesToChildren()