Video Capture Mojo (1.4a.a): Add service configurator interface

services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom

Index: services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom
diff --git a/services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom b/services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..1d8ef279b050b08f43d6f41152257bbe2bb7f23e
--- /dev/null
+++ b/services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom
@@ -0,0 +1,16 @@
+// 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.
+
+module video_capture.mojom;
+
+import "services/video_capture/public/interfaces/video_capture_device_factory.mojom";
+
+// Allows clients to create and configure set of fake capture devices that will
+// be exposed through the fake factory offered by VideoCaptureService.
+interface FakeVideoCaptureDeviceFactoryConfigurator {
+  // Returns the descriptor under which the newly added fake device will be
+  // advertised in EnumerateDeviceDescriptors().
+  AddFakeVideoCaptureDevice()
+      => (VideoCaptureDeviceDescriptor descriptor);

[mcasas, 2016/08/16 02:18:13]

I still feel that this should be 
a) in video_capture_device_factory.mojom, and not in a .mojom file of its 
own
b) should not just say "Add", since the first time we create a FakeVCD,
it should not remove all "normal" system devices. ("Create" or
"Append" might be better, but read on).
c) should not refer to FakeVCD, because there is another FileVCD.
(I sugggest "Test").

New proposal: In the browser, we know with certainty on startup
if the VCDF is supposed to be full of test devices or not, and how
many. So: add a new method called e.g. 
  SwitchOnTestMode(number_of_devices).
to the interface VideoCaptureDeviceFactory. We can either
enforce this method to be called before EnumerateDevices() 
(adding DCHECKs for this in the code), or we can invalidate
the VCDDescriptors generated so far and kill the potentiall 
associated capture sessions, but the latter seems overkill to
me.

[chfremer, 2016/08/16 16:48:02]

This ambiguity of what should happen when switching the factory to a test mode
is why I want to avoid the switchable factory and am proposing to use two
separate factories instead. The two-factory design does not suffer from this
ambiguity and we don't need to put any extra documentation to explain correct
usage or runtime checks to guard against wrong usage.   
Acknowledged, but I would love to see the argument for what makes you feel this
way.
An argument in favor of having the configurator interface separate is that it
does not force clients that are only interested in using the factory interface
to also know about fake devices. Setting up fake devices is a separate
responsibility, so we should not allow clients to handle them separately.
I feel the cost of having an extra *.mojom is lower than the cost of mixing
responsibilities.
I am not sure I understand your concern correctly.
My idea is that a few CLs down the road, this interface will not only support
fake devices, but also file-based devices and possibly mock devices. In this CL,
we only have added support for fake devices. That is why there is currently only
a single method specifically for adding fake devices. It is called "add",
because it adds a new fake device to the fake factory.
Did you mean that the second factory should be called
"TestVideoCaptureDeviceFactory" instead of "FakeVideoCaptureDeviceFactory"?

[mcasas, 2016/08/16 21:43:02]

Hmm I see no ambiguity :) - VCDF works in normal mode
or in test mode ("test" mode would override "normal" mode).
I think your pointis: 2 interfaces, one for normal operation, 
one for "test", versus one single interface with a specific 
method. I think the second one is simpler to a newcomer, 
hence my opinion.
VCDF service will need to be instantiated somewhere,
(that'd be the VCManager nowadays, right?) which is where
the parsing of the command line flags could happen, so
something like (pseudocode, sorry):

factory = connect_to_vcdf_service();
if (flag-for-fake-device-is-on)
  factory->useFakeVCDs(2 /* num_devices */);
else if (flag-for-file-device-is-on)
  factory->useFileVCD(file-name);

the client of the factory interface must know how 
to connect and use it, so why shouldn't it know 
about its special modes? And, let me insist, there 
is no coexistence of either mode (using Fakes, 
using Files, or "normal");

Also I'm not sure why a client would need the |descriptor|.
Isn't it the purpose of the fake-mode to mimic the 
normal-mode? Any client, test or not, should do a 
EnumerateDeviceDescriptors() and then 
CreateDeviceProxy(), but if this code here returns
a VideoCaptureDeviceDescriptor, callers might
feel tempted to skip the enumeration, and just call
directly CreateDeviceProxy().
Why not adding FileVCDs as well? The VCD interface is the
same, and for the time being you can access the CommandLine
from services/ (examples abound).

[chfremer, 2016/08/16 23:22:45]

The ambiguity I see is that the interface allows clients to switch the mode at
an 
"unconvenient" time, i.e. any time after it has already been in use. 

My design proposal is to have 1 interface for operation (production or test,
both 
use the same interface) and 1 separate interface for configuration. To me this 
feels cleaner and simpler than 1 interface for both operation and configuration,

because it separates responsibilities.
My point here is that the client who connects to the service and configures the
special modes 
does not have to be the same as the client who eventually uses the factory.
Hence the two separate interfaces.

I understand that there is currently no coexistence of different modes.
The proposed design makes it easy for the client who chooses the factory and
configures the modes to replicate exactly that.
But it does seem useful for tests to be able to configure more than one
fake/file/mock device and it does not add 
significant complexity, so I don't see a need to have the interface design
enforce single fake/file device modes.
 
My idea here was to give clients (tests) that set up more than one fake device
the ability 
to identify which one is which without having to go through
EnumerateDeviceDescriptors and 
"guessing" what identifier the service has generated. I think it is reasonable
for a test 
that both sets up fake devices and exercises the factory interface to skip the 
EnumerateDeviceDescriptors() step. There are actually test in the follow-up CLs
that do 
exactly that.
Sorry if my explanations were a bit unclear. I absolutely do want to add
FileVCDs as well.
But since they are not yet needed in the current tests, I have not done it just
yet.

We can have the VideoCaptureManager check the CommandLine and then pick and
configure the 
corresponding VideoCaptureDeviceFactory accordingly.
For the unit (service) tests, we do no need to go through the CommandLine.

Please let me know if this makes sense. If not, I am always happy to chat about
this offline 
in more detail.

+};