bluetooth: Refactor LayoutTestBluetoothAdapterProvider.

content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc

 // Copyright 2015 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 "content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h"
 
 #include "device/bluetooth/bluetooth_adapter.h"
 #include "device/bluetooth/bluetooth_device.h"
 #include "device/bluetooth/bluetooth_discovery_session.h"
 #include "device/bluetooth/bluetooth_uuid.h"
@@ skipping 35 matching lines @@
   NOTREACHED();
   return NULL;
 }
 
 // static
 scoped_refptr<NiceMock<MockBluetoothAdapter>>
 LayoutTestBluetoothAdapterProvider::GetEmptyAdapter() {
   scoped_refptr<NiceMock<MockBluetoothAdapter>> adapter(
       new NiceMock<MockBluetoothAdapter>());
 
+  scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session(
+      GetDiscoverySession());
+
   ON_CALL(*adapter, StartDiscoverySession(_, _))
-      .WillByDefault(Invoke(
-          &LayoutTestBluetoothAdapterProvider::SuccessfulDiscoverySession));
+      .WillByDefault(RunCallbackScoped<0 /* success_callback */,
+                                       BluetoothDiscoverySession>(
+          discovery_session.release()));

[Jeffrey Yasskin, 2015/05/19 01:09:01]

This will crash if StartDiscoverySession is called more than once. You may have
to write the action explicitly, to call GetDiscoverySession() each time.

[ortuno, 2015/05/19 01:51:53]

Ah but then you can't construct the session beforehand. What about making it
explicit that it can only be called once? If someone is expecting for it to be
called more than once then it can define more DiscoverySessions that can be
returned:

scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session1(
      GetDiscoverySession());
scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session2(
      GetOtherDiscoverySession());

EXPECT_CALL(*adapter, StartDiscoverySEssion(_, _))
  .WillOnce(RunCallbackScoped<...> discovery_session1)
  .WillOnce(RunCallbackScoped<...> discovery_session2)

[Jeffrey Yasskin, 2015/05/19 17:13:59]

Hm, why do you need to construct the session before StartDiscoverySession is
called? I could see wanting to configure it, but there's not actually that much
configuration to do, and I wonder if any complicated configuration would be
clearer if it happened in the test itself instead of these helper functions.
You definitely should use EXPECT_CALL(*adapter, StartDiscoverySession(_,
_)).WillOnce(...) in GetEmptyAdapter() if you keep the loose pointer around,
since 0 calls will result in a leak, and >1 call will result in a double-free.

What you have would be safer if gmock supported scoped_ptr arguments (you could
skip the .release()), but that would take C++11 support everywhere, which we
don't have.

[ortuno, 2015/05/19 19:51:18]

For BluetoothDiscoverySession we don't. But we will for BluetoothGattConnection.
Also I think it's clearer if configuration happens in one place as opposed to
hidden inside another function.
Do you mean in requestDevice.html? As tests become more complex, we would have
to expose a lot of
functions to do that. I think it's easier if instead we document how each
adapter behaves.
Done. I think keeping the loose pointer with more explicit syntax is a better
alternative than making a new function for every configuration of
BluetoothDiscoverySession, BluetoothGattConnection, etc.
Ah yes. That would be great.

[Jeffrey Yasskin, 2015/05/19 20:47:49]

Mhmm. Note that you can use lambdas to embed other functions in your main
adapter getter. I wonder if it would make sense to write something like:

ON_CALL(*adapter, StartDiscoverySession(_, _))
    .WillByDefault(RunCallbackWithResult<0 /* success_callback */>,
        []() {
  scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session(
    GetDiscoverySession());
  EXPECT_CALL(*discovery_session, whatever);
  return discovery_session.Pass();
});
Oh yeah, GetEmptyAdapter() is actually the top level of the test, so it's the
place to do all of the configuration. That makes me less worried about replacing
ON_CALL with EXPECT_CALL, since nothing else is going to need to use the
returned adapter.

[ortuno, 2015/05/19 23:03:34]

Completely forgot about lambdas. Done. Please let me know if I'm not using them
correctly.
Actually all adapters are constructed using GetEmptyAdapter(). But now that
we're using lambdas we don't need to change ON_CALL :)

 
   ON_CALL(*adapter, GetDevices())
