cc: Use BeginFrameArgs

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 b18a88a7865e0f2e4ea6fc61ecc7e095ebcaa77e..ab851c3f798ff2d7b0d36d33bf12ed49a638194d 100644
--- a/cc/trees/layer_tree_host_unittest.cc
+++ b/cc/trees/layer_tree_host_unittest.cc
@@ -86,8 +86,8 @@ class LayerTreeHostTestSetNeedsCommit1 : public LayerTreeHostTest {
   }
 
   virtual void AfterTest() OVERRIDE {
-    EXPECT_GE(1, num_commits_);
-    EXPECT_GE(1, num_draws_);
+    EXPECT_GE(num_commits_, 1);

[enne (OOO), 2013/08/15 19:47:59]

I think this isn't what this test is looking for.  Given the comment above the
class, I'd expect this to be:

    EXPECT_EQ(1, num_commits_);
    EXPECT_GE(num_draws_, 1);

...although I don't see where a second draw would come from and it seems like
that should be EXPECT_EQ as well.

[brianderson, 2013/08/15 23:05:10]

I tried to make the code match the comments here ("at least 1 commit and at
least 1 draw"), but looking at the code I would expect exactly 1 of each as
well.  I made this change pretty early on, when my changes were likely
misbehaving, so I will change it to EXPECT_EQ and see how it goes.

+    EXPECT_GE(num_draws_, 1);
   }
 
  private:
@@ -622,7 +622,7 @@ class LayerTreeHostTestCompositeAndReadbackWhileInvisible
 
   virtual void BeginTest() OVERRIDE { PostSetNeedsCommitToMainThread(); }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     num_commits_++;
     if (num_commits_ == 1) {
       layer_tree_host()->SetVisible(false);
@@ -701,7 +701,8 @@ MULTI_THREAD_TEST_F(LayerTreeHostTestCommit);
 class LayerTreeHostTestFrameTimeUpdatesAfterActivationFails
     : public LayerTreeHostTest {
  public:
-  LayerTreeHostTestFrameTimeUpdatesAfterActivationFails() : frame_(0) {}
+  LayerTreeHostTestFrameTimeUpdatesAfterActivationFails()
+  : frame_count_with_pending_tree_(0) {}

[enne (OOO), 2013/08/15 19:47:59]

style nit: indentation.  Consider 'git cl format'-ing your change.

[brianderson, 2013/08/15 23:05:10]

Magic! Thanks for the pointer.

 
   virtual void BeginTest() OVERRIDE {
     layer_tree_host()->SetViewportSize(gfx::Size(20, 20));
@@ -710,9 +711,16 @@ class LayerTreeHostTestFrameTimeUpdatesAfterActivationFails
     PostSetNeedsCommitToMainThread();
   }
 
+  virtual void WillBeginFrameOnThread(LayerTreeHostImpl* host_impl,
+                              const BeginFrameArgs &args) OVERRIDE {
+    if (host_impl->pending_tree())
+      frame_count_with_pending_tree_++;
+  }
+
   virtual void DrawLayersOnThread(LayerTreeHostImpl* impl) OVERRIDE {
-    if (frame_ >= 1) {
-      EXPECT_NE(first_frame_time_, impl->CurrentFrameTimeTicks());
+    if (frame_count_with_pending_tree_ > 1) {

[enne (OOO), 2013/08/15 19:47:59]

Why do you need two begin frames before the first draw? I only see one
PostSetNeedsCommit; where does the second begin frame come from?

[brianderson, 2013/08/15 23:05:10]

I really need to clarify the uses of "BeginFrame" in the code. 
WillBeginFrameOnThread refers to the iml thread's BeginFrame not the commit's
BeginFrame.

In this test, the first BeginFrame on the impl thread does not activate or draw,
then the second BeginFrame activates and draws.

I should actually remove CanActivatePendingTreeIfNeeded from this test all
together since it should no longer be called after the activation changes in
more recent versions of this patch.

[enne (OOO), 2013/08/16 00:53:22]

OH.  One confusing begin frame name to rule them all.  :(

+      EXPECT_NE(first_frame_time_.ToInternalValue(),
+                impl->CurrentFrameTimeTicks().ToInternalValue());
       EndTest();
       return;
     }
@@ -722,33 +730,34 @@ class LayerTreeHostTestFrameTimeUpdatesAfterActivationFails
   }
 
   virtual bool CanActivatePendingTree(LayerTreeHostImpl* impl) OVERRIDE {
-    if (frame_ >= 1)
-      return true;
-
-    return false;
+    return frame_count_with_pending_tree_ > 1;
   }
 
   virtual bool CanActivatePendingTreeIfNeeded(LayerTreeHostImpl* impl)
       OVERRIDE {
-    frame_++;
-    if (frame_ == 1) {
+    if (frame_count_with_pending_tree_ > 1)
+      return true;
+
+    if (first_frame_time_.is_null()) {
       first_frame_time_ = impl->CurrentFrameTimeTicks();
 
       // Since base::TimeTicks::Now() uses a low-resolution clock on
       // Windows, we need to make sure that the clock has incremented past
       // first_frame_time_.
       while (first_frame_time_ == base::TimeTicks::Now()) {}
-
-      return false;
     }
+    return false;
+  }
 
-    return true;
+  virtual void DidActivateTreeOnThread(LayerTreeHostImpl *impl) OVERRIDE {
+    if (impl->settings().impl_side_painting)
+      EXPECT_NE(frame_count_with_pending_tree_, 1);
   }
 
   virtual void AfterTest() OVERRIDE {}
 
  private:
-  int frame_;
+  int frame_count_with_pending_tree_;
   base::TimeTicks first_frame_time_;
 };
 
@@ -830,8 +839,8 @@ class LayerTreeHostTestStartPageScaleAnimation : public LayerTreeHostTest {
     layer_tree_host()->SetPageScaleFactorAndLimits(scale, 0.5f, 2.f);
   }
 
-  virtual void DidActivateTreeOnThread(LayerTreeHostImpl* impl) OVERRIDE {
-    impl->ProcessScrollDeltas();
+  virtual void SwapBuffersOnThread(LayerTreeHostImpl *impl, bool result)
+      OVERRIDE {
     // We get one commit before the first draw, and the animation doesn't happen
     // until the second draw.
     switch (impl->active_tree()->source_frame_number()) {
@@ -841,7 +850,6 @@ class LayerTreeHostTestStartPageScaleAnimation : public LayerTreeHostTest {
         break;
       case 1:
         EXPECT_EQ(1.f, impl->active_tree()->page_scale_factor());
-        PostSetNeedsRedrawToMainThread();
         break;
       case 2:
         EXPECT_EQ(1.25f, impl->active_tree()->page_scale_factor());
@@ -852,7 +860,7 @@ class LayerTreeHostTestStartPageScaleAnimation : public LayerTreeHostTest {
     }
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     switch (layer_tree_host()->source_frame_number()) {
       case 1:
         layer_tree_host()->SetPageScaleFactorAndLimits(1.f, 0.5f, 2.f);
@@ -1263,7 +1271,7 @@ class LayerTreeHostTestAtomicCommitWithPartialUpdate
     PostSetNeedsCommitToMainThread();
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     switch (layer_tree_host()->source_frame_number()) {
       case 1:
         parent_->SetNeedsDisplay();
@@ -1394,7 +1402,7 @@ class LayerTreeHostTestFinishAllRendering : public LayerTreeHostTest {
     PostSetNeedsCommitToMainThread();
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     if (once_)
       return;
     once_ = true;
@@ -1482,6 +1490,16 @@ class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit
     PostSetNeedsCommitToMainThread();
   }
 
+  virtual void DidActivateTreeOnThread(LayerTreeHostImpl *host_impl) OVERRIDE {
+    if (host_impl->active_tree()->source_frame_number() == 0) {

[enne (OOO), 2013/08/15 19:47:59]

It looks like you're changing the memory policy during activation of source
frame 0 but before drawing frame 0, whereas before it was between draw of frame
0 and draw of frame 1.  How is this still working?

[brianderson, 2013/08/15 23:05:10]

My understanding of the memory policy comes from the existing comment below. The
comment or my understanding of it may be wrong: I took the comment to mean that
new resources for tiles would not be allocated after changing the memory policy
here.

I believe this test was flaky after my changes because the second commit could
finish and start rasterizing before the first commit was drawn. If the memory
policy wasn't changed until the first draw, then the second commit could get
resources allocated to it.

So I changed the memory policy after the first activation, which definitely
occurs before the second commit.

[brianderson, 2013/08/15 23:30:25]

@reveman or @vmpstr, can changing the memory policy after activation affect the
subsequent draw?

[enne (OOO), 2013/08/16 00:53:22]

Ok.  Maybe this could be more robust if when you activate source frame #1 you
change the policy *and* manage tiles.  Otherwise, moving around when we manage
tiles could break this test.

[brianderson, 2013/08/16 01:20:49]

Hmmm. I tried calling ManageTiles after changing the memory policy for source
frame #0 and the first draw still succeeded. Is that expected?

I'm not sure if calling ManageTiles after the second activate will help make
this test more robust, since ManagegTiles might still be initiated after the
second commit but before the first draw.

I'm trying to think of a better way to write this test... maybe move the checks
for HaveCachedResourcesForRenderPassId for source frame #0 here.

+      // Reduce the memory limit to only fit the root layer and one render
+      // surface. This prevents any contents drawing into surfaces
+      // from being allocated.
+      host_impl->SetMemoryPolicy(ManagedMemoryPolicy(100 * 100 * 4 * 2));
+      host_impl->SetDiscardBackBufferWhenNotVisible(true);
+    }
+  }
+
   virtual void DrawLayersOnThread(LayerTreeHostImpl* host_impl) OVERRIDE {
     Renderer* renderer = host_impl->renderer();
     RenderPass::Id surface1_render_pass_id = host_impl->active_tree()
@@ -1496,25 +1514,18 @@ class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit
             surface1_render_pass_id));
         EXPECT_TRUE(renderer->HaveCachedResourcesForRenderPassId(
             surface2_render_pass_id));
-
-        // Reduce the memory limit to only fit the root layer and one render
-        // surface. This prevents any contents drawing into surfaces
-        // from being allocated.
-        host_impl->SetMemoryPolicy(ManagedMemoryPolicy(100 * 100 * 4 * 2));
-        host_impl->SetDiscardBackBufferWhenNotVisible(true);
         break;
       case 1:
         EXPECT_FALSE(renderer->HaveCachedResourcesForRenderPassId(
             surface1_render_pass_id));
         EXPECT_FALSE(renderer->HaveCachedResourcesForRenderPassId(
             surface2_render_pass_id));
-
         EndTest();
         break;
     }
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     if (layer_tree_host()->source_frame_number() < 2)
       root_layer_->SetNeedsDisplay();
   }
@@ -1820,7 +1831,7 @@ class LayerTreeHostTestContinuousInvalidate : public LayerTreeHostTest {
     PostSetNeedsCommitToMainThread();
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {

[enne (OOO), 2013/08/15 19:47:59]

I think this change here is right, but this is a bizarre test.  It seems like
CommitCompleteOnThread should EXPECT_EQ(num_draw_layers, num_commit_complete++),
if it's actually trying to make sure that we don't commit without drawing?

[brianderson, 2013/08/15 23:05:10]

I will add that check since it would check all commits rather than just the
commits between the first and second draw.

     if (num_draw_layers_ == 2)
       return;
     content_layer_->SetNeedsDisplay();
@@ -2217,9 +2228,9 @@ class LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled
     // The BeginFrame notification is turned off now but will get enabled
     // once we return. End test while it's enabled.
     ImplThreadTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&LayerTreeHostTestBeginFrameNotification::EndTest,
-                   base::Unretained(this)));
+        FROM_HERE, base::Bind(
+          &LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled::EndTest,

[enne (OOO), 2013/08/15 19:47:59]

style nit: indentation.

+          base::Unretained(this)));
   }
 
   virtual void AfterTest() OVERRIDE {}
@@ -2471,7 +2482,7 @@ class LayerTreeHostTestAsyncReadback : public LayerTreeHostTest {
     PostSetNeedsCommitToMainThread();
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     WaitForCallback();
   }
 
@@ -3013,6 +3024,10 @@ class LayerTreeHostTestNumFramesPending : public LayerTreeHostTest {
         // Round 4 done.
         EXPECT_EQ(2, frame_);
         layer_tree_host()->SetNeedsCommit();
+        // We can't SetNeedsRedraw immediately because it will race the commit.
+        break;
+      case 5:
+        EXPECT_EQ(2, frame_);
         layer_tree_host()->SetNeedsRedraw();
         break;
     }
@@ -3333,7 +3348,7 @@ class LayerTreeHostTestLayersPushProperties : public LayerTreeHostTest {
     LayerTreeHostTest::SetupTree();
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     ++num_commits_;
 
     EXPECT_EQ(expected_push_properties_root_, root_->push_properties_count());
@@ -3527,7 +3542,7 @@ class LayerTreeHostTestPropertyChangesDuringUpdateArePushed
     LayerTreeHostTest::SetupTree();
   }
 
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     switch (layer_tree_host()->source_frame_number()) {
       case 0:
         break;
@@ -3610,7 +3625,7 @@ class LayerTreeHostTestCasePushPropertiesThreeGrandChildren
 class LayerTreeHostTestPushPropertiesAddingToTreeRequiresPush
     : public LayerTreeHostTestCasePushPropertiesThreeGrandChildren {
  protected:
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     int last_source_frame_number = layer_tree_host()->source_frame_number() - 1;
     switch (last_source_frame_number) {
       case 0:
@@ -3650,7 +3665,7 @@ MULTI_THREAD_TEST_F(LayerTreeHostTestPushPropertiesAddingToTreeRequiresPush);
 class LayerTreeHostTestPushPropertiesRemovingChildStopsRecursion
     : public LayerTreeHostTestCasePushPropertiesThreeGrandChildren {
  protected:
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     int last_source_frame_number = layer_tree_host()->source_frame_number() - 1;
     switch (last_source_frame_number) {
       case 0:
@@ -3733,7 +3748,7 @@ MULTI_THREAD_TEST_F(LayerTreeHostTestPushPropertiesRemovingChildStopsRecursion);
 class LayerTreeHostTestPushPropertiesRemovingChildStopsRecursionWithPersistence
     : public LayerTreeHostTestCasePushPropertiesThreeGrandChildren {
  protected:
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     int last_source_frame_number = layer_tree_host()->source_frame_number() - 1;
     switch (last_source_frame_number) {
       case 0:
@@ -3781,7 +3796,7 @@ MULTI_THREAD_TEST_F(
 class LayerTreeHostTestPushPropertiesSetPropertiesWhileOutsideTree
     : public LayerTreeHostTestCasePushPropertiesThreeGrandChildren {
  protected:
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     int last_source_frame_number = layer_tree_host()->source_frame_number() - 1;
     switch (last_source_frame_number) {
       case 0:
@@ -3849,7 +3864,7 @@ MULTI_THREAD_TEST_F(
 class LayerTreeHostTestPushPropertiesSetPropertyInParentThenChild
     : public LayerTreeHostTestCasePushPropertiesThreeGrandChildren {
  protected:
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     int last_source_frame_number = layer_tree_host()->source_frame_number() - 1;
     switch (last_source_frame_number) {
       case 0:
@@ -3913,7 +3928,7 @@ MULTI_THREAD_TEST_F(
 class LayerTreeHostTestPushPropertiesSetPropertyInChildThenParent
     : public LayerTreeHostTestCasePushPropertiesThreeGrandChildren {
  protected:
-  virtual void DidCommitAndDrawFrame() OVERRIDE {
+  virtual void DidCommit() OVERRIDE {
     int last_source_frame_number = layer_tree_host()->source_frame_number() - 1;
     switch (last_source_frame_number) {
       case 0: