getUserMeida: report device starting states

content/browser/renderer_host/media/video_capture_unittest.cc

Index: content/browser/renderer_host/media/video_capture_unittest.cc
diff --git a/content/browser/renderer_host/media/video_capture_unittest.cc b/content/browser/renderer_host/media/video_capture_unittest.cc
index b2ce87ec007dd49609bef2d9367daaf932f70ab9..b792e9cee8ce497bff93a17ca97ceea0d6203c46 100644
--- a/content/browser/renderer_host/media/video_capture_unittest.cc
+++ b/content/browser/renderer_host/media/video_capture_unittest.cc
@@ -38,6 +38,7 @@
 
 using ::testing::_;
 using ::testing::AnyNumber;
+using ::testing::AtMost;
 using ::testing::DoAll;
 using ::testing::InSequence;
 using ::testing::Mock;
@@ -244,7 +245,10 @@ class VideoCaptureTest : public testing::Test,
     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());