|
|
Description[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 2)
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL24_part2.
Goal of CL24:
The interface content::VideoCaptureProvider currently has one implementation
called InProcessVideoCaptureProvider, which is essentially a factory for the
legacy in-process video capture stack. This CL adds a second implementation
called MojoServiceVideoCaptureProvider which is essentially a wrapper for
connecting to and communicating with the new video capture service.
Changes in part2:
* Add implementation for class ServiceVideoCaptureDeviceLauncher
* Add basic implementation for class ServiceLaunchedVideoCaptureDevice
BUG=584797
TEST=
service_unittests --gtest_filter="*Video*"
content_unittests --gtest_filter="*Video*"
content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
Review-Url: https://codereview.chromium.org/2857303002
Cr-Commit-Position: refs/heads/master@{#470645}
Committed: https://chromium.googlesource.com/chromium/src/+/46083e3783e7a71d7eef4aeb5c5f6b3cc4c0338d
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rebase to May 5th #
Total comments: 9
Patch Set 3 : Incorporated suggestions from PatchSets 1 and 2 #
Total comments: 1
Patch Set 4 : Add const #Patch Set 5 : Rebase to May 9th #Patch Set 6 : Use base::SequenceChecker #Patch Set 7 : Added back update to |state_| which was accidentally dropped in PatchSet 3 #Dependent Patchsets: Messages
Total messages: 39 (20 generated)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chfremer@chromium.org changed reviewers: + emircan@chromium.org, mcasas@chromium.org, miu@chromium.org, rockot@chromium.org
mcasas@: PTAL non-Mojo parts rockot@: PTAL Mojo parts emircan@, miu@: FYI
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ping
Some minor comments https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; If we move this update of |state_| (resp. the one in l.123 to l.76 (resp., 82), OnDeviceCreatedSuccessfully() (resp OnDeviceCreationFailed()) do not operate on any member variables, and can be made file-static. https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); Actually since OnDeviceCreatedSuccessfully() and OnDeviceCreationFailed() are not bound as callbacks, they can drop the "On" name prefix.
PTAL https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; On 2017/05/05 22:41:02, mcasas wrote: > If we move this update of |state_| (resp. the one in > l.123 to l.76 (resp., 82), OnDeviceCreatedSuccessfully() > (resp OnDeviceCreationFailed()) do not operate on > any member variables, and can be made file-static. What is the advantage of having these two methods be file-static? I see several downsides: 1. These methods move to the top of the file, making it harder to read the file from top to bottom. 2. Since the methods take different paths depending on the |State| we are coming from, we would have to pass that state in, and to do that we would have to make the type |State| public. 3. Since OnDeviceCreationFailed() is called twice, moving the logic for copying the state before updating it out of the method would require duplicating it. https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/05 22:41:02, mcasas wrote: > Actually since OnDeviceCreatedSuccessfully() > and OnDeviceCreationFailed() are not bound as > callbacks, they can drop the "On" name prefix. 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.
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 17:01:54, chfremer wrote: > On 2017/05/05 22:41:02, mcasas wrote: > > Actually since OnDeviceCreatedSuccessfully() > > and OnDeviceCreationFailed() are not bound as > > callbacks, they can drop the "On" name prefix. > > 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. 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. https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/service_launched_video_capture_device.h (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_launched_video_capture_device.h:43: base::ThreadChecker thread_checker_; Is this necessary? Seems like this class is single-threaded, and recent discussions on chromium-dev@ suggested not using ThreadChecker on classes that aren't meant to be multithreaded. https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:94: State state_copy = state_; naming: How about |prior_state|? Actually, you don't even need this. A slight refactoring of your code: ... { DCHECK(thread_checker_.CalledOnValidThread()); if (state_ == State::DEVICE_START_ABORTING) { device.reset(); callbacks->OnDeviceLaunchAborted(); base::ResetAndReturn(&done_cb).Run(); return; } state_ = State::READY_TO_LAUNCH; ... This assumes it's okay to leave |state_| set to DEVICE_START_ABORTING. If not, maybe add a separated DEVICE_START_ABORTED state, since READY_TO_LAUNCH doesn't seem accurate? https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:122: State state_copy = state_; ditto: I think you could simplify by not mutating |state_|, similar to my last comment.
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 20:21:11, miu wrote: > On 2017/05/08 17:01:54, chfremer wrote: > > On 2017/05/05 22:41:02, mcasas wrote: > > > Actually since OnDeviceCreatedSuccessfully() > > > and OnDeviceCreationFailed() are not bound as > > > callbacks, they can drop the "On" name prefix. > > > > 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. > > 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. > 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? https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/service_launched_video_capture_device.h (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_launched_video_capture_device.h:43: base::ThreadChecker thread_checker_; On 2017/05/08 20:21:11, miu wrote: > Is this necessary? Seems like this class is single-threaded, and recent > discussions on chromium-dev@ suggested not using ThreadChecker on classes that > aren't meant to be multithreaded. I was not aware of that discussion and wasn't able to find it with a quick search in chromium-dev. Could you please provide a link? https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:94: State state_copy = state_; On 2017/05/08 20:21:11, miu wrote: > naming: How about |prior_state|? Agreed that this is probably better than |state_copy|, even though the "prior" might be confusing as to exactly when we are referring to. How do you feel about |coming_from_state|? > Actually, you don't even need this. A slight refactoring of your code: > > ... { > DCHECK(thread_checker_.CalledOnValidThread()); > > if (state_ == State::DEVICE_START_ABORTING) { > device.reset(); > callbacks->OnDeviceLaunchAborted(); > base::ResetAndReturn(&done_cb).Run(); > return; > } > > state_ = State::READY_TO_LAUNCH; > > ... > > This assumes it's okay to leave |state_| set to DEVICE_START_ABORTING. If not, > maybe add a separated DEVICE_START_ABORTED state, since READY_TO_LAUNCH doesn't > seem accurate? The idea here was to make it legal for callbacks such as |callbacks->OnDeviceLaunched(...)| to synchronously invoke |ServiceVideoCaptureDeviceLauncher::LaunchDeviceAsync(...)| again in order to retry or start a next device. And the device launch queue in VideoCaptureManager [1], does exactly that. The somewhat ugly local copy of |state_| is only needed because of that. In that design, a state DEVICE_START_ABORTED would have the exact same meaning as READY_TO_LAUNCH, so I don't think it would add value to introduce it as a separate state. [1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide... https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:122: State state_copy = state_; On 2017/05/08 20:21:11, miu wrote: > ditto: I think you could simplify by not mutating |state_|, similar to my last > comment. see other reply
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; On 2017/05/08 17:01:54, chfremer wrote: > On 2017/05/05 22:41:02, mcasas wrote: > > If we move this update of |state_| (resp. the one in > > l.123 to l.76 (resp., 82), OnDeviceCreatedSuccessfully() > > (resp OnDeviceCreationFailed()) do not operate on > > any member variables, and can be made file-static. > > What is the advantage of having these two methods be file-static? > > I see several downsides: > 1. These methods move to the top of the file, making it harder to read the file > from top to bottom. > 2. Since the methods take different paths depending on the |State| we are coming > from, we would have to pass that state in, and to do that we would have to make > the type |State| public. > 3. Since OnDeviceCreationFailed() is called twice, moving the logic for copying > the state before updating it out of the method would require duplicating it. Reduces the exposed surface of the class (the one people read), and the vtable. Also: "minimize the code in headers" : https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 20:51:46, chfremer wrote: > On 2017/05/08 20:21:11, miu wrote: > > On 2017/05/08 17:01:54, chfremer wrote: > > > On 2017/05/05 22:41:02, mcasas wrote: > > > > Actually since OnDeviceCreatedSuccessfully() > > > > and OnDeviceCreationFailed() are not bound as > > > > callbacks, they can drop the "On" name prefix. > > > > > > 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. 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. > > > > 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. > > > > 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?
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; On 2017/05/08 21:08:04, mcasas wrote: > On 2017/05/08 17:01:54, chfremer wrote: > > On 2017/05/05 22:41:02, mcasas wrote: > > > If we move this update of |state_| (resp. the one in > > > l.123 to l.76 (resp., 82), OnDeviceCreatedSuccessfully() > > > (resp OnDeviceCreationFailed()) do not operate on > > > any member variables, and can be made file-static. > > > > What is the advantage of having these two methods be file-static? > > > > I see several downsides: > > 1. These methods move to the top of the file, making it harder to read the > file > > from top to bottom. > > 2. Since the methods take different paths depending on the |State| we are > coming > > from, we would have to pass that state in, and to do that we would have to > make > > the type |State| public. > > 3. Since OnDeviceCreationFailed() is called twice, moving the logic for > copying > > the state before updating it out of the method would require duplicating it. > > Reduces the exposed surface of the class (the one people > read), and the vtable. Also: > "minimize the code in headers" : > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... Hmm, I now understand that the goal is to free up cognitive load from a reader. As per the downsides I mentioned, I am not quite sure that making these non-static private methods static is worth the benefits, though. How strongly do you feel about this? If you still prefer it, I'll follow your suggestion and refactor things to move the methods into cc-file-scope. I don't think they appear in the vtable, since they are not virtual. And a reader not interested in the implementation details, should not care much about the private section of a class declaration. https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 21:08:04, mcasas wrote: > On 2017/05/08 20:51:46, chfremer wrote: > > On 2017/05/08 20:21:11, miu wrote: > > > On 2017/05/08 17:01:54, chfremer wrote: > > > > On 2017/05/05 22:41:02, mcasas wrote: > > > > > Actually since OnDeviceCreatedSuccessfully() > > > > > and OnDeviceCreationFailed() are not bound as > > > > > callbacks, they can drop the "On" name prefix. > > > > > > > > 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. > > 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. > > > > > > > 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. > > > > > > > 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? > 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?
On 2017/05/08 21:56:31, chfremer wrote: > https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... > File > content/browser/renderer_host/media/service_video_capture_device_launcher.cc > (right): > > https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: > state_ = State::READY_TO_LAUNCH; > On 2017/05/08 21:08:04, mcasas wrote: > > On 2017/05/08 17:01:54, chfremer wrote: > > > On 2017/05/05 22:41:02, mcasas wrote: > > > > If we move this update of |state_| (resp. the one in > > > > l.123 to l.76 (resp., 82), OnDeviceCreatedSuccessfully() > > > > (resp OnDeviceCreationFailed()) do not operate on > > > > any member variables, and can be made file-static. > > > > > > What is the advantage of having these two methods be file-static? > > > > > > I see several downsides: > > > 1. These methods move to the top of the file, making it harder to read the > > file > > > from top to bottom. > > > 2. Since the methods take different paths depending on the |State| we are > > coming > > > from, we would have to pass that state in, and to do that we would have to > > make > > > the type |State| public. > > > 3. Since OnDeviceCreationFailed() is called twice, moving the logic for > > copying > > > the state before updating it out of the method would require duplicating it. > > > > Reduces the exposed surface of the class (the one people > > read), and the vtable. Also: > > "minimize the code in headers" : > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... > > Hmm, I now understand that the goal is to free up cognitive load from a reader. > As per the downsides I mentioned, I am not quite sure that making these > non-static private methods static is worth the benefits, though. > > How strongly do you feel about this? If you still prefer it, I'll follow your > suggestion and refactor things to move the methods into cc-file-scope. > > I don't think they appear in the vtable, since they are not virtual. > And a reader not interested in the implementation details, should not care much > about the private section of a class declaration. > That maybe is the theory, but personally I like to read the members of a class in the private section because they are usually more informative than the methods and/or comments -- actually I believe it's on purpose that Java and C++ don't hide the (private) member variables like e.g. Objective-C does. > https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/media/service_video_capture_device_launcher.h > (right): > > https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: > void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); > On 2017/05/08 21:08:04, mcasas wrote: > > On 2017/05/08 20:51:46, chfremer wrote: > > > On 2017/05/08 20:21:11, miu wrote: > > > > On 2017/05/08 17:01:54, chfremer wrote: > > > > > On 2017/05/05 22:41:02, mcasas wrote: > > > > > > Actually since OnDeviceCreatedSuccessfully() > > > > > > and OnDeviceCreationFailed() are not bound as > > > > > > callbacks, they can drop the "On" name prefix. > > > > > > > > > > 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. > > > > 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. > > > > > > > > > > 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. > > > > > > > > > > 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? > > > > 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. When I said Callback-interfaces I meant abstract classes that are composed of pure virtual methods, that form a weak contract, because clients use them to notify the called-back class about events, e.g. what we (partially) do in VideoCaptureDevice::Client https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device... Also, if a method is hard to name wouldn't that be a code smell (same as a class being hard to 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?
PTAL Incorporated suggestions from PS1 and PS2 https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; On 2017/05/08 21:56:31, chfremer wrote: > On 2017/05/08 21:08:04, mcasas wrote: > > On 2017/05/08 17:01:54, chfremer wrote: > > > On 2017/05/05 22:41:02, mcasas wrote: > > > > If we move this update of |state_| (resp. the one in > > > > l.123 to l.76 (resp., 82), OnDeviceCreatedSuccessfully() > > > > (resp OnDeviceCreationFailed()) do not operate on > > > > any member variables, and can be made file-static. > > > > > > What is the advantage of having these two methods be file-static? > > > > > > I see several downsides: > > > 1. These methods move to the top of the file, making it harder to read the > > file > > > from top to bottom. > > > 2. Since the methods take different paths depending on the |State| we are > > coming > > > from, we would have to pass that state in, and to do that we would have to > > make > > > the type |State| public. > > > 3. Since OnDeviceCreationFailed() is called twice, moving the logic for > > copying > > > the state before updating it out of the method would require duplicating it. > > > > Reduces the exposed surface of the class (the one people > > read), and the vtable. Also: > > "minimize the code in headers" : > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... > > Hmm, I now understand that the goal is to free up cognitive load from a reader. > As per the downsides I mentioned, I am not quite sure that making these > non-static private methods static is worth the benefits, though. > > How strongly do you feel about this? If you still prefer it, I'll follow your > suggestion and refactor things to move the methods into cc-file-scope. > > I don't think they appear in the vtable, since they are not virtual. > And a reader not interested in the implementation details, should not care much > about the private section of a class declaration. Moved the two private methods to cc-file-scope and renamed them to ConcludeLaunchDeviceWithSuccess() and ConcludeLaunchDeviceWithFailure(). https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/service_launched_video_capture_device.h (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_launched_video_capture_device.h:43: base::ThreadChecker thread_checker_; On 2017/05/08 20:51:46, chfremer wrote: > On 2017/05/08 20:21:11, miu wrote: > > Is this necessary? Seems like this class is single-threaded, and recent > > discussions on chromium-dev@ suggested not using ThreadChecker on classes that > > aren't meant to be multithreaded. > > I was not aware of that discussion and wasn't able to find it with a quick > search in chromium-dev. Could you please provide a link? > Removed ThreadChecker here and in ServiceVideoCaptureDeviceLauncher.
lgtm, thanks! https://codereview.chromium.org/2857303002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/service_video_capture_device_launcher.cc:113: bool abort_requested = (state_ == State::DEVICE_START_ABORTING); micro-nit: const
Thanks for the review!
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2857303002/#ps60001 (title: "Add const")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
PS4 lgtm. https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 21:56:31, chfremer wrote: > > > So for the example at hand, would the OnSomething() case be a match? 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. https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 20:51:46, chfremer wrote: > 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). The caller. Always the caller. There are thousands of examples of this throughout Chromium and Blink code. ;-) https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/service_launched_video_capture_device.h (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_launched_video_capture_device.h:43: base::ThreadChecker thread_checker_; On 2017/05/08 20:51:46, chfremer wrote: > I was not aware of that discussion and wasn't able to find it with a quick > search in chromium-dev. Could you please provide a link? > Here: https://groups.google.com/a/chromium.org/forum/#!search/PSA$3A$20base$3A$3ANo...
https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/service_launched_video_capture_device.h (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/service_launched_video_capture_device.h:43: base::ThreadChecker thread_checker_; On 2017/05/09 19:34:19, miu wrote: > On 2017/05/08 20:51:46, chfremer wrote: > > I was not aware of that discussion and wasn't able to find it with a quick > > search in chromium-dev. Could you please provide a link? > > > > Here: > https://groups.google.com/a/chromium.org/forum/#!search/PSA$3A$20base$3A$3ANo... After reading the e-mail thread, my conclusion is that I misinterpreted your initial comment. The recommendation here is not to simply remove any checking. Instead, what we want is to use a base::SequenceChecker. I will add those back in with a new PatchSet. Please let me know if you disagree.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Added base::SequenceChecker with PatchSet 6
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2857303002/#ps120001 (title: "Added back update to |state_| which was accidentally dropped in PatchSet 3")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494439198473120, "parent_rev": "5163e3b4833463b8e30df21b91a527f7c7297baa", "commit_rev": "46083e3783e7a71d7eef4aeb5c5f6b3cc4c0338d"}
Message was sent while issue was closed.
Description was changed from ========== [Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 2) This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL24_part2. Goal of CL24: The interface content::VideoCaptureProvider currently has one implementation called InProcessVideoCaptureProvider, which is essentially a factory for the legacy in-process video capture stack. This CL adds a second implementation called MojoServiceVideoCaptureProvider which is essentially a wrapper for connecting to and communicating with the new video capture service. Changes in part2: * Add implementation for class ServiceVideoCaptureDeviceLauncher * Add basic implementation for class ServiceLaunchedVideoCaptureDevice BUG=584797 TEST= service_unittests --gtest_filter="*Video*" content_unittests --gtest_filter="*Video*" content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== [Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 2) This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL24_part2. Goal of CL24: The interface content::VideoCaptureProvider currently has one implementation called InProcessVideoCaptureProvider, which is essentially a factory for the legacy in-process video capture stack. This CL adds a second implementation called MojoServiceVideoCaptureProvider which is essentially a wrapper for connecting to and communicating with the new video capture service. Changes in part2: * Add implementation for class ServiceVideoCaptureDeviceLauncher * Add basic implementation for class ServiceLaunchedVideoCaptureDevice BUG=584797 TEST= service_unittests --gtest_filter="*Video*" content_unittests --gtest_filter="*Video*" content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Review-Url: https://codereview.chromium.org/2857303002 Cr-Commit-Position: refs/heads/master@{#470645} Committed: https://chromium.googlesource.com/chromium/src/+/46083e3783e7a71d7eef4aeb5c5f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/46083e3783e7a71d7eef4aeb5c5f... |