Add GetConnectionInfo function for BluetoothDevice, replacing the existing

Add GetConnectionInfo function for BluetoothDevice, replacing the existing
GetRSSI, GetHostTransmitPower, and GetMaximumHostTransmitPower functions.

On Mac, this function uses IOBluetooth APIs.
On ChromeOS, this function is a wrapper for the GetConnInfo() DBus plugin API.
On Windows, this function is unimplemented.

BUG=382683
Committed: https://crrev.com/d060690f69581b758adb9b477ebe5e08acbccfa7
Cr-Commit-Position: refs/heads/master@{#310372}

[Tim Song, 2014-11-19 00:33:36]

tengs@chromium.org changed reviewers:
+ isherman@chromium.org, keybuk@chromium.org

[Ilya Sherman, 2014-11-19 22:18:08]

Thanks, Tim.

https://codereview.chromium.org/735893002/diff/1/chromeos/dbus/bluetooth_devi...
File chromeos/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/1/chromeos/dbus/bluetooth_devi...
chromeos/dbus/bluetooth_device_client.cc:22: const int kUnknownPower = 127;
nit: It seems a shame to define this in two separate places -- any chance of
sharing the code somehow?

https://codereview.chromium.org/735893002/diff/1/chromeos/dbus/bluetooth_devi...
chromeos/dbus/bluetooth_device_client.cc:354:
reader.PopInt16(&max_transmit_power);
Are these PopInt16 calls guaranteed to succeed, or should there be
error-handling code?

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
File device/bluetooth/bluetooth_chromeos_unittest.cc (right):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:149:
BluetoothDevice::ConnectionInfo conn_info) {
nit: Pass by const-reference?

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3202: int unkown_power =
BluetoothDevice::kUnknownPower;
nit: Please add an explicit check that the unknown power is not equal to 0.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3202: int unkown_power =
BluetoothDevice::kUnknownPower;
nit: "unkown" -> "unknown"

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3205: ASSERT_EQ(unkown_power,
conn_info.max_transmit_power);
nit: s/ASSERT/EXPECT.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3214: // Calling
GetConnectionInfo for an unconnected device should return valid
nit: "an unconnected" -> "a connected"

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3222: ASSERT_EQ(4,
conn_info.max_transmit_power);
nit: s/ASSERT/EXPECT.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3223: }
Optional nit: I'd write this as two separate tests, rather than a single test
with two parts.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_chromeos.cc:503: void
BluetoothDeviceChromeOS::OnGetConnInfo(
nit: "Conn" -> "Connection"; ditto below (though I guess you've chosen this name
to match the DBUS API name; I'd prefer to rename that too, but if that's not
feasible, then maybe consistency with that name is more important).

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device_mac.mm (right):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_mac.mm:154: rssi = kUnknownPower;
nit: rssi -> connection_info.rssi

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_mac.mm:158: connection.max_transmit_power =
nit: connection -> connection_info

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/test/mock_b...
File device/bluetooth/test/mock_bluetooth_device.cc (left):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/test/mock_b...
device/bluetooth/test/mock_bluetooth_device.cc:34:
.WillByDefault(testing::Return(kUnknownPower));
Do you not need a default value to return for calls to GetConnectionInfo()?

https://codereview.chromium.org/735893002/diff/1/extensions/browser/api/bluet...
File extensions/browser/api/bluetooth/bluetooth_api_utils.cc (left):

https://codereview.chromium.org/735893002/diff/1/extensions/browser/api/bluet...
extensions/browser/api/bluetooth/bluetooth_api_utils.cc:126: }
Do you want to remove these from the IDL as well?

[Tim Song, 2014-12-06 00:51:38]

https://codereview.chromium.org/735893002/diff/1/chromeos/dbus/bluetooth_devi...
File chromeos/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/1/chromeos/dbus/bluetooth_devi...
chromeos/dbus/bluetooth_device_client.cc:22: const int kUnknownPower = 127;
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: It seems a shame to define this in two separate places -- any chance of
> sharing the code somehow?

Sharing this constant would be pretty hard to do. chromeos/ can't depend on
device/ and device/ can't depend only on chromeos/ for this constant because it
also needs to use it for other platforms. It's not worth making a separate
shared target.

