Fix flaky test: RenderWidgetHostViewBrowserTest.CopyFromBackingStore.

content/browser/renderer_host/render_widget_host_view_browsertest.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 #include "base/message_loop_proxy.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "content/browser/gpu/gpu_data_manager_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
@@ skipping 20 matching lines @@
   RenderWidgetHostViewBrowserTest() : finish_called_(false), size_(400, 300) {}
 
   virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
     ASSERT_TRUE(PathService::Get(DIR_TEST_DATA, &test_dir_));
   }
 
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     ui::DisableTestCompositor();
   }
 
-  bool CheckAcceleratedCompositingActive() {
+  bool IsAcceleratedCompositingActive() {
     RenderWidgetHostImpl* impl =
         RenderWidgetHostImpl::From(
             shell()->web_contents()->GetRenderWidgetHostView()->
                 GetRenderWidgetHost());
     return impl->is_accelerated_compositing_active();
   }
 
-  bool CheckCompositingSurface() {
+  bool IsCompositingSurfaceReady() {
 #if defined(OS_WIN)
     if (!GpuDataManagerImpl::GetInstance()->IsUsingAcceleratedSurface())
       return false;
 #endif
 
     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;
   }
 
+  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())
       return false;
 #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,
                                   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());
@@ skipping 145 matching lines @@
       new FakeFrameSubscriber(base::Bind(&DeliverFrameFunc,
                                          base::MessageLoopProxy::current(),
                                          run_loop.QuitClosure(),
                                          &frame_captured)));
   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) {
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewBrowserTest, CopyFromBackingStore) {
   SetupNonCompositing();
+  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_,
       base::Bind(&RenderWidgetHostViewBrowserTest::FinishCopyFromBackingStore,
                  base::Unretained(this), true, run_loop.QuitClosure()));
   run_loop.Run();
 
   EXPECT_TRUE(finish_called_);
 }
 #endif
@@ skipping 33 matching lines @@
                  &second_frame_captured,
                  base::Time::Now()));
   run_loop.Run();
 
   EXPECT_TRUE(first_frame_captured);
   EXPECT_TRUE(second_frame_captured);
 }
 #endif
 
 }  // namespace content