Video Capture Mojo (1.4c): Handle Subsequent Access

services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc

Index: services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc
diff --git a/services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc b/services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1c3e1b38367997c5ae2f82213db8f6685c640189
--- /dev/null
+++ b/services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc
@@ -0,0 +1,98 @@
+// Copyright 2016 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 "base/run_loop.h"
+#include "services/video_capture/fake_device_descriptor_video_capture_service_test.h"
+#include "services/video_capture/mock_video_capture_device_client.h"
+
+using testing::_;
+using testing::InvokeWithoutArgs;
+
+namespace video_capture {
+
+// Tests that when requesting a second proxy for a device without closing the
+// first one, the service revokes access to the first one by closing the
+// connection.
+TEST_F(FakeDeviceDescriptorVideoCaptureServiceTest,
+       AccessIsRevokedOnSecondAccess) {
+  mojom::DeviceAccessResultCode result_code_1 =
+      mojom::DeviceAccessResultCode::NOT_INITIALIZED;
+  mojom::DeviceAccessResultCode result_code_2 =
+      mojom::DeviceAccessResultCode::NOT_INITIALIZED;
+
+  mojom::VideoCaptureDeviceProxyPtr device_proxy_1;
+  bool device_access_1_revoked = false;
+  factory_->CreateDeviceProxy(
+      fake_device_descriptor_->Clone(), mojo::GetProxy(&device_proxy_1),
+      base::Bind(
+          [](mojom::DeviceAccessResultCode* target,
+             mojom::DeviceAccessResultCode result_code) {
+            *target = result_code;
+          },
+          &result_code_1));
+  device_proxy_1.set_connection_error_handler(
+      base::Bind([](bool* access_revoked) { *access_revoked = true; },
+                 &device_access_1_revoked));
+
+  base::RunLoop wait_loop;
+  mojom::VideoCaptureDeviceProxyPtr device_proxy_2;

[mcasas, 2016/09/02 16:15:14]

Insert in between l.38-39
ASSERT_FALSE(device_access_1_revoked);

Also, shouldn't 
ASSERT_EQ(mojom::DeviceAccessResultCode::SUCCESS, result_code_1);
be here? 

Oh I see, you run the RunLoop only once at the end,
but in that case, aren't you potentially missing
the intermediate state where |device_proxy_1| is
correctly bound and |device_proxy_2| is not existing
yet? 

Consider running the loop twice, or adding a simple
test case where a Proxy is just created successfully.

[chfremer, 2016/09/02 17:44:31]

Good observation. I am considering adding a new test case for checking that a
Proxy is just created successfully.
But the same already happens and is verified as part of the
FakeDeviceTest::SetUp() method.
Is it worth adding an explicit test for it?

+  bool device_access_2_revoked = false;
+  factory_->CreateDeviceProxy(
+      fake_device_descriptor_->Clone(), mojo::GetProxy(&device_proxy_2),
+      base::Bind(
+          [](base::RunLoop* wait_loop, mojom::DeviceAccessResultCode* target,
+             mojom::DeviceAccessResultCode result_code) {
+            *target = result_code;
+            wait_loop->Quit();
+          },
+          &wait_loop, &result_code_2));
+  device_proxy_2.set_connection_error_handler(
+      base::Bind([](bool* access_revoked) { *access_revoked = true; },
+                 &device_access_2_revoked));
+  wait_loop.Run();
+  ASSERT_EQ(mojom::DeviceAccessResultCode::SUCCESS, result_code_1);
+  ASSERT_EQ(mojom::DeviceAccessResultCode::SUCCESS, result_code_2);
+  ASSERT_TRUE(device_access_1_revoked);
+  ASSERT_FALSE(device_access_2_revoked);
+}
+
+// Tests that a second proxy requested for a device can be used successfully.
+TEST_F(FakeDeviceDescriptorVideoCaptureServiceTest,
+       CanUseSecondRequestedProxy) {
+  mojom::VideoCaptureDeviceProxyPtr device_proxy_1;
+  factory_->CreateDeviceProxy(
+      fake_device_descriptor_->Clone(), mojo::GetProxy(&device_proxy_1),
+      base::Bind([](mojom::DeviceAccessResultCode result_code) {}));
+
+  base::RunLoop wait_loop;
+  mojom::VideoCaptureDeviceProxyPtr device_proxy_2;
+  factory_->CreateDeviceProxy(
+      fake_device_descriptor_->Clone(), mojo::GetProxy(&device_proxy_2),
+      base::Bind(
+          [](base::RunLoop* wait_loop,
+             mojom::DeviceAccessResultCode result_code) { wait_loop->Quit(); },
+          &wait_loop));

[mcasas, 2016/09/02 16:15:15]

I like the local lambda methods, but MOCKs would
also allow you to specify how many times the method
is expected to be called, and will kick in if
expectations are not met or unexpected method calls
are there.  So, why not MOCKing :-)  ?

[chfremer, 2016/09/02 17:44:32]

Good point. I didn't think of it, because I traditionally only think of using
mocks to implement interface classes. But as you are suggesting, callback
methods are actually also interfaces. I guess the only reason not to do it would
be the extra amount of code it would need. But it is not that much.

I'll give it a try.

+  wait_loop.Run();
+
+  auto arbitrary_requested_format = mojom::VideoCaptureFormat::New();
+  arbitrary_requested_format->frame_size.SetSize(640, 480);
+  arbitrary_requested_format->frame_rate = 15;
+  arbitrary_requested_format->pixel_format = media::mojom::VideoFormat::I420;
+  arbitrary_requested_format->pixel_storage = mojom::VideoPixelStorage::CPU;
+
+  base::RunLoop wait_loop_2;
+  mojom::VideoCaptureDeviceClientPtr client_proxy;
+  MockVideoCaptureDeviceClient client(mojo::GetProxy(&client_proxy));
+  EXPECT_CALL(client, OnFrameAvailablePtr(_))
+      .WillRepeatedly(
+          InvokeWithoutArgs([&wait_loop_2]() { wait_loop_2.Quit(); }));
+
+  device_proxy_2->Start(std::move(arbitrary_requested_format),
+                        mojom::ResolutionChangePolicy::FIXED_RESOLUTION,
+                        mojom::PowerLineFrequency::DEFAULT,
+                        std::move(client_proxy));
+  wait_loop_2.Run();
+}
+
+}  // namespace video_capture