Fix flaky test: RenderWidgetHostViewBrowserTest.CopyFromBackingStore.

content/browser/renderer_host/render_widget_host_view_browsertest.cc

Index: content/browser/renderer_host/render_widget_host_view_browsertest.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_browsertest.cc b/content/browser/renderer_host/render_widget_host_view_browsertest.cc
index 2235486de06d10e9c562f9f8a73b9eebf561fc14..75718aad56c551bd0c28b979cf61d0ab631c07c6 100644
--- a/content/browser/renderer_host/render_widget_host_view_browsertest.cc
+++ b/content/browser/renderer_host/render_widget_host_view_browsertest.cc
@@ -13,69 +13,132 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_paths.h"
+#include "content/public/common/content_switches.h"
 #include "content/shell/shell.h"
 #include "content/test/content_browser_test.h"
 #include "content/test/content_browser_test_utils.h"
 #include "media/base/video_frame.h"
 #include "net/base/net_util.h"
-#include "skia/ext/platform_canvas.h"
 #include "ui/compositor/compositor_setup.h"
 #if defined(OS_MACOSX)
 #include "ui/surface/io_surface_support_mac.h"
 #endif
 
 namespace content {
+namespace {
 
+// Common base class for browser tests.  This is subclassed twice: Once to test
+// the browser in forced-compositing mode, and once to test with compositing
+// mode disabled.
 class RenderWidgetHostViewBrowserTest : public ContentBrowserTest {
  public:
-  RenderWidgetHostViewBrowserTest() : finish_called_(false), size_(400, 300) {}
+  RenderWidgetHostViewBrowserTest()
+      : frame_size_(400, 300),
+        callback_invoke_count_(0),
+        frames_captured_(0) {}
 
   virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
+    ui::DisableTestCompositor();
     ASSERT_TRUE(PathService::Get(DIR_TEST_DATA, &test_dir_));
   }
 
-  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
-    ui::DisableTestCompositor();
+  // Overridden in subclasses to source either a compositing surface or a
+  // backing store.
+  virtual void SetUpSourceSurface() = 0;
+  virtual bool IsCopySourceReady() = 0;
+
+  int callback_invoke_count() const {
+    return callback_invoke_count_;
   }
 
-  bool CheckAcceleratedCompositingActive() {
-    RenderWidgetHostImpl* impl =
-        RenderWidgetHostImpl::From(
-            shell()->web_contents()->GetRenderWidgetHostView()->
-                GetRenderWidgetHost());
-    return impl->is_accelerated_compositing_active();
+  int frames_captured() const {
+    return frames_captured_;
   }
 
-  bool CheckCompositingSurface() {
-#if defined(OS_WIN)
-    if (!GpuDataManagerImpl::GetInstance()->IsUsingAcceleratedSurface())
-      return false;
-#endif
+  const gfx::Size& frame_size() const {
+    return frame_size_;
+  }
 
-    RenderViewHost* const rwh =
-        shell()->web_contents()->GetRenderViewHost();
-    RenderWidgetHostViewPort* rwhvp =
-        static_cast<RenderWidgetHostViewPort*>(rwh->GetView());
-    bool ret = !rwhvp->GetCompositingSurface().is_null();
-#if defined(OS_MACOSX)
-    ret &= rwhvp->HasAcceleratedSurface(gfx::Size());
-#endif
-    return ret;
+  const base::FilePath& test_dir() const {
+    return test_dir_;
   }
 
-  bool SetupCompositingSurface() {
-#if defined(OS_MACOSX)
-    if (!IOSurfaceSupport::Initialize())
-      return false;
-#endif
-    NavigateToURL(shell(), net::FilePathToFileURL(
-        test_dir_.AppendASCII("rwhv_compositing_animation.html")));
-    if (!CheckAcceleratedCompositingActive())
-      return false;
+  RenderViewHost* GetRenderViewHost() const {
+    RenderViewHost* const rvh = shell()->web_contents()->GetRenderViewHost();
+    CHECK(rvh);
+    return rvh;
+  }
 
-    // The page is now accelerated composited but a compositing surface might
-    // not be available immediately so wait for it.
-    while (!CheckCompositingSurface()) {
+  RenderWidgetHostImpl* GetRenderWidgetHost() const {
+    RenderWidgetHostImpl* const rwh = RenderWidgetHostImpl::From(
+        shell()->web_contents()->GetRenderWidgetHostView()->
+            GetRenderWidgetHost());
+    CHECK(rwh);
+    return rwh;
+  }
+
+  RenderWidgetHostViewPort* GetRenderWidgetHostViewPort() const {
+    RenderWidgetHostViewPort* const view =
+        RenderWidgetHostViewPort::FromRWHV(GetRenderViewHost()->GetView());
+    CHECK(view);
+    return view;
+  }
+
+  // Callback when using CopyFromBackingStore() API.
+  void FinishCopyFromBackingStore(const base::Closure& quit_closure,
+                                  bool frame_captured,
+                                  const SkBitmap& bitmap) {
+    ++callback_invoke_count_;
+    if (frame_captured) {
+      ++frames_captured_;
+      EXPECT_FALSE(bitmap.empty());
+    }
+    if (!quit_closure.is_null())
+      quit_closure.Run();
+  }
+
+  // Callback when using CopyFromCompositingSurfaceToVideoFrame() API.
+  void FinishCopyFromCompositingSurface(const base::Closure& quit_closure,
+                                        bool frame_captured) {
+    ++callback_invoke_count_;
+    if (frame_captured)
+      ++frames_captured_;
+    if (!quit_closure.is_null())
+      quit_closure.Run();
+  }
+
+  // Callback when using frame subscriber API.
+  void FrameDelivered(const scoped_refptr<base::MessageLoopProxy>& loop,
+                      base::Closure quit_closure,
+                      base::Time timestamp,
+                      bool frame_captured) {
+    ++callback_invoke_count_;
+    if (frame_captured)
+      ++frames_captured_;
+    if (!quit_closure.is_null())
+      loop->PostTask(FROM_HERE, quit_closure);
+  }
+
+  // Copy one frame using the CopyFromBackingStore API.
+  void RunBasicCopyFromBackingStoreTest() {
+    SetUpSourceSurface();
+
+    base::RunLoop run_loop;
+    GetRenderViewHost()->CopyFromBackingStore(
+        gfx::Rect(),
+        frame_size(),
+        base::Bind(&RenderWidgetHostViewBrowserTest::FinishCopyFromBackingStore,
+                   base::Unretained(this), run_loop.QuitClosure()));
+    run_loop.Run();
+
+    EXPECT_EQ(1, callback_invoke_count());
+    EXPECT_EQ(1, frames_captured());
+  }
+
+ protected:
+  // Waits until the source is available for copying.
+  void WaitForCopySourceReady() {
+    while (!IsCopySourceReady()) {
       base::RunLoop run_loop;
       MessageLoop::current()->PostDelayedTask(
           FROM_HERE,
@@ -83,39 +146,72 @@ class RenderWidgetHostViewBrowserTest : public ContentBrowserTest {
           base::TimeDelta::FromMilliseconds(10));
       run_loop.Run();
     }
-    return true;
   }
 
-  bool SetupNonCompositing() {
+ private:
+  const gfx::Size frame_size_;
+  base::FilePath test_dir_;
+  int callback_invoke_count_;
+  int frames_captured_;
+};
+
+class RenderWidgetHostViewBrowserTest_Compositing

[Ken Russell (switch to Gerrit), 2013/04/30 23:23:02]

The "_" here and in the subclass below doesn't really follow the naming
convention.

[miu, 2013/05/01 00:19:36]

Done.  Was concerned about what people might try in --gtest_filter=... command
line args.  Move "Compositing" to front of class name.  Similar change for the
"NonCompositing" one, too.

+    : public RenderWidgetHostViewBrowserTest {
+ public:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    command_line->AppendSwitch(switches::kForceCompositingMode);
+  }
+
+  virtual void SetUpSourceSurface() OVERRIDE {
+#if defined(OS_MACOSX)
+    ASSERT_TRUE(IOSurfaceSupport::Initialize());
+#endif
     NavigateToURL(shell(), net::FilePathToFileURL(
-        test_dir_.AppendASCII("rwhv_compositing_static.html")));
-    return !CheckCompositingSurface();
+        test_dir().AppendASCII("rwhv_compositing_animation.html")));
+#if !defined(USE_AURA)
+    ASSERT_TRUE(GetRenderWidgetHost()->is_accelerated_compositing_active());
+#endif
+
+    // The page is now accelerated composited but a compositing surface might
+    // not be available immediately so wait for it.
+    WaitForCopySourceReady();
   }
 
-  void FinishCopyFromBackingStore(bool expected_result,
-                                  const base::Closure& quit_closure,
-                                  bool result,
-                                  const SkBitmap& bitmap) {
-    quit_closure.Run();
-    EXPECT_EQ(expected_result, result);
-    if (expected_result)
-      EXPECT_FALSE(bitmap.empty());
-    finish_called_ = true;
+  virtual bool IsCopySourceReady() OVERRIDE {
+#if defined(OS_WIN)
+    if (!GpuDataManagerImpl::GetInstance()->IsUsingAcceleratedSurface())
+      return false;
+#endif
+
+    RenderWidgetHostViewPort* const view = GetRenderWidgetHostViewPort();
+    bool ret = !view->GetCompositingSurface().is_null();
+#if defined(OS_MACOSX)
+    ret &= view->HasAcceleratedSurface(gfx::Size());
+#endif
+    return ret;
+  }
+};
+
+class RenderWidgetHostViewBrowserTest_NonCompositing
+    : public RenderWidgetHostViewBrowserTest {
+ public:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    command_line->AppendSwitch(switches::kDisableAcceleratedCompositing);
   }
 
-  void FinishCopyFromCompositingSurface(bool expected_result,
-                                        const base::Closure& quit_closure,
-                                        bool result) {
-    if (!quit_closure.is_null())
-      quit_closure.Run();
-    EXPECT_EQ(expected_result, result);
-    finish_called_ = true;
+  virtual void SetUpSourceSurface() OVERRIDE {
+    // Assumption: Browser-internal pages will cause RenderWidget to turn off
+    // compositing.

[Ken Russell (switch to Gerrit), 2013/04/30 23:23:02]

Wasn't the point of the subclass to avoid these assumptions? Can you use real
content now instead of about:blank?

[miu, 2013/05/01 00:19:36]

Ah!  A should have removed the comment.  The command line flag will ensure
compositing is turned off.  Done.

BTW--This code path used to NavigateToURL(rwhv_compositing_static.html).  I
changed it to NavigateToURL(about:blank) as an optimization (since there's no
testing that depends on the content of the page).

+    NavigateToURL(shell(), GURL("about:blank"));
+    WaitForCopySourceReady();
   }
 
- protected:
-  base::FilePath test_dir_;
-  bool finish_called_;
-  gfx::Size size_;
+  virtual bool IsCopySourceReady() OVERRIDE {
+    // Note: Passing false to GetBackingStore() asks for the backing store
+    // without forcing its creation.  We want to the browser to create it via
+    // its normal mechanism.
+    return GetRenderWidgetHost()->GetBackingStore(false) != NULL;

[Ken Russell (switch to Gerrit), 2013/04/30 23:23:02]

Do you want to make assertions that the composited code path isn't active at
this point?

[miu, 2013/05/01 00:19:36]

Good point.  Done.

+  }
 };
 
 class FakeFrameSubscriber : public RenderWidgetHostViewFrameSubscriber {
@@ -146,177 +242,142 @@ class FakeFrameSubscriber : public RenderWidgetHostViewFrameSubscriber {
   DeliverFrameCallback callback_;
 };
 
-#if defined(OS_MACOSX) || defined(OS_WIN)
+// Disable tests for Android and IOS as these platforms have incomplete
+// implementation.
+#if !defined(OS_ANDROID) && !defined(OS_IOS)
 
-static void DeliverFrameFunc(const scoped_refptr<base::MessageLoopProxy>& loop,
-                             base::Closure quit_closure,
-                             bool* frame_captured_out,
-                             base::Time timestamp,
-                             bool frame_captured) {
-  *frame_captured_out = frame_captured;
-  if (!quit_closure.is_null())
-    loop->PostTask(FROM_HERE, quit_closure);
+// The CopyFromBackingStore() API should work on all platforms when compositing
+// is enabled.
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest_Compositing,
+                       CopyFromBackingStore) {
+  RunBasicCopyFromBackingStoreTest();
 }
 
-#endif
+// The CopyFromBackingStore() API should work on all platforms when compositing
+// is disabled.
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest_NonCompositing,
+                       CopyFromBackingStore) {
+  RunBasicCopyFromBackingStoreTest();
+}
 
-#if defined(OS_MACOSX)
-// Tests that the callback passed to CopyFromBackingStore is always called, even
-// when the RenderWidgetHost is deleting in the middle of an async copy.
-IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest,
-                       MacAsyncCopyFromBackingStoreCallbackTest) {
-  if (!SetupCompositingSurface()) {
-    LOG(WARNING) << "Accelerated compositing not running.";
-    return;
-  }
+// Tests that the callback passed to CopyFromBackingStore is always called,
+// even when the RenderWidgetHost is deleting in the middle of an async copy.
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest_Compositing,
+                       CopyFromBackingStore_CallbackDespiteDelete) {
+  SetUpSourceSurface();
 
   base::RunLoop run_loop;
-  RenderViewHost* const rwh =
-      shell()->web_contents()->GetRenderViewHost();
-  RenderWidgetHostViewPort* rwhvp =
-      static_cast<RenderWidgetHostViewPort*>(rwh->GetView());
-
-  rwh->CopyFromBackingStore(
+  GetRenderViewHost()->CopyFromBackingStore(
       gfx::Rect(),
-      size_,
+      frame_size(),
       base::Bind(&RenderWidgetHostViewBrowserTest::FinishCopyFromBackingStore,
-                 base::Unretained(this), false, run_loop.QuitClosure()));
-
-  // Delete the surface before the callback is run. This is synchronous until
-  // we get to the copy_timer_, so we will always end up in the destructor
-  // before the timer fires.
-  rwhvp->AcceleratedSurfaceRelease();
+                 base::Unretained(this), run_loop.QuitClosure()));
+  // Delete the surface before the callback is run.
+  GetRenderWidgetHostViewPort()->AcceleratedSurfaceRelease();
   run_loop.Run();
 
-  EXPECT_TRUE(finish_called_);
+  EXPECT_EQ(1, callback_invoke_count());
 }
 
 // Tests that the callback passed to CopyFromCompositingSurfaceToVideoFrame is
 // always called, even when the RenderWidgetHost is deleting in the middle of
 // an async copy.
-IN_PROC_BROWSER_TEST_F(
-    RenderWidgetHostViewBrowserTest,
-    MacAsyncCopyFromCompositingSurfaceToVideoFrameCallbackTest) {
-  if (!SetupCompositingSurface()) {
-    LOG(WARNING) << "Accelerated compositing not running.";
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest_Compositing,
+                       CopyFromCompositingSurface_CallbackDespiteDelete) {
+  SetUpSourceSurface();
+  RenderWidgetHostViewPort* const view = GetRenderWidgetHostViewPort();
+  if (!view->CanCopyToVideoFrame()) {
+    LOG(WARNING) << ("CopyFromCompositingSurfaceToVideoFrame() not supported "
+                     "on this platform.");
     return;
   }
 
   base::RunLoop run_loop;
-  RenderViewHost* const rwh =
-      shell()->web_contents()->GetRenderViewHost();
-  RenderWidgetHostViewPort* rwhvp =
-      static_cast<RenderWidgetHostViewPort*>(rwh->GetView());
-
   scoped_refptr<media::VideoFrame> dest =
-      media::VideoFrame::CreateBlackFrame(size_);
-  rwhvp->CopyFromCompositingSurfaceToVideoFrame(
-      gfx::Rect(rwhvp->GetViewBounds().size()), dest, base::Bind(
+      media::VideoFrame::CreateBlackFrame(frame_size());
+  view->CopyFromCompositingSurfaceToVideoFrame(
+      gfx::Rect(view->GetViewBounds().size()), dest, base::Bind(
           &RenderWidgetHostViewBrowserTest::FinishCopyFromCompositingSurface,
-          base::Unretained(this), false, run_loop.QuitClosure()));
-
-  // Delete the surface before the callback is run. This is synchronous until
-  // we get to the copy_timer_, so we will always end up in the destructor
-  // before the timer fires.
-  rwhvp->AcceleratedSurfaceRelease();
+          base::Unretained(this), run_loop.QuitClosure()));
+  // Delete the surface before the callback is run.
+  view->AcceleratedSurfaceRelease();
   run_loop.Run();
 
-  ASSERT_TRUE(finish_called_);
+  EXPECT_EQ(1, callback_invoke_count());
 }
-#endif
-
-#if (defined(OS_WIN) && !defined(USE_AURA)) || defined(OS_MACOSX)
-IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest,
-                       FrameSubscriberTest) {
-  if (!SetupCompositingSurface()) {
-    LOG(WARNING) << "Accelerated compositing not running.";
-    return;
-  }
 
-  base::RunLoop run_loop;
-  RenderWidgetHostViewPort* view = RenderWidgetHostViewPort::FromRWHV(
-      shell()->web_contents()->GetRenderViewHost()->GetView());
-  ASSERT_TRUE(view);
+// With compositing turned off, no platforms should support the
+// CopyFromCompositingSurfaceToVideoFrame() API.
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest_NonCompositing,
+                       CopyFromCompositingSurfaceToVideoFrameCallbackTest) {
+  SetUpSourceSurface();
+  EXPECT_FALSE(GetRenderWidgetHostViewPort()->CanCopyToVideoFrame());
+}
 
+// Test basic frame subscription functionality.  We subscribe, and then run
+// until at least one DeliverFrameCallback has been invoked.
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest_Compositing,
+                       FrameSubscriberTest) {
+  SetUpSourceSurface();
+  RenderWidgetHostViewPort* const view = GetRenderWidgetHostViewPort();
   if (!view->CanSubscribeFrame()) {
-    LOG(WARNING) << "Frame subscription no supported on this platform.";
+    LOG(WARNING) << "Frame subscription not supported on this platform.";
     return;

[ncarter (slow), 2013/05/01 00:04:32]

I expect this will happen on XP.

   }
 
-  bool frame_captured = false;
+  base::RunLoop run_loop;
   scoped_ptr<RenderWidgetHostViewFrameSubscriber> subscriber(
-      new FakeFrameSubscriber(base::Bind(&DeliverFrameFunc,
-                                         base::MessageLoopProxy::current(),
-                                         run_loop.QuitClosure(),
-                                         &frame_captured)));
+      new FakeFrameSubscriber(
+          base::Bind(&RenderWidgetHostViewBrowserTest::FrameDelivered,
+                     base::Unretained(this),
+                     base::MessageLoopProxy::current(),
+                     run_loop.QuitClosure())));
   view->BeginFrameSubscription(subscriber.Pass());
   run_loop.Run();
   view->EndFrameSubscription();
-  EXPECT_TRUE(frame_captured);
-}
 
-// Test copying from backing store when page is non-accelerated-composited.
-// Flaky. http://crbug.com/224351
-#if defined(OS_MACOSX) || defined(OS_WIN)
-#define MAYBE_CopyFromBackingStore DISABLED_CopyFromBackingStore
-#else
-#define MAYBE_CopyFromBackingStore CopyFromBackingStore
-#endif
-IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest,
-                       MAYBE_CopyFromBackingStore) {
-  SetupNonCompositing();
-  base::RunLoop run_loop;
-
-  shell()->web_contents()->GetRenderViewHost()->CopyFromBackingStore(
-      gfx::Rect(),
-      size_,
-      base::Bind(&RenderWidgetHostViewBrowserTest::FinishCopyFromBackingStore,
-                 base::Unretained(this), true, run_loop.QuitClosure()));
-  run_loop.Run();
-
-  EXPECT_TRUE(finish_called_);
+  EXPECT_LE(1, callback_invoke_count());
+  EXPECT_LE(1, frames_captured());
 }
