[Mojo Video Capture] Add unit tests for ServiceVideoCaptureDeviceLauncher

[Mojo Video Capture] Add unit tests for ServiceVideoCaptureDeviceLauncher

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL25_tests.

* Added tests
* Added handling of case where the connection to the service is lost while
  waiting for the service response for a call to CreateDevice().

BUG=584797
TEST=
  content_unittests --gtest_filter="ServiceVideoCaptureDeviceLauncherTest.*"
  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/2886303002
Cr-Commit-Position: refs/heads/master@{#474564}
Committed: https://chromium.googlesource.com/chromium/src/+/9da3c7fa4bd8295789a5900732f9091524fe07c1

[chfremer, 2017-05-19 18:21:34]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 18:22:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chfremer, 2017-05-19 18:25:32]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org, mcasas@chromium.org, miu@chromium.org,
rockot@chromium.org

[chfremer, 2017-05-19 18:25:33]

emircan@: PTAL
rockot@: Please look at LaunchingDeviceFailsBecauseConnectionLostWhileLaunching.
I am not sure if I am handling this case correctly, so would like to get your
input on it.

mcasas@, miu@: FYI

[commit-bot: I haz the power, 2017-05-19 18:27:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 18:27:20]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2017-05-19 18:29:50]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 18:30:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 21:04:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 21:04:25]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-05-23 23:05:40]

lgtm % nits.

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/service_video_capture_device_launcher.cc
(right):

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:97:
base::Unretained(this)));
Can you add a comment explaining why Unretained is safe?

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:153:
Callbacks* callbacks = callbacks_;
DCHECK(callbacks_)

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc
(right):

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc:138:
video_capture::mojom::DeviceAccessResultCode::SUCCESS);
As we discussed offline, if this returns ERROR_DEVICE_NOT_FOUND or
NOT_INITIALIZED, that would trigger a different codepath. Lets add tests that
cover it too.

[chfremer, 2017-05-23 23:39:19]

rockot@: PTAL my question regarding test case
LaunchingDeviceFailsBecauseConnectionLostWhileLaunching

Is it correct to test for this?
Is this the correct way to test for this?

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/service_video_capture_device_launcher.cc
(right):

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:97:
base::Unretained(this)));
On 2017/05/23 23:05:40, emircan wrote:
> Can you add a comment explaining why Unretained is safe?

Done.

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:153:
Callbacks* callbacks = callbacks_;
On 2017/05/23 23:05:40, emircan wrote:
> DCHECK(callbacks_)

Done.

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc
(right):

https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc:138:
video_capture::mojom::DeviceAccessResultCode::SUCCESS);
On 2017/05/23 23:05:40, emircan wrote:
> As we discussed offline, if this returns ERROR_DEVICE_NOT_FOUND or
> NOT_INITIALIZED, that would trigger a different codepath. Lets add tests that
> cover it too.

Done.

[Ken Rockot(use gerrit already), 2017-05-25 01:47:15]

On 2017/05/23 at 23:39:19, chfremer wrote:
> rockot@: PTAL my question regarding test case
LaunchingDeviceFailsBecauseConnectionLostWhileLaunching
> 
> Is it correct to test for this?

Seems reasonable to me, but that's really your call.

> Is this the correct way to test for this?

It certainly appears to test what you intend to test. It's unfortunate that it
has to be so verbose, but don't have any suggestions to improve it.

In this case gmock might be adding more noise than if you had just created
one-off testing impls of these interfaces as needed.

> 
>
https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
> File
content/browser/renderer_host/media/service_video_capture_device_launcher.cc
(right):
> 
>
https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:97:
base::Unretained(this)));
> On 2017/05/23 23:05:40, emircan wrote:
> > Can you add a comment explaining why Unretained is safe?
> 
> Done.
> 
>
https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:153:
Callbacks* callbacks = callbacks_;
> On 2017/05/23 23:05:40, emircan wrote:
> > DCHECK(callbacks_)
> 
> Done.
> 
>
https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
> File
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2886303002/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc:138:
video_capture::mojom::DeviceAccessResultCode::SUCCESS);
> On 2017/05/23 23:05:40, emircan wrote:
> > As we discussed offline, if this returns ERROR_DEVICE_NOT_FOUND or
> > NOT_INITIALIZED, that would trigger a different codepath. Lets add tests
that
> > cover it too.
> 
> Done.

[chfremer, 2017-05-25 01:55:17]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-05-25 01:55:17]

The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org
Link to the patchset: https://codereview.chromium.org/2886303002/#ps40001
(title: "Incorporated suggestions from Patch Set 2")

[chfremer, 2017-05-25 01:55:29]

Thanks for the review!

[commit-bot: I haz the power, 2017-05-25 01:55:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 05:27:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495677317063430,
"parent_rev": "1ab7aa8f394ec33c1fd775adbd34f22907cdae05", "commit_rev":
"9da3c7fa4bd8295789a5900732f9091524fe07c1"}

[commit-bot: I haz the power, 2017-05-25 05:27:28]

Description was changed from

==========
[Mojo Video Capture] Add unit tests for ServiceVideoCaptureDeviceLauncher

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL25_tests.

* Added tests
* Added handling of case where the connection to the service is lost while
  waiting for the service response for a call to CreateDevice().

BUG=584797
TEST=
  content_unittests --gtest_filter="ServiceVideoCaptureDeviceLauncherTest.*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Add unit tests for ServiceVideoCaptureDeviceLauncher

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL25_tests.

* Added tests
* Added handling of case where the connection to the service is lost while
  waiting for the service response for a call to CreateDevice().

BUG=584797
TEST=
  content_unittests --gtest_filter="ServiceVideoCaptureDeviceLauncherTest.*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

Review-Url: https://codereview.chromium.org/2886303002
Cr-Commit-Position: refs/heads/master@{#474564}
Committed:
https://chromium.googlesource.com/chromium/src/+/9da3c7fa4bd8295789a5900732f9...
==========

[commit-bot: I haz the power, 2017-05-25 05:27:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9da3c7fa4bd8295789a5900732f9...

[Ken Rockot(use gerrit already), 2017-05-26 00:45:05]

I realize you landed this already, but I just noticed I forgot to publish my
inline comments. Doing so for posterity and in case you feel they're worth
addressing in a follow-up.

https://codereview.chromium.org/2886303002/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc
(right):

https://codereview.chromium.org/2886303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc:275:
// |create_device_cb| will be dropped.
nit: "can" be dropped. Resetting the binding doesn't drop the callback, it
simply allows the callback to be dropped without asserting.

https://codereview.chromium.org/2886303002/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc:277:
// We have to invoke the callback, because not doing so triggers a DCHECK.
nit: This shouldn't be necessary since you're resetting the factory binding
immediately above.