Rewrite WebContentsVideoCaptureDeviceTest.

content/browser/renderer_host/media/web_contents_video_capture_device.cc

Index: content/browser/renderer_host/media/web_contents_video_capture_device.cc
diff --git a/content/browser/renderer_host/media/web_contents_video_capture_device.cc b/content/browser/renderer_host/media/web_contents_video_capture_device.cc
index 8b30cb18d8b4aa4995101dadc658e0537be84d18..d760b1c13806203dc164a9c34cb0794abcd1926e 100644
--- a/content/browser/renderer_host/media/web_contents_video_capture_device.cc
+++ b/content/browser/renderer_host/media/web_contents_video_capture_device.cc
@@ -73,7 +73,7 @@
 #include "ui/gfx/rect.h"
 #include "ui/gfx/skia_util.h"
 
-// Used to self-trampoline invocation of methods to the approprate thread.  This
+// Used to self-trampoline invocation of methods to the appropriate thread. This
 // should be used sparingly, only when it's not clear which thread is invoking a
 // method.
 #define ENSURE_INVOKED_ON_THREAD(thread, ...) {  \
@@ -146,10 +146,6 @@ class BackingStoreCopier : public WebContentsObserver {
 
   virtual ~BackingStoreCopier();
 
-  // If non-NULL, use the given |override| to access the backing store.
-  // This is used for unit testing.
-  void SetRenderWidgetHostForTesting(RenderWidgetHost* override);
-
   // Starts the copy from the backing store. Must be run on the UI
   // BrowserThread. Resulting frame is conveyed back to |consumer|.
   void StartCopy(const scoped_refptr<CaptureMachine>& consumer,
@@ -205,10 +201,6 @@ class BackingStoreCopier : public WebContentsObserver {
   // Last known RenderView size.
   gfx::Size last_view_size_;
 
-  // If the following is NULL (normal behavior), the implementation should
-  // access RenderWidgetHost via web_contents().
-  RenderWidgetHost* rwh_for_testing_;
-
   DISALLOW_COPY_AND_ASSIGN(BackingStoreCopier);
 };
 
@@ -319,8 +311,9 @@ class VideoFrameDeliverer {
 
 BackingStoreCopier::BackingStoreCopier(int render_process_id,
                                        int render_view_id)
-    : render_process_id_(render_process_id), render_view_id_(render_view_id),
-      fullscreen_widget_id_(MSG_ROUTING_NONE), rwh_for_testing_(NULL) {}
+    : render_process_id_(render_process_id),
+      render_view_id_(render_view_id),
+      fullscreen_widget_id_(MSG_ROUTING_NONE) {}
 
 BackingStoreCopier::~BackingStoreCopier() {
   DCHECK(!web_contents());
@@ -369,11 +362,6 @@ void BackingStoreCopier::LookUpAndObserveWebContents() {
   }
 }
 
-void BackingStoreCopier::SetRenderWidgetHostForTesting(
-    RenderWidgetHost* override) {
-  rwh_for_testing_ = override;
-}
-
 VideoFrameRenderer::VideoFrameRenderer()
     : render_thread_("WebContentsVideo_RenderThread") {
   output_[0].in_use = false;
@@ -673,12 +661,6 @@ class CaptureMachine
 
   CaptureMachine(int render_process_id, int render_view_id);
 
-  // Sets the capture source to the given |override| for unit testing.
-  // Also, |destroy_cb| will be invoked after CaptureMachine is fully destroyed
-  // (to synchronize tear-down).
-  void InitializeForTesting(RenderWidgetHost* override,

[miu, 2013/02/20 06:45:10]

For the tests, how can you be sure CaptureMachine has finished using the "UI
thread" before you destroy it in WCVCDTest::TearDown()?

[ncarter (slow), 2013/02/22 02:16:11]

This is moot now, but the old code did reliably run DoShutdownTasksOnUIThread()
(it's posted to the UI thread and thus would be there when we pumped the message
queue).  What it didn't do reliably, was to actually join the threads and free
the CaptureMachine and its components before test-case TearDown().

So I've restored the destroy_cb, though it uses a RunLoop now instead of the
ConditionVar.

-                            const base::Closure& destroy_cb);
-
   // Synchronously sets/unsets the consumer.  Pass |consumer| as NULL to remove
   // the reference to the consumer; then, once this method returns,
   // CaptureMachine will no longer invoke callbacks on the old consumer from any
@@ -773,8 +755,6 @@ class CaptureMachine
   VideoFrameRenderer renderer_;
   VideoFrameDeliverer deliverer_;
 
-  base::Closure destroy_cb_;  // Invoked once CaptureMachine is destroyed.
-
   DISALLOW_COPY_AND_ASSIGN(CaptureMachine);
 };
 
@@ -788,12 +768,6 @@ CaptureMachine::CaptureMachine(int render_process_id, int render_view_id)
   manager_thread_.Start();
 }
 
-void CaptureMachine::InitializeForTesting(RenderWidgetHost* override,
-                                          const base::Closure& destroy_cb) {
-  copier_.SetRenderWidgetHostForTesting(override);
-  destroy_cb_ = destroy_cb;
-}
-
 void CaptureMachine::SetConsumer(
     media::VideoCaptureDevice::EventHandler* consumer) {
   consumer_.SetConsumer(consumer);
@@ -905,12 +879,7 @@ void CaptureMachine::Destruct(const CaptureMachine* x) {
 
 // static
 void CaptureMachine::DeleteFromOutsideThread(const CaptureMachine* x) {
-  const base::Closure run_after_delete = x->destroy_cb_;
-  // Note: Thread joins are about to happen here (in ~CaptureThread()).

[miu, 2013/02/20 06:45:10]

This comment should be retained (it's not related to the destroy_cb_ stuff).  It
might not be clear that thread joining will happen on the call to delete.

[ncarter (slow), 2013/02/22 02:16:11]

Done.

   delete x;
-  if (!run_after_delete.is_null()) {
-    run_after_delete.Run();
-  }
 }
 
 void CaptureMachine::TransitionStateTo(State next_state) {
@@ -1101,31 +1070,27 @@ void BackingStoreCopier::StartCopy(
     int desired_height) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  RenderWidgetHost* rwh;
-  if (rwh_for_testing_) {
-    rwh = rwh_for_testing_;
-  } else {
-    if (!web_contents()) {  // No source yet.
-      LookUpAndObserveWebContents();
-      if (!web_contents()) {  // No source ever.
-        consumer->OnSnapshotFailed(CaptureMachine::NO_SOURCE, frame_number);
-        return;
-      }
+  if (!web_contents()) {  // No source yet.
+    LookUpAndObserveWebContents();
+    if (!web_contents()) {  // No source ever.
+      consumer->OnSnapshotFailed(CaptureMachine::NO_SOURCE, frame_number);
+      return;
     }
+  }
 
-    if (fullscreen_widget_id_ != MSG_ROUTING_NONE) {
-      RenderProcessHost* process = web_contents()->GetRenderProcessHost();
-      rwh = process ? process->GetRenderWidgetHostByID(fullscreen_widget_id_)
-                    : NULL;
-    } else {
-      rwh = web_contents()->GetRenderViewHost();
-    }
+  RenderWidgetHost* rwh;
+  if (fullscreen_widget_id_ != MSG_ROUTING_NONE) {
+    RenderProcessHost* process = web_contents()->GetRenderProcessHost();
+    rwh = process ? process->GetRenderWidgetHostByID(fullscreen_widget_id_)
+                  : NULL;
+  } else {
+    rwh = web_contents()->GetRenderViewHost();
+  }
 
-    if (!rwh) {
-      // Transient failure state (e.g., a RenderView is being replaced).
-      consumer->OnSnapshotFailed(CaptureMachine::TRANSIENT_ERROR, frame_number);
-      return;
-    }
+  if (!rwh) {
+    // Transient failure state (e.g., a RenderView is being replaced).
+    consumer->OnSnapshotFailed(CaptureMachine::TRANSIENT_ERROR, frame_number);
+    return;
   }
 
   RenderWidgetHostViewPort* view =
@@ -1232,14 +1197,6 @@ WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
     : device_name_(name),
       capturer_(new CaptureMachine(render_process_id, render_view_id)) {}
 
-WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
-    RenderWidgetHost* test_source, const base::Closure& destroy_cb)
-    : capturer_(new CaptureMachine(-1, -1)) {
-  device_name_.device_name = "WebContentsForTesting";
-  device_name_.unique_id = "-1:-1";
-  capturer_->InitializeForTesting(test_source, destroy_cb);
-}
-
 WebContentsVideoCaptureDevice::~WebContentsVideoCaptureDevice() {
   DVLOG(2) << "WebContentsVideoCaptureDevice@" << this << " destroying.";
 }
@@ -1265,12 +1222,6 @@ media::VideoCaptureDevice* WebContentsVideoCaptureDevice::Create(
       name, render_process_id, render_view_id);
 }
 
-// static
-media::VideoCaptureDevice* WebContentsVideoCaptureDevice::CreateForTesting(
-    RenderWidgetHost* test_source, const base::Closure& destroy_cb) {
-  return new WebContentsVideoCaptureDevice(test_source, destroy_cb);
-}
-
 void WebContentsVideoCaptureDevice::Allocate(
     int width, int height, int frame_rate,
     VideoCaptureDevice::EventHandler* consumer) {