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

content/browser/renderer_host/media/in_process_video_capture_provider.h

Index: content/browser/renderer_host/media/in_process_video_capture_provider.h
diff --git a/content/browser/renderer_host/media/in_process_video_capture_provider.h b/content/browser/renderer_host/media/in_process_video_capture_provider.h
index 947d49454e2d383a3264f4c812ba9b1462439b27..e4bebc59721b951bb506a63815ea675d296b9be4 100644
--- a/content/browser/renderer_host/media/in_process_video_capture_provider.h
+++ b/content/browser/renderer_host/media/in_process_video_capture_provider.h
@@ -16,21 +16,29 @@ class CONTENT_EXPORT InProcessVideoCaptureProvider
     : public VideoCaptureProvider {
  public:
   InProcessVideoCaptureProvider(
-      std::unique_ptr<media::VideoCaptureSystem> video_capture_system,
+      std::unique_ptr<media::VideoCaptureSystem> optional_video_capture_system,
       scoped_refptr<base::SingleThreadTaskRunner> device_task_runner);
+
   ~InProcessVideoCaptureProvider() override;
 
+  static std::unique_ptr<VideoCaptureProvider>
+  CreateInstanceForNonDeviceCapture(
+      scoped_refptr<base::SingleThreadTaskRunner> device_task_runner);
+
+  static std::unique_ptr<VideoCaptureProvider> CreateInstance(
+      std::unique_ptr<media::VideoCaptureSystem> video_capture_system,
+      scoped_refptr<base::SingleThreadTaskRunner> device_task_runner);
+
   void Uninitialize() override;
 
   void GetDeviceInfosAsync(
-      const base::Callback<void(
-          const std::vector<media::VideoCaptureDeviceInfo>&)>& result_callback)
-      override;
+      const GetDeviceInfosCallback& result_callback) override;
 
   std::unique_ptr<VideoCaptureDeviceLauncher> CreateDeviceLauncher() override;
 
  private:
-  const std::unique_ptr<media::VideoCaptureSystem> video_capture_system_;
+  const std::unique_ptr<media::VideoCaptureSystem>
+      optional_video_capture_system_;

[mcasas, 2017/05/18 17:42:12]

With the same rationale as in the other comments, I think
the |optional_| prefix here is superfluous since this unique ptr 
can perfectly be not initialized to anything meaningful, as proven
by the lack of DCHECK()s and the test before use in l.40.  

The absence of the DCHECK is not trivial: you should strive 
to document by using pre-post conditions enforced by the 
compiler and not (only) by naming conventions.  OTOH there's 
plenty of examples of member unique_ptrs that are 
tested-before-use and it seems an established pattern versus 
the prefix, which is not present in content/.  

FTR I'd say it's appropriate to add a comment saying that this
member can be null and in that case you fall back to whatever.
With that, both the casual reader would gets the information, 
and the compiler will enforce your preconditions.

[chfremer, 2017/05/18 18:28:32]

A DCHECK only enforces at runtime, though. At "read-time" the reader still has
to go and find the DCHECKs in the implementation to find out whether or not a
member is optional.
 
Agreed that it can be done this way. And I forgot to remove the "optional_"
prefix here.

Done.

   // The message loop of media stream device thread, where VCD's live.
   scoped_refptr<base::SingleThreadTaskRunner> device_task_runner_;

[mcasas, 2017/05/18 17:42:12]

nit: const?

[chfremer, 2017/05/18 18:28:32]

Done.

 };

[mcasas, 2017/05/18 17:42:12]

This class needs a thread checker.

[chfremer, 2017/05/18 18:28:32]

Done.