Bluetooth: D-Bus client interface for org.bluez.Input1

Bluetooth: D-Bus client interface for org.bluez.Input1

Implements the D-Bus client for the new Input1 interface currently
exporting only one property.

BUG=220553
TEST=device_unittests pass.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195350

[deymo, 2013-04-13 01:28:12]

This is WIP I think.
Not sure if I have to implement the mock_* or implement the fake_* version.

[keybuk, 2013-04-13 03:34:54]

On 2013/04/13 01:28:12, deymo wrote:
> This is WIP I think.
> Not sure if I have to implement the mock_* or implement the fake_* version.

Let's go with the fake_ version from the get go, since you're not about to use
the mock in a test

[keybuk, 2013-04-13 03:35:00]

https://codereview.chromium.org/14048007/diff/2001/chromeos/dbus/experimental...
File chromeos/dbus/experimental_bluetooth_input_client.cc (right):

https://codereview.chromium.org/14048007/diff/2001/chromeos/dbus/experimental...
chromeos/dbus/experimental_bluetooth_input_client.cc:133: : public
ExperimentalBluetoothInputClient {
This should now be named FakeBluetoothInputClient and go in a separate file, see
for example:

https://codereview.chromium.org/13637016/diff/5001/chromeos/dbus/fake_bluetoo...
https://codereview.chromium.org/13637016/diff/5001/chromeos/dbus/fake_bluetoo...

It's the same code, just done as a separate file

https://codereview.chromium.org/14048007/diff/2001/chromeos/dbus/experimental...
chromeos/dbus/experimental_bluetooth_input_client.cc:194: VLOG(1) <<
"GetProperties: " << object_path.value();
The continual GetProperties log messages will drive you nuts ;-) delete this
line

https://codereview.chromium.org/14048007/diff/2001/chromeos/dbus/experimental...
chromeos/dbus/experimental_bluetooth_input_client.cc:228: return new
ExperimentalBluetoothInputClientStubImpl();
new Fake...

[deymo, 2013-04-15 23:00:48]

PTAL.

[keybuk, 2013-04-15 23:12:51]

https://codereview.chromium.org/14048007/diff/6001/chromeos/dbus/fake_bluetoo...
File chromeos/dbus/fake_bluetooth_input_client.h (right):

https://codereview.chromium.org/14048007/diff/6001/chromeos/dbus/fake_bluetoo...
chromeos/dbus/fake_bluetooth_input_client.h:45: void AnnounceInputDevice(const
dbus::ObjectPath& object_path);
For completeness, should have a "Remove" too - since tests are going to have to
cover Disconnection

[deymo, 2013-04-15 23:43:19]

Fake impl now includes a removal method. PTAL.
Also added the InputAdded and InputRemoved calls and a missing constructor (=D).
Should be ok now.

[keybuk, 2013-04-15 23:57:34]

lgtm

https://codereview.chromium.org/14048007/diff/11001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_input_client.h (right):

https://codereview.chromium.org/14048007/diff/11001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_input_client.h:45: void AnnounceInputDevice(const
dbus::ObjectPath& object_path);
nit AddInputDevice ?

[deymo, 2013-04-16 00:02:41]

Adding satorux@ for OWNER review.

[satorux1, 2013-04-16 06:19:14]

LGTM