-#endif
 
-#if defined(OS_MACOSX)
 // Test that we can copy twice from an accelerated composited page.
-// This test is only running on Mac because this is the only platform that
-// we can reliably detect that accelerated surface is in use.
-IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest, CopyTwice) {
-  if (!SetupCompositingSurface()) {
-    LOG(WARNING) << "Accelerated compositing not running.";
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest_Compositing, CopyTwice) {
+  SetUpSourceSurface();
+  RenderWidgetHostViewPort* const view = GetRenderWidgetHostViewPort();
+  if (!view->CanCopyToVideoFrame()) {
+    LOG(WARNING) << ("CopyFromCompositingSurfaceToVideoFrame() not supported "
+                     "on this platform.");

[ncarter (slow), 2013/05/01 00:04:32]

I expect this will happen on XP.

     return;
   }
 
   base::RunLoop run_loop;
-  RenderViewHost* const rwh =
-      shell()->web_contents()->GetRenderViewHost();
-  RenderWidgetHostViewPort* rwhvp =
-      static_cast<RenderWidgetHostViewPort*>(rwh->GetView());
-  scoped_refptr<media::VideoFrame> dest =
-      media::VideoFrame::CreateBlackFrame(size_);
-
-  bool first_frame_captured = false;
-  bool second_frame_captured = false;
-  rwhvp->CopyFromCompositingSurfaceToVideoFrame(
-      gfx::Rect(rwhvp->GetViewBounds().size()), dest,
-      base::Bind(&DeliverFrameFunc,
+  scoped_refptr<media::VideoFrame> first_output =
+      media::VideoFrame::CreateBlackFrame(frame_size());
+  ASSERT_TRUE(first_output);
+  scoped_refptr<media::VideoFrame> second_output =
+      media::VideoFrame::CreateBlackFrame(frame_size());
+  ASSERT_TRUE(second_output);
+  view->CopyFromCompositingSurfaceToVideoFrame(
+      gfx::Rect(view->GetViewBounds().size()), first_output,
+      base::Bind(&RenderWidgetHostViewBrowserTest::FrameDelivered,
+                 base::Unretained(this),
                  base::MessageLoopProxy::current(),
                  base::Closure(),
-                 &first_frame_captured,
                  base::Time::Now()));
-  rwhvp->CopyFromCompositingSurfaceToVideoFrame(
-      gfx::Rect(rwhvp->GetViewBounds().size()), dest,
-      base::Bind(&DeliverFrameFunc,
+  view->CopyFromCompositingSurfaceToVideoFrame(
+      gfx::Rect(view->GetViewBounds().size()), second_output,
+      base::Bind(&RenderWidgetHostViewBrowserTest::FrameDelivered,
+                 base::Unretained(this),
                  base::MessageLoopProxy::current(),
                  run_loop.QuitClosure(),
-                 &second_frame_captured,
                  base::Time::Now()));
   run_loop.Run();
 
-  EXPECT_TRUE(first_frame_captured);
-  EXPECT_TRUE(second_frame_captured);
+  EXPECT_EQ(2, callback_invoke_count());
+  EXPECT_EQ(2, frames_captured());
 }
-#endif
 
+#endif  // !defined(OS_ANDROID) && !defined(OS_IOS)
+
+}  // namespace
 }  // namespace content