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

content/browser/renderer_host/media/in_process_video_capture_provider.h

 // Copyright 2016 The Chromium Authors. All rights reserved.

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

This class was landed in 2017:

https://chromium.googlesource.com/chromium/src/+log/eb9bfd648536ce835bb620265...

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

Done.

 // 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_SYSTEM_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_IN_PROCESS_VIDEO_CAPTURE_SYSTEM_H_
 
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "content/browser/renderer_host/media/video_capture_provider.h"
 #include "media/capture/video/video_capture_system.h"
 
 namespace content {
 
 class CONTENT_EXPORT InProcessVideoCaptureProvider

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

nit: CONTENT_EXPORT might not be needed since all
its usage is from inside content (same .dll) -- but only
maybe, because of video_capture_manager_unittest.cc	

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

True. Just tried, but turns out you were right about content_unittests requiring
the export.

     : public VideoCaptureProvider {
  public:
   InProcessVideoCaptureProvider(
+      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);
-  ~InProcessVideoCaptureProvider() override;
 
   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.

 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_IN_PROCESS_VIDEO_CAPTURE_SYSTEM_H_