[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 2)

content/browser/renderer_host/media/service_video_capture_device_launcher.cc

Index: content/browser/renderer_host/media/service_video_capture_device_launcher.cc
diff --git a/content/browser/renderer_host/media/service_video_capture_device_launcher.cc b/content/browser/renderer_host/media/service_video_capture_device_launcher.cc
index 94d4e0617c3ab107963fd72f075933bd7f451e66..15600454be4aa430fdf35918b53f86d405959ab3 100644
--- a/content/browser/renderer_host/media/service_video_capture_device_launcher.cc
+++ b/content/browser/renderer_host/media/service_video_capture_device_launcher.cc
@@ -4,13 +4,22 @@
 
 #include "content/browser/renderer_host/media/service_video_capture_device_launcher.h"
 
+#include "content/browser/renderer_host/media/service_launched_video_capture_device.h"
+#include "content/public/browser/browser_thread.h"
+#include "media/capture/video/video_frame_receiver_on_task_runner.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "services/video_capture/public/cpp/receiver_media_to_mojo_adapter.h"
+
 namespace content {
 
 ServiceVideoCaptureDeviceLauncher::ServiceVideoCaptureDeviceLauncher(
     video_capture::mojom::DeviceFactoryPtr* device_factory)
-    : device_factory_(device_factory) {}
+    : device_factory_(device_factory), state_(State::READY_TO_LAUNCH) {}
 
-ServiceVideoCaptureDeviceLauncher::~ServiceVideoCaptureDeviceLauncher() {}
+ServiceVideoCaptureDeviceLauncher::~ServiceVideoCaptureDeviceLauncher() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(state_ == State::READY_TO_LAUNCH);
+}
 
 void ServiceVideoCaptureDeviceLauncher::LaunchDeviceAsync(
     const std::string& device_id,
@@ -19,11 +28,104 @@ void ServiceVideoCaptureDeviceLauncher::LaunchDeviceAsync(
     base::WeakPtr<media::VideoFrameReceiver> receiver,
     Callbacks* callbacks,
     base::OnceClosure done_cb) {
-  NOTIMPLEMENTED();
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(state_ == State::READY_TO_LAUNCH);
+
+  if (stream_type != content::MEDIA_DEVICE_VIDEO_CAPTURE) {
+    // This launcher only supports MEDIA_DEVICE_VIDEO_CAPTURE.
+    NOTREACHED();
+    return;
+  }
+
+  if (!device_factory_->is_bound()) {
+    // This can happen when the ServiceVideoCaptureProvider owning
+    // |device_factory_| loses connection to the service process and resets
+    // |device_factory_|.
+    OnDeviceCreationFailed(callbacks, std::move(done_cb));
+    return;
+  }
+  video_capture::mojom::DevicePtr device;
+  (*device_factory_)
+      ->CreateDevice(
+          device_id, mojo::MakeRequest(&device),
+          base::Bind(
+              // Use of Unretained |this| is safe, because |done_cb| guarantees
+              // that |this| stays alive.
+              &ServiceVideoCaptureDeviceLauncher::OnCreateDeviceCallback,
+              base::Unretained(this), params, base::Passed(&device),
+              std::move(receiver), callbacks, base::Passed(&done_cb)));
+  state_ = State::DEVICE_START_IN_PROGRESS;
 }
 
 void ServiceVideoCaptureDeviceLauncher::AbortLaunch() {
-  NOTIMPLEMENTED();
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (state_ == State::DEVICE_START_IN_PROGRESS)
+    state_ = State::DEVICE_START_ABORTING;
+}
+
+void ServiceVideoCaptureDeviceLauncher::OnCreateDeviceCallback(
+    const media::VideoCaptureParams& params,
+    video_capture::mojom::DevicePtr device,
+    base::WeakPtr<media::VideoFrameReceiver> receiver,
+    Callbacks* callbacks,
+    base::OnceClosure done_cb,
+    video_capture::mojom::DeviceAccessResultCode result_code) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  switch (result_code) {
+    case video_capture::mojom::DeviceAccessResultCode::SUCCESS:
+      OnDeviceCreatedSuccessfully(params, std::move(device),
+                                  std::move(receiver), callbacks,
+                                  std::move(done_cb));
+      return;
+    case video_capture::mojom::DeviceAccessResultCode::ERROR_DEVICE_NOT_FOUND:
+    case video_capture::mojom::DeviceAccessResultCode::NOT_INITIALIZED:
+      OnDeviceCreationFailed(callbacks, std::move(done_cb));
+      return;
+  }
+}
+
+void ServiceVideoCaptureDeviceLauncher::OnDeviceCreatedSuccessfully(
+    const media::VideoCaptureParams& params,
+    video_capture::mojom::DevicePtr device,
+    base::WeakPtr<media::VideoFrameReceiver> receiver,
+    Callbacks* callbacks,
+    base::OnceClosure done_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  State state_copy = state_;
+  state_ = State::READY_TO_LAUNCH;

[mcasas, 2017/05/05 22:41:02]

If we move this update of |state_| (resp. the one in
l.123 to l.76 (resp., 82), OnDeviceCreatedSuccessfully()
(resp OnDeviceCreationFailed()) do not operate on
any member variables, and can be made file-static.

[chfremer, 2017/05/08 17:01:54]

What is the advantage of having these two methods be file-static?

I see several downsides:
1. These methods move to the top of the file, making it harder to read the file
from top to bottom.
2. Since the methods take different paths depending on the |State| we are coming
from, we would have to pass that state in, and to do that we would have to make
the type |State| public.
3. Since OnDeviceCreationFailed() is called twice, moving the logic for copying
the state before updating it out of the method would require duplicating it.

[mcasas, 2017/05/08 21:08:04]

Reduces the exposed surface of the class (the one people 
read), and the vtable.  Also:
"minimize the code in headers" :
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi...

[chfremer, 2017/05/08 21:56:31]

Hmm, I now understand that the goal is to free up cognitive load from a reader.
As per the downsides I mentioned, I am not quite sure that making these
non-static private methods static is worth the benefits, though.

How strongly do you feel about this? If you still prefer it, I'll follow your
suggestion and refactor things to move the methods into cc-file-scope.

I don't think they appear in the vtable, since they are not virtual.
And a reader not interested in the implementation details, should not care much
about the private section of a class declaration.

[chfremer, 2017/05/09 18:41:33]

Moved the two private methods to cc-file-scope and renamed them to
ConcludeLaunchDeviceWithSuccess() and ConcludeLaunchDeviceWithFailure().

+
+  if (state_copy == State::DEVICE_START_ABORTING) {
+    device.reset();
+    callbacks->OnDeviceLaunchAborted();
+    base::ResetAndReturn(&done_cb).Run();
+    return;
+  }
+
+  auto receiver_adapter =
+      base::MakeUnique<video_capture::ReceiverMediaToMojoAdapter>(
+          base::MakeUnique<media::VideoFrameReceiverOnTaskRunner>(
+              std::move(receiver),
+              BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)));
+  video_capture::mojom::ReceiverPtr receiver_proxy;
+  mojo::MakeStrongBinding<video_capture::mojom::Receiver>(
+      std::move(receiver_adapter), mojo::MakeRequest(&receiver_proxy));
+  device->Start(params, std::move(receiver_proxy));
+  callbacks->OnDeviceLaunched(
+      base::MakeUnique<ServiceLaunchedVideoCaptureDevice>(std::move(device)));
+  base::ResetAndReturn(&done_cb).Run();
+}
+
+void ServiceVideoCaptureDeviceLauncher::OnDeviceCreationFailed(
+    Callbacks* callbacks,
+    base::OnceClosure done_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  State state_copy = state_;
+  state_ = State::READY_TO_LAUNCH;
+  if (state_copy == State::DEVICE_START_ABORTING)
+    callbacks->OnDeviceLaunchAborted();
+  else
+    callbacks->OnDeviceLaunchFailed();
+  base::ResetAndReturn(&done_cb).Run();
 }
 
 }  // namespace content