Fix flakiness in DragAndDropBrowserTests

content/public/test/browser_test_utils.cc

Index: content/public/test/browser_test_utils.cc
diff --git a/content/public/test/browser_test_utils.cc b/content/public/test/browser_test_utils.cc
index 01620c8a7b8f5ae3e0998534f42919a75cd6a900..1fd769a26d134d02d74f7a49eb161ce1dc97d82a 100644
--- a/content/public/test/browser_test_utils.cc
+++ b/content/public/test/browser_test_utils.cc
@@ -1169,26 +1169,66 @@ void SendRoutedGestureTapSequence(content::WebContents* web_contents,
   rwhva->OnGestureEvent(&gesture_tap);
 }
 
+#endif
+
 // TODO(wjmaclean): The next two functions are a modified version of
 // SurfaceHitTestReadyNotifier that (1) works for BrowserPlugin-based guests,
 // and (2) links outside of content-browsertests. At some point in time we
 // should probably merge these.

[alexmos, 2017/03/31 21:57:55]

Update this comment?

[kenrb, 2017/04/03 17:28:29]

Removed instead, it becomes obsolete with this change, since the paths are
basically merged.

 namespace {
 
-bool ContainsSurfaceId(const cc::SurfaceId& container_surface_id,
-                       RenderWidgetHostViewChildFrame* target_view) {
+class SurfaceHitTestReadyNotifier {
+ public:
+  SurfaceHitTestReadyNotifier(RenderWidgetHostViewBase* target_view);
+  ~SurfaceHitTestReadyNotifier() {}
+
+  void WaitForSurfaceReady(RenderWidgetHostViewBase* root_container);
+
+ private:
+  bool ContainsSurfaceId(const cc::SurfaceId& container_surface_id);
+
+  cc::SurfaceManager* surface_manager_;
+  cc::SurfaceId root_surface_id_;

[alexmos, 2017/03/31 21:57:55]

This appears to be unused now.

[kenrb, 2017/04/03 17:28:29]

Done.

+  RenderWidgetHostViewBase* target_view_;
+
+  DISALLOW_COPY_AND_ASSIGN(SurfaceHitTestReadyNotifier);
+};
+
+SurfaceHitTestReadyNotifier::SurfaceHitTestReadyNotifier(
+    RenderWidgetHostViewBase* target_view)
+    : target_view_(target_view) {
+  surface_manager_ = GetSurfaceManager();
+}
+
+void SurfaceHitTestReadyNotifier::WaitForSurfaceReady(
+    RenderWidgetHostViewBase* root_view) {
+  cc::SurfaceId root_surface_id = root_view->SurfaceIdForTesting();
+  while (!ContainsSurfaceId(root_surface_id)) {
+    // TODO(kenrb): Need a better way to do this. If
+    // RenderWidgetHostViewBase lifetime observer lands (see
+    // https://codereview.chromium.org/1711103002/), we can add a callback
+    // from OnSwapCompositorFrame and avoid this busy waiting, which is very
+    // frequent in tests in this file.

[alexmos, 2017/03/31 21:57:55]

"in this file" is outdated, since the tests are now elsewhere

[kenrb, 2017/04/03 17:28:29]

Done. Also reworked the comment a bit.

+    base::RunLoop run_loop;
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, run_loop.QuitClosure(), TestTimeouts::tiny_timeout());
+    run_loop.Run();
+  }
+}
+
+bool SurfaceHitTestReadyNotifier::ContainsSurfaceId(
+    const cc::SurfaceId& container_surface_id) {
   if (!container_surface_id.is_valid())
     return false;
 
   cc::Surface* container_surface =
-      GetSurfaceManager()->GetSurfaceForId(container_surface_id);
+      surface_manager_->GetSurfaceForId(container_surface_id);
   if (!container_surface || !container_surface->active_referenced_surfaces())
     return false;
 
   for (const cc::SurfaceId& id :
        *container_surface->active_referenced_surfaces()) {
-    if (id == target_view->SurfaceIdForTesting() ||
-        ContainsSurfaceId(id, target_view))
+    if (id == target_view_->SurfaceIdForTesting() || ContainsSurfaceId(id))
       return true;
   }
   return false;
@@ -1196,27 +1236,40 @@ bool ContainsSurfaceId(const cc::SurfaceId& container_surface_id,
 
 }  // namespace
 
+#if defined(USE_AURA)
 void WaitForGuestSurfaceReady(content::WebContents* guest_web_contents) {
   RenderWidgetHostViewChildFrame* child_view =
       static_cast<RenderWidgetHostViewChildFrame*>(
           guest_web_contents->GetRenderWidgetHostView());
 
-  cc::SurfaceId root_surface_id =
-      static_cast<RenderWidgetHostViewAura*>(
-          static_cast<content::WebContentsImpl*>(guest_web_contents)
-              ->GetOuterWebContents()
-              ->GetRenderWidgetHostView())
-          ->SurfaceIdForTesting();
+  RenderWidgetHostViewBase* root_view = static_cast<RenderWidgetHostViewBase*>(
+      static_cast<content::WebContentsImpl*>(guest_web_contents)
+          ->GetOuterWebContents()
+          ->GetRenderWidgetHostView());
 
-  while (!ContainsSurfaceId(root_surface_id, child_view)) {
-    base::RunLoop run_loop;
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, run_loop.QuitClosure(), TestTimeouts::tiny_timeout());
-    run_loop.Run();
-  }
+  SurfaceHitTestReadyNotifier notifier(child_view);
+  notifier.WaitForSurfaceReady(root_view);
 }
+
 #endif
 
+void WaitForChildFrameSurfaceReady(content::RenderFrameHost* child_frame) {
+  RenderWidgetHostViewBase* child_view =
+      static_cast<RenderFrameHostImpl*>(child_frame)
+          ->GetRenderWidgetHost()
+          ->GetView();
+  if (!child_view->IsRenderWidgetHostViewChildFrame())

[alexmos, 2017/03/31 21:57:54]

Do we care to handle the case where child_view is null? (e.g., if the frame
crashed?)

[kenrb, 2017/04/03 17:28:29]

Added.

+    return;
+
+  RenderWidgetHostViewBase* root_view =
+      static_cast<RenderWidgetHostViewChildFrame*>(child_view)
+          ->FrameConnectorForTesting()
+          ->GetRootRenderWidgetHostViewForTesting();
+
+  SurfaceHitTestReadyNotifier notifier(child_view);
+  notifier.WaitForSurfaceReady(root_view);
+}
+
 TitleWatcher::TitleWatcher(WebContents* web_contents,
                            const base::string16& expected_title)
     : WebContentsObserver(web_contents) {