getUserMeida: report device starting states

content/browser/renderer_host/media/video_capture_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.
 
 #include <stdint.h>
 
 #include <map>
 #include <memory>
 #include <string>
 
@@ skipping 20 matching lines @@
 #include "media/audio/mock_audio_manager.h"
 #include "media/base/media_switches.h"
 #include "media/capture/video_capture_types.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "net/url_request/url_request_context.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
 using ::testing::AnyNumber;
+using ::testing::AtMost;
 using ::testing::DoAll;
 using ::testing::InSequence;
 using ::testing::Mock;
 using ::testing::Return;
 using ::testing::SaveArg;
 using ::testing::StrictMock;
 
 namespace content {
 
 namespace {
@@ skipping 186 matching lines @@
   void StartAndImmediateStopCapture() {
     // Quickly start and then stop capture, without giving much chance for
     // asynchronous capture operations to produce frames.
     InSequence s;
     base::RunLoop run_loop;
 
     media::VideoCaptureParams params;
     params.requested_format = media::VideoCaptureFormat(
         gfx::Size(352, 288), 30, media::PIXEL_FORMAT_I420);
 
-    EXPECT_CALL(*this, OnStateChanged(mojom::VideoCaptureState::STARTED));
+    // |STARTED| is reported asynchronously now, which may not be got if capture
+    // is stopped immediately.

[chfremer, 2017/02/16 01:00:50]

Please remove the "now", because the reader of the comment is not typically
interested in the history of the code.

[braveyao, 2017/02/17 20:37:21]

Done.

+    EXPECT_CALL(*this, OnStateChanged(mojom::VideoCaptureState::STARTED))
+        .Times(AtMost(1));

[chfremer, 2017/02/09 23:34:52]

Do we really want to modify the contract between VideoCaptureHost and
VideoCaptureObserver such that it is legal for VideoCaptureHost to report
STOPPED without first reporting STARTED? 
As a user of the interface that would feel unexpected to me. 
If a stop request can cancel a start request before it has completed, could we
make it that in that case neither STARTED nor STOPPED is reported?

I would prefer that we guarantee that we get either a) STARTED followed by
STOPPED or b) no state changes at all.

[braveyao, 2017/02/14 01:05:49]

It's natural to me that a stop request can cancel a start request before it has
completed. And at this point, only the STOPPED report will be concerned, as the
STARTED is not important any more. 
WDTY?

[chfremer, 2017/02/14 19:21:22]

I think it is confusing for the listener when a STOPPED event can arrive without
any preceding STARTED event. If starting is aborted without any visible effect 
for the listener, there is no reason to send any notification to the listener, 
not even a STOPPED. If we do want to notify listeners about the aborted attemt 
to start, we should send a STARTING event followed by a STARTING_ABORTED event.

[braveyao, 2017/02/15 21:47:15]

As we discussed, the current processing is OK. Keep it as is.
Let me know if there is any other concern.

[chfremer, 2017/02/16 01:00:50]

As per our offline discussion, I understand that the listener requires
the STOPPED event to tear down state that it creates even before requesting 
to start the device.

In that case, let us make the contract as clear as possible.
It seems that if STARTED is called, it must be called before STOPPED.
We should express that in this test using gmock's InSequence.

Then, in the file video_capture.mojom, there is a description comment 
outlining the protocol of communication between Observer and VideoCaptureHost.
We should make a note there that it is legal that no STARTED event is 
sent in case a Stop request cancels the preceding Start request.

[braveyao, 2017/02/17 20:37:20]

Done.

[chfremer, 2017/02/17 23:51:14]

I couldn't find this change in PatchSet#6.
Also note that the InSequence constraint should probably go to all the unit
tests that check for the arrival of the STARTED and STOPPED events, not just
this one.
I also couldn't find this change in PatchSet#6.

[braveyao, 2017/02/22 18:06:44]

"InSequence" is in this function already, at line 241. 
And it can't be added into the "StartCapture" above, which will cause bots error
on some platforms.

     host_->Start(kDeviceId, opened_session_id_, params,
                  observer_binding_.CreateInterfacePtrAndBind());
 
     EXPECT_CALL(*this, OnStateChanged(mojom::VideoCaptureState::STOPPED));
     host_->Stop(kDeviceId);
     run_loop.RunUntilIdle();
   }
 
   void PauseResumeCapture() {
     InSequence s;
@@ skipping 100 matching lines @@
   StartCapture();
 
   // When the session is closed via the stream without stopping capture, the
   // ENDED event is sent.
   EXPECT_CALL(*this, OnStateChanged(mojom::VideoCaptureState::ENDED));
   CloseSession();
   base::RunLoop().RunUntilIdle();
 }
 
 }  // namespace content