https://codereview.chromium.org/735893002/diff/1/chromeos/dbus/bluetooth_devi...
chromeos/dbus/bluetooth_device_client.cc:354:
reader.PopInt16(&max_transmit_power);
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> Are these PopInt16 calls guaranteed to succeed, or should there be
> error-handling code?

The return value should always follow this format or there is something
seriously wrong. I added a DCHECK.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
File device/bluetooth/bluetooth_chromeos_unittest.cc (right):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:149:
BluetoothDevice::ConnectionInfo conn_info) {
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: Pass by const-reference?

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3202: int unkown_power =
BluetoothDevice::kUnknownPower;
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: "unkown" -> "unknown"

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3202: int unkown_power =
BluetoothDevice::kUnknownPower;
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: Please add an explicit check that the unknown power is not equal to 0.

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3205: ASSERT_EQ(unkown_power,
conn_info.max_transmit_power);
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: s/ASSERT/EXPECT.

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3214: // Calling
GetConnectionInfo for an unconnected device should return valid
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: "an unconnected" -> "a connected"

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3222: ASSERT_EQ(4,
conn_info.max_transmit_power);
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: s/ASSERT/EXPECT.

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_c...
device/bluetooth/bluetooth_chromeos_unittest.cc:3223: }
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> Optional nit: I'd write this as two separate tests, rather than a single test
> with two parts.

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_chromeos.cc:503: void
BluetoothDeviceChromeOS::OnGetConnInfo(
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: "Conn" -> "Connection"; ditto below (though I guess you've chosen this
name
> to match the DBUS API name; I'd prefer to rename that too, but if that's not
> feasible, then maybe consistency with that name is more important).

The kernel mgmt API was called GetConnInfo, which I copied for the plugin. We
should keep the DBus API name consistent everywhere, and at this point, it would
be too much of a hassle to change the name.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device_mac.mm (right):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_mac.mm:154: rssi = kUnknownPower;
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: rssi -> connection_info.rssi

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_mac.mm:158: connection.max_transmit_power =
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> nit: connection -> connection_info

Done.

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/test/mock_b...
File device/bluetooth/test/mock_bluetooth_device.cc (left):

https://codereview.chromium.org/735893002/diff/1/device/bluetooth/test/mock_b...
device/bluetooth/test/mock_bluetooth_device.cc:34:
.WillByDefault(testing::Return(kUnknownPower));
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> Do you not need a default value to return for calls to GetConnectionInfo()?

GetConnectionInfo is an async method, so adding a default action is a bit more
complicated. The caller of the mock should probably configure it themselves.

https://codereview.chromium.org/735893002/diff/1/extensions/browser/api/bluet...
File extensions/browser/api/bluetooth/bluetooth_api_utils.cc (left):

https://codereview.chromium.org/735893002/diff/1/extensions/browser/api/bluet...
extensions/browser/api/bluetooth/bluetooth_api_utils.cc:126: }
On 2014/11/19 22:18:07, Ilya Sherman wrote:
> Do you want to remove these from the IDL as well?

Done.

[Ilya Sherman, 2014-12-06 03:18:37]

LGTM, thanks.

[Tim Song, 2014-12-11 18:16:46]

ping keybuk

[Tim Song, 2015-01-06 19:29:55]

tengs@chromium.org changed reviewers:
+ armansito@chromium.org

[Tim Song, 2015-01-06 19:29:55]

Arman, can you take a look?

[armansito, 2015-01-06 20:36:08]

Also, I think you meant "IOBluetooth" instead of "CoreBluetooth" in the commit
message. Please correct this to avoid confusion, since those to are two
different libraries.

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.cc:354: success &=
reader.PopInt16(&max_transmit_power);
I find this a bit difficult to read. It might be a little more clear to place
this within an if statement and short circuit in case of failure? Maybe do:

  if (!response)
    LOG(ERROR) << ...
  else if (!reader.PopInt16(&rssi) || !reader.PopInt16(..) || !reader...)
    LOG(WARNING) << "Received invalid response";

  callback.Run(..)

