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..394adfb04df98e3fd0e4bc9237809c4c7fac776f 100644
--- a/content/browser/renderer_host/render_widget_host_view_browsertest.cc
+++ b/content/browser/renderer_host/render_widget_host_view_browsertest.cc
@@ -38,7 +38,7 @@ class RenderWidgetHostViewBrowserTest : public ContentBrowserTest {
     ui::DisableTestCompositor();
   }
 
-  bool CheckAcceleratedCompositingActive() {
+  bool IsAcceleratedCompositingActive() {
     RenderWidgetHostImpl* impl =
         RenderWidgetHostImpl::From(
             shell()->web_contents()->GetRenderWidgetHostView()->
@@ -46,7 +46,7 @@ class RenderWidgetHostViewBrowserTest : public ContentBrowserTest {
     return impl->is_accelerated_compositing_active();
   }
 
-  bool CheckCompositingSurface() {
+  bool IsCompositingSurfaceReady() {
 #if defined(OS_WIN)
     if (!GpuDataManagerImpl::GetInstance()->IsUsingAcceleratedSurface())
       return false;
@@ -63,6 +63,30 @@ class RenderWidgetHostViewBrowserTest : public ContentBrowserTest {
     return ret;
   }
 
+  bool IsBackingStoreReady() {
+    RenderWidgetHostImpl* const rwh = RenderWidgetHostImpl::From(
+        shell()->web_contents()->GetRenderWidgetHostView()->
+            GetRenderWidgetHost());
+    // 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 rwh->GetBackingStore(false) != NULL;
+  }
+
+  // Waits until either the compositing surface or the backing store is ready as
+  // a source for copying.
+  void WaitForCopySourceReady(bool is_compositing_surface) {
+    while ((is_compositing_surface && !IsCompositingSurfaceReady()) ||
+           (!is_compositing_surface && !IsBackingStoreReady())) {
+      base::RunLoop run_loop;
+      MessageLoop::current()->PostDelayedTask(
+          FROM_HERE,
+          run_loop.QuitClosure(),
+          base::TimeDelta::FromMilliseconds(10));
+      run_loop.Run();
+    }
+  }
+
   bool SetupCompositingSurface() {
 #if defined(OS_MACOSX)
     if (!IOSurfaceSupport::Initialize())
@@ -70,26 +94,22 @@ class RenderWidgetHostViewBrowserTest : public ContentBrowserTest {
 #endif
     NavigateToURL(shell(), net::FilePathToFileURL(
         test_dir_.AppendASCII("rwhv_compositing_animation.html")));
-    if (!CheckAcceleratedCompositingActive())
+    if (!IsAcceleratedCompositingActive())
       return false;
 
     // The page is now accelerated composited but a compositing surface might
     // not be available immediately so wait for it.
-    while (!CheckCompositingSurface()) {
-      base::RunLoop run_loop;
-      MessageLoop::current()->PostDelayedTask(
-          FROM_HERE,
-          run_loop.QuitClosure(),
-          base::TimeDelta::FromMilliseconds(10));
-      run_loop.Run();
-    }
+    WaitForCopySourceReady(true);
     return true;
   }
 
   bool SetupNonCompositing() {
+    // Assumption: Static HTML pages with only text will cause the RenderWidget
+    // to turn on compositing.

[ncarter (slow), 2013/04/29 23:34:21]

The text here describes the opposite of what the name suggests.

     NavigateToURL(shell(), net::FilePathToFileURL(
         test_dir_.AppendASCII("rwhv_compositing_static.html")));
-    return !CheckCompositingSurface();
+    WaitForCopySourceReady(false);
+    return true;
   }
 
   void FinishCopyFromBackingStore(bool expected_result,
@@ -255,18 +275,12 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest,
   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) {
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest, CopyFromBackingStore) {
   SetupNonCompositing();
-  base::RunLoop run_loop;
+  LOG_IF(WARNING, IsCompositingSurfaceReady())
+      << "Accelerated compositing is still running for a static HTML page.";

[Ken Russell (switch to Gerrit), 2013/04/29 23:08:54]

It looks to me like this test's assumptions are broken on platforms that use
threaded compositing and/or force-compositing-mode.

[ncarter (slow), 2013/04/29 23:34:21]

Agree, and I'm not such a big fan of a LOG_WARNING from  tests which nobody
might notice -- my attitude might be summarized as "fail or do not fail, there
is no warn".

Kbr, can you think of some way (from the RVH) that software mode can be forced
on or off for a page?

If not, we might refactor the intent of these tests so that they're more like
"CopyFromBackingStore_StaticContent" and "CopyFromBackingStore_3DTransform", so
that while the tests are designed to provide a certain kind of coverage, the
test logic doesn't depend on the actual path taken. Such a test might be easier
to maintain as the underlying heuristics change.

 
+  base::RunLoop run_loop;
   shell()->web_contents()->GetRenderViewHost()->CopyFromBackingStore(
       gfx::Rect(),
       size_,