device/bluetooth/bluez: add discoverable timeout adapter property

device/bluetooth/bluez: add discoverable timeout adapter property

We already have this in our DBus properties and it is useful for
some consumers such as ARC. Let's expose this on the BluetoothAdapter
for Linux and Chrome OS and implement it there.

BUG=b:29617227
TEST=hook up through ARC and read the adapter's discoverable timeout

Committed: https://crrev.com/8b8a0d9ac73f7a51f46e928ce9574b1029e4fc76
Cr-Commit-Position: refs/heads/master@{#402010}

[Eric Caruso, 2016-06-23 22:54:20]

ejcaruso@chromium.org changed reviewers:
+ ortuno@chromium.org, rkc@chromium.org

[Eric Caruso, 2016-06-23 22:54:20]

PTAL, thanks.

[ortuno, 2016-06-23 23:22:09]

Please add tests :) Something like the following should might work:


// bluetooth_adapter_unittest.h
InitWithFakeAdapter();

SetDiscoverableTimeout(kSomeTimeout);
EXPECT_EQ(kSomeTimeout, adapter_->GetDiscoverableTimeout());

SetDiscoverableTimeout(kSomeOtherTimeout);
EXPECT_EQ(kSomeOtherTimeout, adapter_->GetDiscoverableTimeout());


// bluetooth_test_bluez.h

void SetAdapterDiscoverableTimeout(uint32_t timeout) {
  fake_bluetooth_adapter_client_->SetDiscoverableTimeout(timeout);
}

// You will need to add fake_bluetooth_adapter_client_ as a member and
initialize it.

// fake_bluetooth_adapter_client.h

void SetDiscoverableTimeout(uint32_t timeout) {
  properties_->discoverable_timeout.ReplaceValue(timeout);
}

[Eric Caruso, 2016-06-23 23:26:03]

On 2016/06/23 23:22:09, ortuno wrote:
> Please add tests :) Something like the following should might work:
> 
> 
> // bluetooth_adapter_unittest.h
> InitWithFakeAdapter();
> 
> SetDiscoverableTimeout(kSomeTimeout);
> EXPECT_EQ(kSomeTimeout, adapter_->GetDiscoverableTimeout());
> 
> SetDiscoverableTimeout(kSomeOtherTimeout);
> EXPECT_EQ(kSomeOtherTimeout, adapter_->GetDiscoverableTimeout());
> 
> 
> // bluetooth_test_bluez.h
> 
> void SetAdapterDiscoverableTimeout(uint32_t timeout) {
>   fake_bluetooth_adapter_client_->SetDiscoverableTimeout(timeout);
> }
> 
> // You will need to add fake_bluetooth_adapter_client_ as a member and
> initialize it.
> 
> // fake_bluetooth_adapter_client.h
> 
> void SetDiscoverableTimeout(uint32_t timeout) {
>   properties_->discoverable_timeout.ReplaceValue(timeout);
> }

Should I add SetDiscoverableTimeout functionality other than just in the tests?
It is a read-write DBus property after all.

[ortuno, 2016-06-23 23:37:41]

No need, unless arc++ needs it.

[rkc, 2016-06-23 23:53:05]

https://codereview.chromium.org/2098653002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/2098653002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter.h:316: virtual uint32_t
GetDiscoverableTimeout() const = 0;
Is this something other platforms may need or is it something we are doing
exclusively for compatibility with ARC++?

[Eric Caruso, 2016-06-24 00:09:05]

On 2016/06/23 23:53:05, rkc wrote:
>
https://codereview.chromium.org/2098653002/diff/1/device/bluetooth/bluetooth_...
> File device/bluetooth/bluetooth_adapter.h (right):
> 
>
https://codereview.chromium.org/2098653002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_adapter.h:316: virtual uint32_t
> GetDiscoverableTimeout() const = 0;
> Is this something other platforms may need or is it something we are doing
> exclusively for compatibility with ARC++?

I'm implementing it for ARC++.

[rkc, 2016-06-24 00:12:35]

If this is something purely for ARC++ and something that no other platforms
(i.e., WebBluetooth) would ever need, we should only keep this in the BlueZ
specific classes. ARC++ can directly use BlueZ classes. As far as I can see,
this is something that I cannot think of a reason why WebBluetooth would ever
need.


This saves us polluting the x-platform API with calls that only one platform
will ever need. It also means less plumbing and less complexity.

[rkc, 2016-06-24 00:14:48]

Usually, I am always in the favor of having tests. I am not quite sure if we can
have a meaningful test for this though. It is quite literally, a getter.

[ortuno, 2016-06-24 18:03:31]

On 2016/06/24 at 00:14:48, rkc wrote:
> Usually, I am always in the favor of having tests. I am not quite sure if we
can have a meaningful test for this though. It is quite literally, a getter.

I think we still need a test.

1. This ends up calling all the way down to the dbus properties and a
refactoring/clean could easily break things.
2. You would be surprised how easy is to make mistakes on getters/setters ;)
https://codereview.chromium.org/1317933003

[Eric Caruso, 2016-06-24 18:48:26]

OK, so I think we need to decide whether or not we keep this in
BluetoothAdapterBlueZ, or do we foresee any reason to have this on the
cross-platform bluetooth stack? If not, then I can add a new
bluetooth_adapter_bluez_unittest file for platform-specific tests.