This way you would also avoid the DCHECK, since you wouldn't be asserting on the
behavior of a message received from a different process. Just a suggestion.

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.h (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:173: // The callback invoked for as
successful GetConnInfo API call with the
nit: s/for as/for a/

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_device_client.cc:226: max_transmit_power_(0) {
Want to set these to 127 by default?

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_device_client.h (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_device_client.h:72: virtual void GetConnInfo(const
dbus::ObjectPath& object_path,
nit: Drop "virtual" when you use "override".

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_chromeos_unittest.cc (right):

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_chromeos_unittest.cc:3303: device->Connect(NULL,
base::Bind(&BluetoothChromeOSTest::Callback,
nit: Indentation, break the line after "NULL". I would also align all the
arguments on the left for better readability:

  device->Connect(
       NULL,
       base::Bind(....),
       ...);

Or just run git cl format.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device.h:84: max_transmit_power(max_transmit_power)
{}
Even though this is a simple struct, I would move the method definitions to the
source file. Also make sure to declare a destructor.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device.h:292: // the callback will filled by
|kUnknownPower|.
nit: s/will filled by/will be populated with/

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device.h:293: virtual void GetConnectionInfo(const
ConnectionInfoCallback& callback) = 0;
Maybe it's better to have an ErrorCallback instead of returning an invalid
ConnectionInfo struct in the case of failure? Up to you.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.h:49: virtual void GetConnectionInfo(
nit: Omit "virtual" when you have "override". I know the existing code does this
incorrectly (I think the person who refactored it just did a find and replace
for OVERRIDE), but let's use the correct style for new code.

[Tim Song, 2015-01-06 22:30:51]

Thanks for the review. I also fixed the comment.

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.cc:354: success &=
reader.PopInt16(&max_transmit_power);
On 2015/01/06 20:36:07, armansito wrote:
> I find this a bit difficult to read. It might be a little more clear to place
> this within an if statement and short circuit in case of failure? Maybe do:
> 
>   if (!response)
>     LOG(ERROR) << ...
>   else if (!reader.PopInt16(&rssi) || !reader.PopInt16(..) || !reader...)
>     LOG(WARNING) << "Received invalid response";
> 
>   callback.Run(..)
> 
> This way you would also avoid the DCHECK, since you wouldn't be asserting on
the
> behavior of a message received from a different process. Just a suggestion.

Done. I don't think it's defined to create a MessageReader with a null response,
so I kept the outer if statement.

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.h (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:173: // The callback invoked for as
successful GetConnInfo API call with the
On 2015/01/06 20:36:07, armansito wrote:
> nit: s/for as/for a/

Done.

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_device_client.cc:226: max_transmit_power_(0) {
On 2015/01/06 20:36:07, armansito wrote:
> Want to set these to 127 by default?

Done.

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_device_client.h (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_device_client.h:72: virtual void GetConnInfo(const
dbus::ObjectPath& object_path,
On 2015/01/06 20:36:07, armansito wrote:
> nit: Drop "virtual" when you use "override".

Done.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_chromeos_unittest.cc (right):

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_chromeos_unittest.cc:3303: device->Connect(NULL,
base::Bind(&BluetoothChromeOSTest::Callback,
On 2015/01/06 20:36:07, armansito wrote:
> nit: Indentation, break the line after "NULL". I would also align all the
> arguments on the left for better readability:
> 
>   device->Connect(
>        NULL,
>        base::Bind(....),
>        ...);
> 
> Or just run git cl format.

The current formatting is from git cl format, but I think your aligned
formatting reads nicer.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device.h:84: max_transmit_power(max_transmit_power)
{}
On 2015/01/06 20:36:07, armansito wrote:
> Even though this is a simple struct, I would move the method definitions to
the
> source file. Also make sure to declare a destructor.

Done.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device.h:292: // the callback will filled by
|kUnknownPower|.
On 2015/01/06 20:36:07, armansito wrote:
> nit: s/will filled by/will be populated with/

Done.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device.h:293: virtual void GetConnectionInfo(const
ConnectionInfoCallback& callback) = 0;
On 2015/01/06 20:36:08, armansito wrote:
> Maybe it's better to have an ErrorCallback instead of returning an invalid
> ConnectionInfo struct in the case of failure? Up to you.

The underlying dbus/mac apis might return invalid values, so I think we should
try to have one consistent error state.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.h:49: virtual void GetConnectionInfo(
On 2015/01/06 20:36:08, armansito wrote:
> nit: Omit "virtual" when you have "override". I know the existing code does
this
> incorrectly (I think the person who refactored it just did a find and replace
> for OVERRIDE), but let's use the correct style for new code.

Done.

[armansito, 2015-01-06 22:39:29]

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.cc:354: success &=
reader.PopInt16(&max_transmit_power);
On 2015/01/06 22:30:50, Tim Song wrote:
> On 2015/01/06 20:36:07, armansito wrote:
> > I find this a bit difficult to read. It might be a little more clear to
place
> > this within an if statement and short circuit in case of failure? Maybe do:
> > 
> >   if (!response)
> >     LOG(ERROR) << ...
> >   else if (!reader.PopInt16(&rssi) || !reader.PopInt16(..) || !reader...)
> >     LOG(WARNING) << "Received invalid response";
> > 
> >   callback.Run(..)
> > 
> > This way you would also avoid the DCHECK, since you wouldn't be asserting on
> the
> > behavior of a message received from a different process. Just a suggestion.
> 
> Done. I don't think it's defined to create a MessageReader with a null
response,
> so I kept the outer if statement.

You can also do an early return if response is NULL. I prefer not to have many
nested levels of if statements since the code just ends up becoming confusing.
So maybe just run the callback and return if response is NULL, otherwise you
create a MessageReader and parse the response. I'd rather run the callback in
two different places than have many levels of ifs.

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/735893002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device.h:293: virtual void GetConnectionInfo(const
ConnectionInfoCallback& callback) = 0;
On 2015/01/06 22:30:51, Tim Song wrote:
> On 2015/01/06 20:36:08, armansito wrote:
> > Maybe it's better to have an ErrorCallback instead of returning an invalid
> > ConnectionInfo struct in the case of failure? Up to you.
> 
> The underlying dbus/mac apis might return invalid values, so I think we should
> try to have one consistent error state.

That's what I figured why you chose to do it this way :) sgtm

[Tim Song, 2015-01-06 23:02:17]

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/735893002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.cc:354: success &=
reader.PopInt16(&max_transmit_power);
On 2015/01/06 22:39:29, armansito wrote:
> On 2015/01/06 22:30:50, Tim Song wrote:
> > On 2015/01/06 20:36:07, armansito wrote:
> > > I find this a bit difficult to read. It might be a little more clear to
> place
> > > this within an if statement and short circuit in case of failure? Maybe
do:
> > > 
> > >   if (!response)
> > >     LOG(ERROR) << ...
> > >   else if (!reader.PopInt16(&rssi) || !reader.PopInt16(..) || !reader...)
> > >     LOG(WARNING) << "Received invalid response";
> > > 
> > >   callback.Run(..)
> > > 
> > > This way you would also avoid the DCHECK, since you wouldn't be asserting
on
> > the
> > > behavior of a message received from a different process. Just a
suggestion.
> > 
> > Done. I don't think it's defined to create a MessageReader with a null
> response,
> > so I kept the outer if statement.
> 
> You can also do an early return if response is NULL. I prefer not to have many
> nested levels of if statements since the code just ends up becoming confusing.
> So maybe just run the callback and return if response is NULL, otherwise you
> create a MessageReader and parse the response. I'd rather run the callback in
> two different places than have many levels of ifs.

Done.

[armansito, 2015-01-06 23:04:19]

lgtm

[Tim Song, 2015-01-07 01:35:58]

tengs@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Tim Song, 2015-01-07 01:35:59]

+jyasskin@ for bluetooth.idl

[Jeffrey Yasskin, 2015-01-07 01:52:35]

LGTM. I'm looking forward to having access to this data. :)

[Tim Song, 2015-01-07 21:18:57]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2015-01-07 21:19:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735893002/80001

[commit-bot: I haz the power, 2015-01-07 21:31:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-07 21:32:39]

Patchset 5 (id:??) landed as
https://crrev.com/d060690f69581b758adb9b477ebe5e08acbccfa7
Cr-Commit-Position: refs/heads/master@{#310372}