getUserMedia: handle the device starting status report.

content/browser/renderer_host/media/video_capture_manager_unittest.cc

 // Copyright (c) 2012 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.
 
 // Unit test for VideoCaptureManager.
 
 #include "content/browser/renderer_host/media/video_capture_manager.h"
 
 #include <stdint.h>
 
@@ skipping 345 matching lines @@
   vcm_->UnregisterListener();
 }
 
 TEST_F(VideoCaptureManagerTest, AddObserver) {
   InSequence s;
   MockVideoCaptureObserver observer;
   vcm_->AddVideoCaptureObserver(&observer);
 
   EXPECT_CALL(observer,
               OnVideoCaptureStarted(WrappedDeviceFactory::DEFAULT_FACING));
+  EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _));
+  EXPECT_CALL(*frame_observer_, OnStarted(_));
   EXPECT_CALL(observer,
               OnVideoCaptureStopped(WrappedDeviceFactory::DEFAULT_FACING));
+  EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _));

[chfremer, 2017/03/02 17:56:10]

Why are we adding these expectations?

It appears that the original purpose of the test was to verify the
VideoCaptureObserver calls, and for that, the other calls are uninteresting, and
therefore should not be explicitly expected.

[braveyao, 2017/03/03 17:53:12]

Done.
No particular reason. Just to remove the GMOCK Warning that these mocked mehtods
are called.

[chfremer, 2017/03/03 19:08:33]

Ah, I understand.
I think the GMOCK Warning also recommends to not add EXPECT_CALL just to get rid
of the warning, so we are probably doing the right thing by removing them.

[braveyao, 2017/03/03 21:34:55]

Acknowledged.

 
   int video_session_id = vcm_->Open(devices_.front());
   VideoCaptureControllerID client_id = StartClient(video_session_id, true);
 
   StopClient(client_id);
   vcm_->Close(video_session_id);
 
   // Wait to check callbacks before removing the listener.
   base::RunLoop().RunUntilIdle();
   vcm_->UnregisterListener();
@@ skipping 355 matching lines @@
   EXPECT_CALL(*video_capture_device_factory_, WillResumeDevice()).Times(1);
   ResumeClient(video_session_id, client_id);
 
   // Attempting to resume the client a second time should not cause any calls to
   // VideoCaptureDevice::Resume().
   ResumeClient(video_session_id, client_id);
 
   // Add a second client that is never paused, then pause/resume the first
   // client, and no calls to VideoCaptureDevice::MaybeSuspend() or Resume() are
   // made.
+  EXPECT_CALL(*frame_observer_, OnStarted(_));

[chfremer, 2017/03/02 17:56:10]

Hmm. The purpose of this test is somewhat unclear to me.
The name suggests that the purpose of the test is to verify that the
WillSuspendDevice() and WillResumeDevice() calls arrive as expected.

The callback that may arrive to |listener_->Opened()|,
|frame_observer_|->OnStarted() and |listener_->Closed()| are not relevant for
testing that, and therefore the EXPECT_CALLs for those are diluting the focus
from what actually matters.

Ideally those callbacks should be covered by a separate test (which they already
might be), and then we should remove the EXPECT_CALLs from this one.

[braveyao, 2017/03/03 17:53:12]

This is needed because StartClient() are called twice, due to the modification
in VideoCaptureConthroller. So either we don't do EXPECT_CALL(OnStarted) or
expect it twice. Here I choose to expect it twice.

   const VideoCaptureControllerID client_id2 =
       StartClient(video_session_id, true);
   PauseClient(client_id);
   ResumeClient(video_session_id, client_id);
 
   StopClient(client_id);
   StopClient(client_id2);
 
   EXPECT_CALL(*listener_, Closed(MEDIA_DEVICE_VIDEO_CAPTURE, _));
   vcm_->Close(video_session_id);
@@ skipping 33 matching lines @@
   // Wait to check callbacks before removing the listener.
   base::RunLoop().RunUntilIdle();
   vcm_->UnregisterListener();
 }
 #endif
 
 // TODO(mcasas): Add a test to check consolidation of the supported formats
 // provided by the device when http://crbug.com/323913 is closed.
 
 }  // namespace content