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..f18edda849337eecec5d00ee72f404318133b2f9 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) {
-    return ReadPixels(bitmap, gfx::Rect(GetCompositor()->size()));
+  void ReadPixels(SkBitmap* bitmap) {
+    ReadPixels(bitmap, gfx::Rect(GetCompositor()->size()));
   }
 
-  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,16 @@ 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_);
       result_ = result->TakeBitmap();
-      completed_ = true;
+      base::MessageLoop::current()->PostTask(FROM_HERE,

[danakj, 2014/09/19 22:51:15]

you could just QuitClosure().Run() instead of posttasking it.

but even better just run_loop_.Quit(); :)

[danakj, 2014/09/19 23:36:17]

LGTM once you Quit() more directly here.

+                                             run_loop_->QuitClosure());
     }
-    bool completed() const {
-      return completed_;
-    };
+
+    void WaitForReadback() { run_loop_->Run(); }
+
     const SkBitmap& result() const { return *result_; }
 
    private:
@@ -208,7 +202,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 +888,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 +926,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 +967,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 +1113,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 +1128,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 +1172,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)));