Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(160)

Issue 2886303002: [Mojo Video Capture] Add unit tests for ServiceVideoCaptureDeviceLauncher (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by chfremer
Modified:
3 months, 4 weeks ago
CC:
chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, posciak+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Patch Set 2 : Rebase to May 19th #

Total comments: 6

Patch Set 3 : Incorporated suggestions from Patch Set 2 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+326 lines, -12 lines) Patch
M content/browser/renderer_host/media/service_video_capture_device_launcher.h View 2 chunks +6 lines, -3 lines 0 comments Download
M content/browser/renderer_host/media/service_video_capture_device_launcher.cc View 1 2 3 chunks +35 lines, -9 lines 0 comments Download
A content/browser/renderer_host/media/service_video_capture_device_launcher_unittest.cc View 1 2 1 chunk +283 lines, -0 lines 2 comments Download
M content/test/BUILD.gn View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 21 (13 generated)
chfremer
emircan@: PTAL rockot@: Please look at LaunchingDeviceFailsBecauseConnectionLostWhileLaunching. I am not sure if I am handling ...
4 months ago (2017-05-19 18:25:33 UTC) #4
emircan
lgtm % nits. https://codereview.chromium.org/2886303002/diff/20001/content/browser/renderer_host/media/service_video_capture_device_launcher.cc File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2886303002/diff/20001/content/browser/renderer_host/media/service_video_capture_device_launcher.cc#newcode97 content/browser/renderer_host/media/service_video_capture_device_launcher.cc:97: base::Unretained(this))); Can you add a comment ...
4 months ago (2017-05-23 23:05:40 UTC) #11
chfremer
rockot@: PTAL my question regarding test case LaunchingDeviceFailsBecauseConnectionLostWhileLaunching Is it correct to test for this? ...
4 months ago (2017-05-23 23:39:19 UTC) #12
Ken Rockot(use gerrit already)
On 2017/05/23 at 23:39:19, chfremer wrote: > rockot@: PTAL my question regarding test case LaunchingDeviceFailsBecauseConnectionLostWhileLaunching ...
3 months, 4 weeks ago (2017-05-25 01:47:15 UTC) #13
chfremer
Thanks for the review!
3 months, 4 weeks ago (2017-05-25 01:55:29 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2886303002/40001
3 months, 4 weeks ago (2017-05-25 01:55:58 UTC) #17
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9da3c7fa4bd8295789a5900732f9091524fe07c1
3 months, 4 weeks ago (2017-05-25 05:27:29 UTC) #20
Ken Rockot(use gerrit already)
3 months, 4 weeks ago (2017-05-26 00:45:05 UTC) #21
Message was sent while issue was closed.
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b