[commit-bot: I haz the power, 2013-04-18 00:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/14048007/14001

[commit-bot: I haz the power, 2013-04-18 01:07:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[deymo, 2013-04-18 07:00:50]

new patch merged with keybuk's commit.

[keybuk, 2013-04-18 15:51:30]

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_input_client.cc (right):

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_input_client.cc:84: // TODO(deymo): Return a
different ReconnectMode value for different devices.
since the requisite CL has landed, it makes sense to do this now and include
unit tests for this change rather than leave that to another CL

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_input_client.h (right):

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_input_client.h:19: // Input objects.
This should be changed to reflect why the Fake* class is different from the main
class - see FakeBluetoothDeviceClient for an example

[deymo, 2013-04-18 20:20:16]

PTAL. New patch with the implementation of this in bluetooth experimental and
test.

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_input_client.cc (right):

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_input_client.cc:84: // TODO(deymo): Return a
different ReconnectMode value for different devices.
On 2013/04/18 15:51:30, keybuk wrote:
> since the requisite CL has landed, it makes sense to do this now and include
> unit tests for this change rather than leave that to another CL

Done.

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_input_client.h (right):

https://codereview.chromium.org/14048007/diff/33001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_input_client.h:19: // Input objects.
On 2013/04/18 15:51:30, keybuk wrote:
> This should be changed to reflect why the Fake* class is different from the
main
> class - see FakeBluetoothDeviceClient for an example

Done.

[keybuk, 2013-04-18 20:50:25]

https://codereview.chromium.org/14048007/diff/42001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_input_client.cc (right):

https://codereview.chromium.org/14048007/diff/42001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_input_client.cc:113: InputRemoved(object_path));
Object Manager ensures that the Properties* is still retrievable when
InputRemoved is called, and deleted after - to give that callback a chance to
examine it

You should match that behavior here; see FakeBluetoothDeviceClient::RemoveDevice
for an example

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_device_experimental_chromeos.cc (right):

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:94: if
(!input_properties)
This bit is worthy of a comment explaining why this can be NULL (not an Input
device) and why true is the right default

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_experimental_chromeos_unittest.cc (right):

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:1124:
dbus::ObjectPath(FakeBluetoothDeviceClient::kAppleMousePath));
This should be handled by the FakeBluetoothDeviceClient, not by the test case;
e.g. in FakeBluetoothDeviceClient::DiscoverySimulationTimer

Otherwise this property won't be correct on the Linux Desktop

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:1182:
dbus::ObjectPath(FakeBluetoothDeviceClient::kAppleKeyboardPath));
ditto

[deymo, 2013-04-18 22:19:55]

PTAL - new patch.

https://codereview.chromium.org/14048007/diff/42001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_input_client.cc (right):

https://codereview.chromium.org/14048007/diff/42001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_input_client.cc:113: InputRemoved(object_path));
On 2013/04/18 20:50:25, keybuk wrote:
> Object Manager ensures that the Properties* is still retrievable when
> InputRemoved is called, and deleted after - to give that callback a chance to
> examine it
> 
> You should match that behavior here; see
FakeBluetoothDeviceClient::RemoveDevice
> for an example

Done.

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_device_experimental_chromeos.cc (right):

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:94: if
(!input_properties)
On 2013/04/18 20:50:25, keybuk wrote:
> This bit is worthy of a comment explaining why this can be NULL (not an Input
> device) and why true is the right default

Done.

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_experimental_chromeos_unittest.cc (right):

https://codereview.chromium.org/14048007/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:1124:
dbus::ObjectPath(FakeBluetoothDeviceClient::kAppleMousePath));
On 2013/04/18 20:50:25, keybuk wrote:
> This should be handled by the FakeBluetoothDeviceClient, not by the test case;
> e.g. in FakeBluetoothDeviceClient::DiscoverySimulationTimer
> 
> Otherwise this property won't be correct on the Linux Desktop

Done. This is emulated by FakeBluetoothDeviceClient based on the device class.
The Input interface is emulated to appear once the device is paired, to mimic
the real bluez5 behavior.

[keybuk, 2013-04-18 22:37:44]

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_device_client.cc:652: }
This also needs to be run in the Connect() function for the Microsoft Mouse

[deymo, 2013-04-18 23:21:02]

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_device_client.cc:652: }
On 2013/04/18 22:37:44, keybuk wrote:
> This also needs to be run in the Connect() function for the Microsoft Mouse
CompleteSimulatedPairing is called also for the Microsoft Mouse.

[keybuk, 2013-04-18 23:45:09]

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_device_client.cc:652: }
On 2013/04/18 23:21:02, deymo wrote:
> On 2013/04/18 22:37:44, keybuk wrote:
> > This also needs to be run in the Connect() function for the Microsoft Mouse
> CompleteSimulatedPairing is called also for the Microsoft Mouse.

It is, but for the "test a connect failing after pairing" case

It's tested separately with just Connect(), in which case this won't be called

[deymo, 2013-04-19 01:23:22]

Please, take yet another look.

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/14048007/diff/50001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_device_client.cc:652: }
Done.

[keybuk, 2013-04-19 01:56:51]

lgtm

[commit-bot: I haz the power, 2013-04-19 02:47:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/14048007/46016

[commit-bot: I haz the power, 2013-04-19 03:42:18]

Retried try job too often on linux_chromeos for step(s) device_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-19 19:55:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/14048007/57007

[commit-bot: I haz the power, 2013-04-19 20:56:19]

Retried try job too often on mac_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-19 21:17:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/14048007/57007

[commit-bot: I haz the power, 2013-04-20 00:55:00]

Change committed as 195350