[Mojo Video Capture] Do not instantiate in-process VideoCaptureSystem when using service

content/browser/renderer_host/media/in_process_video_capture_device_launcher.h

 // Copyright 2017 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.
 
 #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_IN_PROCESS_VIDEO_CAPTURE_DEVICE_LAUNCHER_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_IN_PROCESS_VIDEO_CAPTURE_DEVICE_LAUNCHER_H_
 
 #include "content/browser/renderer_host/media/video_capture_controller.h"
 #include "content/browser/renderer_host/media/video_capture_provider.h"
 #include "content/public/common/media_stream_request.h"
 #include "media/capture/video/video_capture_device.h"
 #include "media/capture/video/video_capture_device_client.h"
 #include "media/capture/video/video_capture_device_descriptor.h"
 #include "media/capture/video/video_capture_system.h"
 
 namespace content {
 
 // Implementation of BuildableVideoCaptureDevice that creates capture devices
 // in the same process as it is being operated on, which must be the Browser
 // process. The devices are operated on the given |device_task_runner|.
 // Instances of this class must be operated from the Browser process IO thread.
 class InProcessVideoCaptureDeviceLauncher : public VideoCaptureDeviceLauncher {
  public:
   InProcessVideoCaptureDeviceLauncher(
       scoped_refptr<base::SingleThreadTaskRunner> device_task_runner,
-      media::VideoCaptureSystem* video_capture_system);
+      media::VideoCaptureSystem* optional_video_capture_system);
   ~InProcessVideoCaptureDeviceLauncher() override;
 
   void LaunchDeviceAsync(const std::string& device_id,
                          MediaStreamType stream_type,
                          const media::VideoCaptureParams& params,
                          base::WeakPtr<media::VideoFrameReceiver> receiver,
                          Callbacks* callbacks,
                          base::OnceClosure done_cb) override;
 
   void AbortLaunch() override;
@@ skipping 28 matching lines @@
       std::unique_ptr<media::VideoCaptureDeviceClient> client,
       ReceiveDeviceCallback result_callback);
 
   void DoStartDesktopCaptureOnDeviceThread(
       const std::string& device_id,
       const media::VideoCaptureParams& params,
       std::unique_ptr<media::VideoCaptureDeviceClient> client,
       ReceiveDeviceCallback result_callback);
 
   const scoped_refptr<base::SingleThreadTaskRunner> device_task_runner_;
-  media::VideoCaptureSystem* const video_capture_system_;
+  media::VideoCaptureSystem* const optional_video_capture_system_;

[mcasas, 2017/05/16 22:22:22]

I think having a naked pointer here is enough
to imply that it can be nullptr, so the prefix 
optional_ is unnecessary.  Also, if you really want
to convey the optionality, consider base::Optional
which in its uses [1] states:
[1]
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...

[chfremer, 2017/05/16 22:45:02]

I disagree with the idea that the raw pointer would be enough to suggest it is
optional, since there are many cases where a raw pointer is used for
non-optional non-owned references.

I feel that the use of base::Optional<> is a bit heavyweight for raw pointer
types (since they already allow nullptr as a value). And I feel that using the
variable name conveys the optionality much more strongly than the type, because
the variable name appears explicitly at every usage site. After this
consideration, I still feel the proposed state is the best option. If you still
prefer one of the other two options, let me know and I'll make the change.

[mcasas, 2017/05/17 23:19:03]

A quick search [1] yields no |optional_| members in content/,
so I'd say, non-owned raw pointers should be checked for
non-nullness.  miu@ WDYT?


[1] grep -rn "optional_" content/ | grep "\.h"

[chfremer, 2017/05/18 17:06:16]

It appears that my reasoning for why I feel that adding the "optional_" prefix
is a good idea did not convince you, so I am removing the prefix.

The fact that there are no |optional_*| members in content/ does not change my
opinion that adding it here would have been a (minor) improvement, though.

   State state_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_IN_PROCESS_VIDEO_CAPTURE_DEVICE_LAUNCHER_H_