-      .WillByDefault(Return(adapter->GetMockDevices()));
+      .WillByDefault(
+          Invoke(adapter.get(), &MockBluetoothAdapter::GetMockDevices));
 
   return adapter.Pass();
 }
 
 // static
 scoped_refptr<NiceMock<MockBluetoothAdapter>>
 LayoutTestBluetoothAdapterProvider::GetSingleEmptyDeviceAdapter() {
-  scoped_refptr<NiceMock<MockBluetoothAdapter>> adapter(
-      new NiceMock<MockBluetoothAdapter>());
-
-  ON_CALL(*adapter, StartDiscoverySession(_, _))
-      .WillByDefault(Invoke(
-          &LayoutTestBluetoothAdapterProvider::SuccessfulDiscoverySession));
+  scoped_refptr<NiceMock<MockBluetoothAdapter>> adapter(GetEmptyAdapter());
 
   adapter->AddMockDevice(GetEmptyDevice(adapter.get()));
 
-  ON_CALL(*adapter, GetDevices())
-      .WillByDefault(Return(adapter->GetMockDevices()));
-
   return adapter.Pass();
 }
 
 // static
 scoped_ptr<NiceMock<MockBluetoothDevice>>
 LayoutTestBluetoothAdapterProvider::GetEmptyDevice(
     MockBluetoothAdapter* adapter) {
   scoped_ptr<NiceMock<MockBluetoothDevice>> empty_device(
       new NiceMock<MockBluetoothDevice>(
           adapter, 0x1F00 /* Bluetooth Class */, "Empty Mock Device name",
           "Empty Mock Device instanceID", true /* Paired */,
           true /* Connected */));
 
   ON_CALL(*empty_device, GetVendorIDSource())
       .WillByDefault(Return(BluetoothDevice::VENDOR_ID_BLUETOOTH));
   ON_CALL(*empty_device, GetVendorID()).WillByDefault(Return(0xFFFF));
   ON_CALL(*empty_device, GetProductID()).WillByDefault(Return(1));
   ON_CALL(*empty_device, GetDeviceID()).WillByDefault(Return(2));
 
   BluetoothDevice::UUIDList list;
   list.push_back(BluetoothUUID("1800"));
   list.push_back(BluetoothUUID("1801"));
   ON_CALL(*empty_device, GetUUIDs()).WillByDefault(Return(list));
   return empty_device.Pass();
 }
 
 // static
-void LayoutTestBluetoothAdapterProvider::SuccessfulDiscoverySession(
-    const BluetoothAdapter::DiscoverySessionCallback& callback,
-    const BluetoothAdapter::ErrorCallback& error_callback) {
+scoped_ptr<NiceMock<MockBluetoothDiscoverySession>>
+LayoutTestBluetoothAdapterProvider::GetDiscoverySession() {
   scoped_ptr<NiceMock<MockBluetoothDiscoverySession>> discovery_session(
       new NiceMock<MockBluetoothDiscoverySession>());
 
   ON_CALL(*discovery_session, Stop(_, _))
-      .WillByDefault(Invoke(
-          &LayoutTestBluetoothAdapterProvider::SuccessfulDiscoverySessionStop));
+      .WillByDefault(RunCallback<0 /* success_callback */>());
 
-  callback.Run(discovery_session.Pass());
-}
-
-// static
-void LayoutTestBluetoothAdapterProvider::SuccessfulDiscoverySessionStop(
-    const base::Closure& callback,
-    const base::Closure& error_callback) {
-  callback.Run();
+  return discovery_session.Pass();
 }
 
 }  // namespace content