Is there any more general way to test dbus::Properties?

[ortuno, 2016-06-24 19:06:10]

On 2016/06/24 at 18:48:26, ejcaruso wrote:
> OK, so I think we need to decide whether or not we keep this in
BluetoothAdapterBlueZ, or do we foresee any reason to have this on the
cross-platform bluetooth stack? If not, then I can add a new
bluetooth_adapter_bluez_unittest file for platform-specific tests.
> 
> Is there any more general way to test dbus::Properties?

See
https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_bluez_u...

[Eric Caruso, 2016-06-24 19:53:55]

On 2016/06/24 19:06:10, ortuno wrote:
> On 2016/06/24 at 18:48:26, ejcaruso wrote:
> > OK, so I think we need to decide whether or not we keep this in
> BluetoothAdapterBlueZ, or do we foresee any reason to have this on the
> cross-platform bluetooth stack? If not, then I can add a new
> bluetooth_adapter_bluez_unittest file for platform-specific tests.
> > 
> > Is there any more general way to test dbus::Properties?
> 
> See
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_bluez_u...

OK, I will add a test there.

[Eric Caruso, 2016-06-24 20:28:08]

Description was changed from

==========
device/bluetooth: add discoverable timeout adapter property

We already have this in our DBus properties and it is useful for
some consumers such as ARC. Let's expose this on the BluetoothAdapter
for Linux and Chrome OS and implement it there.

BUG=b:29617227
TEST=hook up through ARC and read the adapter's discoverable timeout
==========

to

==========
device/bluetooth/bluez: add discoverable timeout adapter property

We already have this in our DBus properties and it is useful for
some consumers such as ARC. Let's expose this on the BluetoothAdapter
for Linux and Chrome OS and implement it there.

BUG=b:29617227
TEST=hook up through ARC and read the adapter's discoverable timeout
==========

[Eric Caruso, 2016-06-24 20:28:21]

PTAL.

[ortuno, 2016-06-24 20:35:53]

lgtm with a nit and a small issue.

https://codereview.chromium.org/2098653002/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_bluez_unittest.cc (right):

https://codereview.chromium.org/2098653002/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4166: const uint32_t
kShortDiscoverableTimeout = 30;
nit: Can you use constexpr here?

https://codereview.chromium.org/2098653002/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4174:
EXPECT_EQ(adapter_bluez->GetDiscoverableTimeout(), kShortDiscoverableTimeout);
Swap these two arguments. The first value should be the expected value and the
second should be the actual value. Same below.

https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?q=EX...

[rkc, 2016-06-24 20:39:14]

lgtm % (ortuno's comments)

[Eric Caruso, 2016-06-24 20:43:10]

Thanks!

[Eric Caruso, 2016-06-24 20:43:14]

The CQ bit was checked by ejcaruso@chromium.org

[Eric Caruso, 2016-06-24 20:43:14]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org,
ortuno@chromium.org
Link to the patchset: https://codereview.chromium.org/2098653002/#ps60001
(title: "fixing nits")

[commit-bot: I haz the power, 2016-06-24 20:43:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 22:02:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 22:02:49]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Eric Caruso, 2016-06-24 22:06:30]

The CQ bit was checked by ejcaruso@chromium.org

[commit-bot: I haz the power, 2016-06-24 22:07:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 23:27:49]

Description was changed from

==========
device/bluetooth/bluez: add discoverable timeout adapter property

We already have this in our DBus properties and it is useful for
some consumers such as ARC. Let's expose this on the BluetoothAdapter
for Linux and Chrome OS and implement it there.

BUG=b:29617227
TEST=hook up through ARC and read the adapter's discoverable timeout
==========

to

==========
device/bluetooth/bluez: add discoverable timeout adapter property

We already have this in our DBus properties and it is useful for
some consumers such as ARC. Let's expose this on the BluetoothAdapter
for Linux and Chrome OS and implement it there.

BUG=b:29617227
TEST=hook up through ARC and read the adapter's discoverable timeout
==========

[commit-bot: I haz the power, 2016-06-24 23:27:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-24 23:30:35]

Description was changed from

==========
device/bluetooth/bluez: add discoverable timeout adapter property

We already have this in our DBus properties and it is useful for
some consumers such as ARC. Let's expose this on the BluetoothAdapter
for Linux and Chrome OS and implement it there.

BUG=b:29617227
TEST=hook up through ARC and read the adapter's discoverable timeout
==========

to

==========
device/bluetooth/bluez: add discoverable timeout adapter property

We already have this in our DBus properties and it is useful for
some consumers such as ARC. Let's expose this on the BluetoothAdapter
for Linux and Chrome OS and implement it there.

BUG=b:29617227
TEST=hook up through ARC and read the adapter's discoverable timeout

Committed: https://crrev.com/8b8a0d9ac73f7a51f46e928ce9574b1029e4fc76
Cr-Commit-Position: refs/heads/master@{#402010}
==========

[commit-bot: I haz the power, 2016-06-24 23:30:36]

Patchset 4 (id:??) landed as
https://crrev.com/8b8a0d9ac73f7a51f46e928ce9574b1029e4fc76
Cr-Commit-Position: refs/heads/master@{#402010}