[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 2)

content/browser/renderer_host/media/service_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_SERVICE_VIDEO_CAPTURE_DEVICE_LAUNCHER_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_SERVICE_VIDEO_CAPTURE_DEVICE_LAUNCHER_H_
 
 #include "content/browser/renderer_host/media/video_capture_provider.h"
 #include "content/public/common/media_stream_request.h"
 #include "services/video_capture/public/interfaces/device_factory.mojom.h"
@@ skipping 13 matching lines @@
                          MediaStreamType stream_type,
                          const media::VideoCaptureParams& params,
                          base::WeakPtr<media::VideoFrameReceiver> receiver,
                          Callbacks* callbacks,
                          base::OnceClosure done_cb) override;
   void AbortLaunch() override;
 
   void OnUtilizationReport(int frame_feedback_id, double utilization);
 
  private:
+  enum class State {
+    READY_TO_LAUNCH,
+    DEVICE_START_IN_PROGRESS,
+    DEVICE_START_ABORTING
+  };
+
+  void OnCreateDeviceCallback(
+      const media::VideoCaptureParams& params,
+      video_capture::mojom::DevicePtr device,
+      base::WeakPtr<media::VideoFrameReceiver> receiver,
+      Callbacks* callbacks,
+      base::OnceClosure done_cb,
+      video_capture::mojom::DeviceAccessResultCode result_code);
+  void OnDeviceCreatedSuccessfully(
+      const media::VideoCaptureParams& params,
+      video_capture::mojom::DevicePtr device,
+      base::WeakPtr<media::VideoFrameReceiver> receiver,
+      Callbacks* callbacks,
+      base::OnceClosure done_cb);
+  void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb);

[mcasas, 2017/05/05 22:41:02]

Actually since OnDeviceCreatedSuccessfully()
and OnDeviceCreationFailed() are not bound as
callbacks, they can drop the "On" name prefix.

[chfremer, 2017/05/08 17:01:54]

I wasn't aware that an "On" prefix is reserved for things that get bound as
callbacks. Is that really the case?

To me, the "On" prefix communicates that the method is intended to get called as
a reaction of some event, regardless of the mechanism that makes that call. If a
method does not use the "On" prefix, I would typically expect it to start with a
verb. A method name "DeviceCreatedSuccessfully" feels weird to me.

[miu, 2017/05/08 20:21:11]

FWIW, I tend to use:

  OnSomething() - A method that could be called zero, one, or multiple times;
loose/external module coupling; generally bound to a callback that is run by
some external code.

  VerbNextStepAction() - A method that is expected to be called once during the
workflow; usually called within a module or bound to a callback within the
module.

  WillDoSomething() - A method that is guaranteed to run just before an action
will take place.

  DidDoSomething() - A method that is guaranteed to run just after an action
took place.

[chfremer, 2017/05/08 20:51:46]

Looks reasonable (even though I am still not quite sold on the WillDoSomething
and DidDoSomething, because it does not unambiguously tell me _who_ is doing,
caller or callee).

So for the example at hand, would the OnSomething() case be a match?

[mcasas, 2017/05/08 21:08:04]

Right. What I meant is that I'd rather use
OnTheEventOfSomething() for binding a callback,
whereas for direct invocations I rather use DoThisAndThat(), 
the latter having more information. 

Callback-interfaces are necessarily less informative
in what they prescribe, because they are meant to be
indications for the client to call upon certain events, and
in so doing they are weak APIs because they don't prescribe
any details as to what is being done. Internally to a class,
method names should describe what the method _does_,
not on what circumstances it should be called.

With that in mind, OnCreateDeviceCallback(), could be
OK (better: OnDeviceCreated()), but 
OnDeviceCreationFailed() would not, I'd rather call it ...
NotifyCallbacks()? I don't know, you'd know better.

[chfremer, 2017/05/08 21:56:31]

Hmm, the method OnCreateDeviceCallback() is also internal to the class. If I
follow the guideline you outlined, shouldn't this one also take the form
DoThisAndThat()? I don't see how the fact that a method is used in a Bind()
suddenly makes it more okay to use a less descriptive name.

Thinking about this, I believe the reason I ended up using OnSomething() names
for all three methods is that I was unable to find suitable method names of type
DoThisAndThat() that are more abstract than the actual implementation of the
methods. And that is still the case.

Renaming OnCreateDeviceCallback() to OnDeviceCreated() seems dangerous, because
an invocation can also indicate that device creation has failed.

Renaming OnDeviceCreationFailed() to NotifyCallbacks() seems dangerous, because
this is not all that it does. It also changes the internal state.

Do we have any alternatives?

[miu, 2017/05/09 19:34:19]

The caller. Always the caller. There are thousands of examples of this
throughout Chromium and Blink code. ;-)

[miu, 2017/05/09 19:34:19]

IMHO, OnSomething() is more for interfaces and the implementation of interfaces.
If the method is private and is only bound in this module, the DoThisAndThat()
is definitely more appropriate.

+
   video_capture::mojom::DeviceFactoryPtr* const device_factory_;
+  State state_;
+  base::ThreadChecker thread_checker_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_SERVICE_VIDEO_CAPTURE_DEVICE_LAUNCHER_H_