Move VISUAL_STATE promise to activation

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 51ea3d91a69aba3cdad2746c61acb6849d9c2e2f..42c6907acd0b9594533d7896cf7e34447ef7fc84 100644
--- a/cc/trees/layer_tree_host_unittest.cc
+++ b/cc/trees/layer_tree_host_unittest.cc
@@ -4927,11 +4927,13 @@ SINGLE_AND_MULTI_THREAD_TEST_F(
 
 struct TestSwapPromiseResult {
   TestSwapPromiseResult()
-      : did_swap_called(false),
+      : did_activate_called(false),
+        did_swap_called(false),
         did_not_swap_called(false),
         dtor_called(false),
-        reason(SwapPromise::DID_NOT_SWAP_UNKNOWN) {}
+        reason(SwapPromise::COMMIT_FAILS) {}
 
+  bool did_activate_called;
   bool did_swap_called;
   bool did_not_swap_called;
   bool dtor_called;
@@ -4948,8 +4950,17 @@ class TestSwapPromise : public SwapPromise {
     result_->dtor_called = true;
   }
 
+  void DidActivate() override {
+    base::AutoLock lock(result_->lock);
+    EXPECT_FALSE(result_->did_activate_called);
+    EXPECT_FALSE(result_->did_swap_called);
+    EXPECT_FALSE(result_->did_not_swap_called);
+    result_->did_activate_called = true;
+  }
+
   void DidSwap(CompositorFrameMetadata* metadata) override {
     base::AutoLock lock(result_->lock);
+    EXPECT_TRUE(result_->did_activate_called);
     EXPECT_FALSE(result_->did_swap_called);
     EXPECT_FALSE(result_->did_not_swap_called);
     result_->did_swap_called = true;
@@ -4959,6 +4970,9 @@ class TestSwapPromise : public SwapPromise {
     base::AutoLock lock(result_->lock);
     EXPECT_FALSE(result_->did_swap_called);
     EXPECT_FALSE(result_->did_not_swap_called);
+    auto implies = [](bool a, bool b) -> bool { return !a || b; };
+    EXPECT_TRUE(implies(reason == DidNotSwapReason::ACTIVATION_FAILS,

[boliu, 2015/05/14 14:40:21]

That's overkill. Can you just write it as EXPECT_TRUE(!foo || bar) then

[Tobias Sargeant, 2015/05/14 21:40:48]

Yes, but then we're almost back to where we started.

IMO, the following is the most legible:

EXPECT_FALSE(result_->did_activate_called && reason ==
DidNotSwapReason::ACTIVATION_FAILS);

+                        !result_->did_activate_called));
     result_->did_not_swap_called = true;
     result_->reason = reason;
   }
@@ -4992,6 +5006,22 @@ class LayerTreeHostTestBreakSwapPromise : public LayerTreeHostTest {
     }
   }
 
+  void WillActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    if (host_impl->pending_tree()) {

[boliu, 2015/05/14 14:40:21]

Is this check the same as "host_impl->settings().impl_side_painting". Prefer to
use that, and assert the pending tree is there if impl side painting is on.

[Tobias Sargeant, 2015/05/14 21:40:48]

No, it's not the same.

If you assert that pending_tree is not null, the following tests fail:

8 tests failed:
   
LayerTreeHostTestBreakSwapPromise.RunMultiThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestBreakSwapPromise.RunMultiThread_DirectRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunMultiThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunMultiThread_DirectRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DelegatingRenderer_ImplSidePaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DirectRenderer_ImplSidePaint
   
LayerTreeHostTestKeepSwapPromise.RunSingleThread_DirectRenderer_MainThreadPaint

[boliu, 2015/05/15 03:50:49]

You should DCHECK pending_tree() only if impl_side_painting is true. So only the
*_ImplSidePaint failures are relevant. I'm not sure why those ones are failing
though. Can you look into it?

I guess LayerTreeHostImpl::ActivateSyncTree only checks for pending_tree()
instead of impl_side_painting, so maybe it's good enough.

+      int frame = host_impl->pending_tree()->source_frame_number();
+      base::AutoLock lock(swap_promise_result_[frame].lock);
+      EXPECT_FALSE(swap_promise_result_[frame].did_activate_called);
+      EXPECT_FALSE(swap_promise_result_[frame].did_swap_called);
+    }
+  }
+
+  void DidActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    int frame = host_impl->active_tree()->source_frame_number();
+    base::AutoLock lock(swap_promise_result_[frame].lock);
+    EXPECT_TRUE(swap_promise_result_[frame].did_activate_called);
+    EXPECT_FALSE(swap_promise_result_[frame].did_swap_called);
+  }
+
   void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) override {
     commit_complete_count_++;
     if (commit_complete_count_ == 1) {
@@ -5019,6 +5049,7 @@ class LayerTreeHostTestBreakSwapPromise : public LayerTreeHostTest {
     {
       // The second commit is aborted since it contains no updates.
       base::AutoLock lock(swap_promise_result_[1].lock);
+      EXPECT_FALSE(swap_promise_result_[1].did_activate_called);
       EXPECT_FALSE(swap_promise_result_[1].did_swap_called);
       EXPECT_TRUE(swap_promise_result_[1].did_not_swap_called);
       EXPECT_EQ(SwapPromise::COMMIT_NO_UPDATE, swap_promise_result_[1].reason);
@@ -5029,6 +5060,7 @@ class LayerTreeHostTestBreakSwapPromise : public LayerTreeHostTest {
       // The last commit completes but it does not cause swap buffer because
       // there is no damage in the frame data.
       base::AutoLock lock(swap_promise_result_[2].lock);
+      EXPECT_TRUE(swap_promise_result_[2].did_activate_called);
       EXPECT_FALSE(swap_promise_result_[2].did_swap_called);
       EXPECT_TRUE(swap_promise_result_[2].did_not_swap_called);
       EXPECT_EQ(SwapPromise::SWAP_FAILS, swap_promise_result_[2].reason);
@@ -5078,6 +5110,23 @@ class LayerTreeHostTestKeepSwapPromise : public LayerTreeTest {
     }
   }
 
+  void WillActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    if (host_impl->pending_tree() &&
+        host_impl->pending_tree()->source_frame_number() == 1) {
+      base::AutoLock lock(swap_promise_result_.lock);
+      EXPECT_FALSE(swap_promise_result_.did_activate_called);
+      EXPECT_FALSE(swap_promise_result_.did_swap_called);
+    }
+  }
+
+  void DidActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    if (host_impl->active_tree()->source_frame_number() == 1) {
+      base::AutoLock lock(swap_promise_result_.lock);
+      EXPECT_TRUE(swap_promise_result_.did_activate_called);
+      EXPECT_FALSE(swap_promise_result_.did_swap_called);
+    }
+  }
+
   void SwapBuffersOnThread(LayerTreeHostImpl* host_impl, bool result) override {
     EXPECT_TRUE(result);
     if (host_impl->active_tree()->source_frame_number() >= 1) {
@@ -5127,6 +5176,7 @@ class LayerTreeHostTestBreakSwapPromiseForVisibility
   void AfterTest() override {
     {
       base::AutoLock lock(swap_promise_result_.lock);
+      EXPECT_FALSE(swap_promise_result_.did_activate_called);
       EXPECT_FALSE(swap_promise_result_.did_swap_called);
       EXPECT_TRUE(swap_promise_result_.did_not_swap_called);
       EXPECT_EQ(SwapPromise::COMMIT_FAILS, swap_promise_result_.reason);
@@ -5176,6 +5226,7 @@ class LayerTreeHostTestBreakSwapPromiseForContext : public LayerTreeHostTest {
   void AfterTest() override {
     {
       base::AutoLock lock(swap_promise_result_.lock);
+      EXPECT_FALSE(swap_promise_result_.did_activate_called);
       EXPECT_FALSE(swap_promise_result_.did_swap_called);
       EXPECT_TRUE(swap_promise_result_.did_not_swap_called);
       EXPECT_EQ(SwapPromise::COMMIT_FAILS, swap_promise_result_.reason);
@@ -5829,6 +5880,7 @@ class LayerTreeHostTestSynchronousCompositeSwapPromise
     // Second swap promise fails to swap.
     {
       base::AutoLock lock(swap_promise_result_[1].lock);
+      EXPECT_TRUE(swap_promise_result_[1].did_activate_called);
       EXPECT_FALSE(swap_promise_result_[1].did_swap_called);
       EXPECT_TRUE(swap_promise_result_[1].did_not_swap_called);
       EXPECT_EQ(SwapPromise::SWAP_FAILS, swap_promise_result_[1].reason);
@@ -5838,6 +5890,7 @@ class LayerTreeHostTestSynchronousCompositeSwapPromise
     // Third swap promises also fails to swap (and draw).
     {
       base::AutoLock lock(swap_promise_result_[2].lock);
+      EXPECT_TRUE(swap_promise_result_[2].did_activate_called);
       EXPECT_FALSE(swap_promise_result_[2].did_swap_called);
       EXPECT_TRUE(swap_promise_result_[2].did_not_swap_called);
       EXPECT_EQ(SwapPromise::SWAP_FAILS, swap_promise_result_[2].reason);