[Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice

content/browser/renderer_host/media/video_capture_device_entry.cc

+// Copyright 2016 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 "content/browser/renderer_host/media/video_capture_device_entry.h"
+
+namespace {
+
+// Counter used for identifying a DeviceRequest to start a capture device.
+static int g_device_start_id = 0;
+}
+
+namespace content {
+
+VideoCaptureDeviceEntry::VideoCaptureDeviceEntry(
+    const std::string& id,
+    MediaStreamType stream_type,
+    const media::VideoCaptureParams& params,
+    std::unique_ptr<BuildableVideoCaptureDevice> buildable_device)
+    : serial_id_(g_device_start_id++),
+      id_(id),
+      stream_type_(stream_type),
+      parameters_(params),
+      buildable_device_(std::move(buildable_device)) {}
+
+VideoCaptureDeviceEntry::~VideoCaptureDeviceEntry() = default;
+
+void VideoCaptureDeviceEntry::CreateAndStartDeviceAsync(

[miu, 2017/03/11 01:13:22]

It looks like every method in this class is just a one-liner hop to a method
call in |controller_| or |buildable_device_|. So, we're adding function call
overhead without any benefit, and lots of LOC that don't add any value. IMHO, it
would be better to delete all of the methods, and just provide accessors:

  BuildableVideoCaptureDevice* device();
  VideoCaptureController* controller();

Then, instead of callers doing this:

  entry->TakePhoto(...);

They do this:

  entry->device()->TakePhoto(...);

--------------

Or, another idea: Maybe VideoCaptureDeviceEntry should be merged into
VideoCaptureController? It already has a "composed of" relationship to
VideoCaptureController; and it wouldn't be a far stretch for
VideoCaptureController to own |buildable_device_|.

[chfremer, 2017/03/13 22:02:03]

I agree that this class, now that all interesting logic has moved into
|buildable_device_|, is not really doing much. I wouldn't say it does nothing,
though. It does the |g_device_start_id| thing, and (more importantly) it hides
the implementation detail that there is a "controller" and a "buildable device"
from the VideoCaptureManager. This has value. But I agree that it may not be
enough to justify the existence of this class.

Note that, for clarity, this CL was originally meant to contain nothing other
than the cut made into VideoCaptureManager.
But I really like the idea of merging it into VideoCaptureController. Let me try
doing that.

Done.

+    const media::VideoCaptureParams& params,
+    BuildableDeviceCallbacks* callbacks,
+    std::unique_ptr<Ownership> context_reference) {
+  buildable_device_->CreateAndStartDeviceAsync(
+      this, &controller_, params, callbacks, std::move(context_reference));
+}
+
+void VideoCaptureDeviceEntry::ReleaseDeviceAsync(
+    std::unique_ptr<Ownership> context_reference) {
+  buildable_device_->ReleaseDeviceAsync(&controller_,
+                                        std::move(context_reference));
+}
+
+void VideoCaptureDeviceEntry::OnLog(const std::string& message) {
+  controller_.OnLog(message);
+}
+
+void VideoCaptureDeviceEntry::OnError() {
+  controller_.OnError();
+}
+
+bool VideoCaptureDeviceEntry::HasDevice() const {
+  return buildable_device_->HasDevice();
+}
+
+bool VideoCaptureDeviceEntry::CorrespondsToController(
+    const VideoCaptureController* controller) const {
+  return &controller_ == controller;
+}
+
+bool VideoCaptureDeviceEntry::HasActiveClient() {
+  return controller_.HasActiveClient();
+}
+
+bool VideoCaptureDeviceEntry::HasPausedClient() {
+  return controller_.HasPausedClient();
+}
+
+void VideoCaptureDeviceEntry::AddClient(
+    VideoCaptureControllerID id,
+    VideoCaptureControllerEventHandler* event_handler,
+    media::VideoCaptureSessionId session_id,
+    const media::VideoCaptureParams& params) {
+  controller_.AddClient(id, event_handler, session_id, params);
+}
+
+void VideoCaptureDeviceEntry::StopSession(int session_id) {
+  controller_.StopSession(session_id);
+}
+
+base::WeakPtr<VideoCaptureController>
+VideoCaptureDeviceEntry::GetControllerWeakPtrForIOThread() {
+  return controller_.GetWeakPtrForIOThread();
+}
+
+media::VideoCaptureFormat VideoCaptureDeviceEntry::GetVideoCaptureFormat() {
+  return controller_.GetVideoCaptureFormat();
+}
+
+void VideoCaptureDeviceEntry::GetPhotoCapabilities(
+    media::VideoCaptureDevice::GetPhotoCapabilitiesCallback callback) const {
+  buildable_device_->GetPhotoCapabilities(std::move(callback));
+}
+
+void VideoCaptureDeviceEntry::SetPhotoOptions(
+    media::mojom::PhotoSettingsPtr settings,
+    media::VideoCaptureDevice::SetPhotoOptionsCallback callback) {
+  buildable_device_->SetPhotoOptions(std::move(settings), std::move(callback));
+}
+
+void VideoCaptureDeviceEntry::TakePhoto(
+    media::VideoCaptureDevice::TakePhotoCallback callback) {
+  buildable_device_->TakePhoto(std::move(callback));
+}
+
+void VideoCaptureDeviceEntry::MaybeSuspend() {
+  buildable_device_->MaybeSuspendDevice();
+}
+
+void VideoCaptureDeviceEntry::Resume() {
+  buildable_device_->ResumeDevice();
+}
+
+void VideoCaptureDeviceEntry::RequestRefreshFrame() {
+  buildable_device_->RequestRefreshFrame();
+}
+
+void VideoCaptureDeviceEntry::SetDesktopCaptureWindowIdAsync(
+    gfx::NativeViewId window_id,
+    std::unique_ptr<Ownership> context_reference) {
+  buildable_device_->SetDesktopCaptureWindowIdAsync(
+      window_id, std::move(context_reference));
+}
+
+}  // namespace content