Remove device enumeration, caching and monitoring from MediaStreamManager.

content/browser/renderer_host/media/video_capture_manager_unittest.cc

Index: content/browser/renderer_host/media/video_capture_manager_unittest.cc
diff --git a/content/browser/renderer_host/media/video_capture_manager_unittest.cc b/content/browser/renderer_host/media/video_capture_manager_unittest.cc
index c2cc9c700711e08b7782fa6940280dc10261d7bc..cff5aef6aff3954cc310ef7899d85db325e68496 100644
--- a/content/browser/renderer_host/media/video_capture_manager_unittest.cc
+++ b/content/browser/renderer_host/media/video_capture_manager_unittest.cc
@@ -41,9 +41,12 @@ class MockMediaStreamProviderListener : public MediaStreamProviderListener {
 
   MOCK_METHOD2(Opened, void(MediaStreamType, int));
   MOCK_METHOD2(Closed, void(MediaStreamType, int));
+  MOCK_METHOD2(Aborted, void(MediaStreamType, int));
+
+  // No longer part of MediaStreamProviderListener, but still useful for
+  // testing.

[hta - Chromium, 2016/09/20 09:41:39]

Does this mean that we mock a method that isn't provided on the interface we're
mocking? This seems weird - should at least give pointers - "this is added as a
mock, and dispatched to in HandleEnumerationResult, because it's easier to write
tests in terms of a mocked DevicesEnumerated function than with other ways to
write the same thing".

[Guido Urdaneta, 2016/09/20 13:59:27]

Removed the method.

   MOCK_METHOD2(DevicesEnumerated, void(MediaStreamType,
                                        const StreamDeviceInfoArray&));
-  MOCK_METHOD2(Aborted, void(MediaStreamType, int));
 };  // class MockMediaStreamProviderListener
 
 // Needed as an input argument to StartCaptureForClient().
@@ -67,6 +70,17 @@ class MockFrameObserver : public VideoCaptureControllerEventHandler {
   void OnGotControllerCallback(VideoCaptureControllerID) {}
 };
 
+static void HandleEnumerationResult(
+    MockMediaStreamProviderListener* listener,
+    const media::VideoCaptureDeviceDescriptors& descriptors) {
+  StreamDeviceInfoArray devices;
+  for (const auto& descriptor : descriptors) {
+    devices.emplace_back(MEDIA_DEVICE_VIDEO_CAPTURE,
+                         descriptor.GetNameAndModel(), descriptor.device_id);
+  }
+  listener->DevicesEnumerated(MEDIA_DEVICE_VIDEO_CAPTURE, devices);
+}
+
 // Test class
 class VideoCaptureManagerTest : public testing::Test {
  public:
@@ -191,7 +205,7 @@ TEST_F(VideoCaptureManagerTest, CreateAndClose) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _));
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _));
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();
@@ -214,7 +228,7 @@ TEST_F(VideoCaptureManagerTest, CreateAndCloseMultipleTimes) {
   EXPECT_CALL(*listener_, DevicesEnumerated(MEDIA_DEVICE_VIDEO_CAPTURE, _))
       .WillOnce(SaveArg<1>(&devices));
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();
@@ -244,7 +258,7 @@ TEST_F(VideoCaptureManagerTest, CreateAndAbort) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _));
   EXPECT_CALL(*listener_, Aborted(MEDIA_DEVICE_VIDEO_CAPTURE, _));
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();
@@ -273,7 +287,7 @@ TEST_F(VideoCaptureManagerTest, OpenTwice) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _)).Times(2);
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _)).Times(2);
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();
@@ -302,7 +316,7 @@ TEST_F(VideoCaptureManagerTest, ConnectAndDisconnectDevices) {
   InSequence s;
   EXPECT_CALL(*listener_, DevicesEnumerated(MEDIA_DEVICE_VIDEO_CAPTURE, _))
       .WillOnce(SaveArg<1>(&devices));
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
   base::RunLoop().RunUntilIdle();
   ASSERT_EQ(devices.size(), 2u);
 
@@ -310,7 +324,7 @@ TEST_F(VideoCaptureManagerTest, ConnectAndDisconnectDevices) {
   video_capture_device_factory_->set_number_of_devices(1);
   EXPECT_CALL(*listener_, DevicesEnumerated(MEDIA_DEVICE_VIDEO_CAPTURE, _))
       .WillOnce(SaveArg<1>(&devices));
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
   base::RunLoop().RunUntilIdle();
   ASSERT_EQ(devices.size(), 1u);
 
@@ -318,7 +332,7 @@ TEST_F(VideoCaptureManagerTest, ConnectAndDisconnectDevices) {
   video_capture_device_factory_->set_number_of_devices(3);
   EXPECT_CALL(*listener_, DevicesEnumerated(MEDIA_DEVICE_VIDEO_CAPTURE, _))
       .WillOnce(SaveArg<1>(&devices));
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
   base::RunLoop().RunUntilIdle();
   ASSERT_EQ(devices.size(), 3u);
 
@@ -342,7 +356,7 @@ TEST_F(VideoCaptureManagerTest, ManipulateDeviceAndCheckCapabilities) {
   InSequence s;
   EXPECT_CALL(*listener_, DevicesEnumerated(MEDIA_DEVICE_VIDEO_CAPTURE, _))
       .WillOnce(SaveArg<1>(&devices));
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
   base::RunLoop().RunUntilIdle();
   ASSERT_GE(devices.size(), 2u);
 
@@ -405,7 +419,7 @@ TEST_F(VideoCaptureManagerTest, StartDeviceAndGetDeviceFormatInUse) {
   InSequence s;
   EXPECT_CALL(*listener_, DevicesEnumerated(MEDIA_DEVICE_VIDEO_CAPTURE, _))
       .WillOnce(SaveArg<1>(&devices));
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
   base::RunLoop().RunUntilIdle();
   ASSERT_GE(devices.size(), 2u);
 
@@ -454,7 +468,7 @@ TEST_F(VideoCaptureManagerTest, OpenTwo) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _)).Times(2);
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _)).Times(2);
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();
@@ -484,7 +498,7 @@ TEST_F(VideoCaptureManagerTest, OpenNotExisting) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _));
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _));
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();
@@ -528,7 +542,7 @@ TEST_F(VideoCaptureManagerTest, CloseWithoutStop) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _));
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _));
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   run_loop.Run();
@@ -557,7 +571,7 @@ TEST_F(VideoCaptureManagerTest, PauseAndResumeClient) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _));
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _));
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));

[hta - Chromium, 2016/09/20 09:41:39]

With the number of calls to this particular function, should it be an utility in
the test fixture? This could allow you to hide HandleEnmuerationResult too - it
seems from a quick read that all that DevicesEnumerated does is to save its
argument in &devices, which means that if you put |devices| into the fixture
too, you can avoid the strange mock completely.

[Guido Urdaneta, 2016/09/20 13:59:27]

Done. The only slightly ugly thing is that HandleEnumerationResult is public so
that it can be used by the test that simulates device removals and additions.

 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();
@@ -589,7 +603,7 @@ TEST_F(VideoCaptureManagerTest, PauseAndResumeDevice) {
   EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _));
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _));
 
-  vcm_->EnumerateDevices(MEDIA_DEVICE_VIDEO_CAPTURE);
+  vcm_->EnumerateDevices(base::Bind(&HandleEnumerationResult, listener_.get()));
 
   // Wait to get device callback.
   base::RunLoop().RunUntilIdle();