Use RunLoop to wait for readback on compositor unittest

ui/compositor/layer_unittest.cc

Index: ui/compositor/layer_unittest.cc
diff --git a/ui/compositor/layer_unittest.cc b/ui/compositor/layer_unittest.cc
index d35143d5e96a557a7209d5feea190a1c70123a3f..c72beadecfe486169445c7eae4f72328e3e3ff8f 100644
--- a/ui/compositor/layer_unittest.cc
+++ b/ui/compositor/layer_unittest.cc
@@ -132,11 +132,11 @@ class LayerWithRealCompositorTest : public testing::Test {
     WaitForSwap();
   }
 
-  bool ReadPixels(SkBitmap* bitmap) {
+  void ReadPixels(SkBitmap* bitmap) {
     return ReadPixels(bitmap, gfx::Rect(GetCompositor()->size()));

[danakj, 2014/09/19 20:52:26]

remove return

[weiliangc, 2014/09/19 21:02:46]

Done.

   }
 
-  bool ReadPixels(SkBitmap* bitmap, gfx::Rect source_rect) {
+  void ReadPixels(SkBitmap* bitmap, gfx::Rect source_rect) {
     scoped_refptr<ReadbackHolder> holder(new ReadbackHolder);
     scoped_ptr<cc::CopyOutputRequest> request =
         cc::CopyOutputRequest::CreateBitmapRequest(
@@ -148,21 +148,15 @@ class LayerWithRealCompositorTest : public testing::Test {
     // Wait for copy response.  The copy output request will get committed
     // before the first draw, but may not be part of the first draw's frame.
     // The second draw will perform the async copy request, post the callback.
-    // The second loop finishes before the callback is run, so a third
-    // loop is needed.
-    for (int i = 0; i < 3; i++) {
+    for (int i = 0; i < 2; i++) {
       GetCompositor()->ScheduleFullRedraw();
       WaitForDraw();
     }
 
-    if (holder->completed()) {
-      *bitmap = holder->result();
-      return true;
-    }
+    // Waits for the callback to finish run and return result.
+    holder->WaitForReadback();
 
-    // Callback never called.
-    NOTREACHED();
-    return false;
+    *bitmap = holder->result();
   }
 
   void WaitForDraw() {
@@ -190,16 +184,17 @@ class LayerWithRealCompositorTest : public testing::Test {
  private:
   class ReadbackHolder : public base::RefCountedThreadSafe<ReadbackHolder> {
    public:
-    ReadbackHolder() : completed_(false) {}
+    ReadbackHolder() : run_loop_(new base::RunLoop) {}
 
     void OutputRequestCallback(scoped_ptr<cc::CopyOutputResult> result) {
-      DCHECK(!completed_);
+      DCHECK(run_loop_->running());

[danakj, 2014/09/19 20:52:26]

what if the result comes before the wait starts, this looks like a race and it's
not important.

[weiliangc, 2014/09/19 21:02:46]

Make sense. Removed.

       result_ = result->TakeBitmap();
-      completed_ = true;
+      base::MessageLoop::current()->PostTask(FROM_HERE,
+                                             run_loop_->QuitClosure());
     }
-    bool completed() const {
-      return completed_;
-    };
+
+    void WaitForReadback() { run_loop_->Run(); }

[danakj, 2014/09/19 20:52:26]

can you verify that Run() will exit if you called the QuitClosure() before
calling Run() otherwise the race is bad, and you need to check if the callback
already happened and skip Run()

[weiliangc, 2014/09/19 21:02:46]

RunLoop's comment on its Quit and QuitClose function says:
- calling Quit before Run will just cause Run to exit immediately
- QuitClosure calls Quit safely, has no effect if RunLoop is gone

How much trust can I put into these comments?

+
     const SkBitmap& result() const { return *result_; }
 
    private:
@@ -208,7 +203,7 @@ class LayerWithRealCompositorTest : public testing::Test {
     virtual ~ReadbackHolder() {}
 
     scoped_ptr<SkBitmap> result_;
-    bool completed_;
+    scoped_ptr<base::RunLoop> run_loop_;
   };
 
   scoped_ptr<TestCompositorHost> compositor_host_;
@@ -894,7 +889,7 @@ TEST_F(LayerWithRealCompositorTest, DrawPixels) {
   DrawTree(layer.get());
 
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap, gfx::Rect(viewport_size)));
+  ReadPixels(&bitmap, gfx::Rect(viewport_size));
   ASSERT_FALSE(bitmap.empty());
 
   SkAutoLockPixels lock(bitmap);
@@ -932,7 +927,7 @@ TEST_F(LayerWithRealCompositorTest, DrawAlphaBlendedPixels) {
   DrawTree(background_layer.get());
 
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap, gfx::Rect(viewport_size)));
+  ReadPixels(&bitmap, gfx::Rect(viewport_size));
   ASSERT_FALSE(bitmap.empty());
 
   SkAutoLockPixels lock(bitmap);
@@ -973,7 +968,7 @@ TEST_F(LayerWithRealCompositorTest, DrawAlphaThresholdFilterPixels) {
   DrawTree(background_layer.get());
 
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap, gfx::Rect(viewport_size)));
+  ReadPixels(&bitmap, gfx::Rect(viewport_size));
   ASSERT_FALSE(bitmap.empty());
 
   SkAutoLockPixels lock(bitmap);
@@ -1119,14 +1114,14 @@ TEST_F(LayerWithRealCompositorTest, ModifyHierarchy) {
   l11->Add(l21.get());
   l0->Add(l12.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   // WritePNGFile(bitmap, ref_img1);
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img1, cc::ExactPixelComparator(true)));
 
   l0->StackAtTop(l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   // WritePNGFile(bitmap, ref_img2);
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img2, cc::ExactPixelComparator(true)));
@@ -1134,28 +1129,28 @@ TEST_F(LayerWithRealCompositorTest, ModifyHierarchy) {
   // should restore to original configuration
   l0->StackAbove(l12.get(), l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img1, cc::ExactPixelComparator(true)));
 
   // l11 back to front
   l0->StackAtTop(l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img2, cc::ExactPixelComparator(true)));
 
   // should restore to original configuration
   l0->StackAbove(l12.get(), l11.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img1, cc::ExactPixelComparator(true)));
 
   // l11 back to front
   l0->StackAbove(l11.get(), l12.get());
   DrawTree(l0.get());
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img2, cc::ExactPixelComparator(true)));
 }
@@ -1178,7 +1173,7 @@ TEST_F(LayerWithRealCompositorTest, Opacity) {
   l0->Add(l11.get());
   DrawTree(l0.get());
   SkBitmap bitmap;
-  ASSERT_TRUE(ReadPixels(&bitmap));
+  ReadPixels(&bitmap);
   ASSERT_FALSE(bitmap.empty());
   // WritePNGFile(bitmap, ref_img);
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img, cc::ExactPixelComparator(true)));