getUserMedia: handle the device starting status report.

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 aefd642ddf038f27afbb99691167b05dcd793d41..3e1f5c2a75d92f17ce288bb14239a6311f0fb612 100644
--- a/content/browser/renderer_host/media/video_capture_manager_unittest.cc
+++ b/content/browser/renderer_host/media/video_capture_manager_unittest.cc
@@ -363,8 +363,11 @@ TEST_F(VideoCaptureManagerTest, AddObserver) {
 
   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);
@@ -740,6 +743,7 @@ TEST_F(VideoCaptureManagerTest, PauseAndResumeClient) {
   // 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);