Refactoring bluetooth API code.

Renames the classes in chrome/browser/chromeos/bluetooth/ ChromeOs-specific (i.e. BluetoothAdapter => BluetoothAdapterChromeOs) and creating interfaces: BluetoothAdapter and BluetoothDevice.


This CL does the Step 1 & 2 of:

1) Renames the classes in chrome/browser/chromeos/bluetooth/ chromeos-specific
   (i.e. BluetoothAdapter => BluetoothAdapterChromeOs)

2) Create interfaces of the classes in chrome/browser/chromeos/bluetooth/.
   These interfaces will be used in the platform-independent logics.

3) Move everything out of chrome/browser/chromeos/bluetooth/ into
   devices/bluetooth/ since the code is no longer specific to linux/chromeos.

4) Add Windows implementations. (i.e. Create BluetoothAdapterWindows)

BUG=135470
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159615

[youngki, 2012-09-06 19:43:03]

Hi Bryan,

I renamed the files with *_dbus and bluetooth_socket_posix.{h|cc} according to
your suggestion. Please take a look.

Also I added interfaces in this CL as well and I refactored chrome_api.{h|cc} to
use the interfaces instead.

I still haven't decoupled BluetoothSocket and BluetoothOutOfPairingData from the
interfaces yet, and I will find the solutions for those while implementing the
windows bluetooth.

[youngki, 2012-09-07 14:15:05]

CC'ing Kevin.

[kevers, 2012-09-07 15:39:25]

http://codereview.chromium.org/10899037/diff/12001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

http://codereview.chromium.org/10899037/diff/12001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:92: virtual const
std::string& address() const { return address_; }
Virtual methods with non-empty bodies shouldn't be declared inline.  Overriding
method should be marked with OVERRIDE.

[youngki, 2012-09-07 18:18:37]

Now the overridden methods are no longer inline and annotated with OVERRIDE.

http://codereview.chromium.org/10899037/diff/12001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

http://codereview.chromium.org/10899037/diff/12001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:92: virtual const
std::string& address() const { return address_; }
On 2012/09/07 15:39:25, kevers wrote:
> Virtual methods with non-empty bodies shouldn't be declared inline. 
Overriding
> method should be marked with OVERRIDE.

Done.

[bryeung, 2012-09-07 18:35:57]

https://chromiumcodereview.appspot.com/10899037/diff/12001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/12001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc:12: #include
"chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h"
can this use only the BluetoothDeviceInterface instead?

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc:121:
BluetoothAdapterDBus::DeviceList BluetoothAdapterDBus::GetDevices() {
DeviceList is in BluetoothAdapterInterface, not BluetoothAdapterDBus

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:41: class Observer {
The observer belongs in the interface.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_BLUETOOTH_BLUETOOTH_ADAPTER_INTERFACE_H_
This file can just be "bluetooth_adapter.h"

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h:25: class
BluetoothAdapterInterface {
This can just be BluetoothAdapter

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:46: class Observer {
this belongs in the interface

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:54: class
PairingDelegate {
this belongs in the interface

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:273: enum DeviceType {
this belongs in the interface

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_interface.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_interface.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_BLUETOOTH_BLUETOOTH_DEVICE_INTERFACE_H_
This file can just be "bluetooth_device.h"

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_interface.h:25: class
BluetoothDeviceInterface {
This can just be BluetoothDevice

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_service_record_dbus.h:19: //
BluetoothServiceRecordInterface using DBus for Linux/ChromeOS platform.
I think that the XML format is actually dictated by the Bluetooth Spec (i.e.
this might actually be platform independent).

Please verify that this is true, and just leave a comment to the XML spec here
(if you can find one).  (And obviously undo the renaming.)

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h:21: class
BluetoothSocketPosix : public base::RefCounted<BluetoothSocketPosix> {
Please make the interface for this (as we discussed).

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter_dbus.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter_dbus.h:19: class
MockBluetoothAdapterDBus : public BluetoothAdapterDBus {
The mocks should be shared for all platforms.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device_dbus.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device_dbus.h:17: class
MockBluetoothDeviceDBus : public BluetoothDeviceDBus {
The mocks should be shared for all platforms.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:25: const
chromeos::BluetoothAdapterDBus& adapter() const {
this should be using the generic interface instead

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:34: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h"
this should be using the generic interface

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:22: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h"
As a warning: once the interfaces move out of the chromeos directory, we should
be changing this file so that it doesn't need the defined(OS_CHROMEOS) sections
anymore

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:13:
#include "chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h"
should be using the interface

[youngki, 2012-09-13 18:05:02]

Comments all addressed. PTAL.

I left all the method-level comments in bluetooth_adapter_dbus.h and
bluetooth_device_dbus.h. I thought about removing those comments since they are
duplicates of the descriptions in bluetooth_adapter.h and bluetooth_device.h,
but I thought you might have other opinion. Let me know if it is okay to remove
those comments.

https://chromiumcodereview.appspot.com/10899037/diff/12001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/12001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc:12: #include
"chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h"
On 2012/09/07 18:35:57, bryeung wrote:
> can this use only the BluetoothDeviceInterface instead?

This class calls BluetoothDeviceDBus specific functions such as SetObjectPath()
and Update() in its BluetoothAdapterDBus::UpdateDevice() function. Given that
this class is also dbus-specific, I think we can use BluetoothDeviceDBus for now
and later we can further refactor this to use BluetoothDeviceInterface if
possible.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc:121:
BluetoothAdapterDBus::DeviceList BluetoothAdapterDBus::GetDevices() {
On 2012/09/07 18:35:57, bryeung wrote:
> DeviceList is in BluetoothAdapterInterface, not BluetoothAdapterDBus

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:41: class Observer {
On 2012/09/07 18:35:57, bryeung wrote:
> The observer belongs in the interface.

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_BLUETOOTH_BLUETOOTH_ADAPTER_INTERFACE_H_
On 2012/09/07 18:35:57, bryeung wrote:
> This file can just be "bluetooth_adapter.h"

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h:25: class
BluetoothAdapterInterface {
On 2012/09/07 18:35:57, bryeung wrote:
> This can just be BluetoothAdapter

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:46: class Observer {
On 2012/09/07 18:35:57, bryeung wrote:
> this belongs in the interface

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:54: class
PairingDelegate {
On 2012/09/07 18:35:57, bryeung wrote:
> this belongs in the interface

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:273: enum DeviceType {
On 2012/09/07 18:35:57, bryeung wrote:
> this belongs in the interface

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_interface.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_interface.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_BLUETOOTH_BLUETOOTH_DEVICE_INTERFACE_H_
On 2012/09/07 18:35:57, bryeung wrote:
> This file can just be "bluetooth_device.h"

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_interface.h:25: class
BluetoothDeviceInterface {
On 2012/09/07 18:35:57, bryeung wrote:
> This can just be BluetoothDevice

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_service_record_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_service_record_dbus.h:19: //
BluetoothServiceRecordInterface using DBus for Linux/ChromeOS platform.
On 2012/09/07 18:35:57, bryeung wrote:
> I think that the XML format is actually dictated by the Bluetooth Spec (i.e.
> this might actually be platform independent).
> 
> Please verify that this is true, and just leave a comment to the XML spec here
> (if you can find one).  (And obviously undo the renaming.)

I think the logic in this file is pretty much about processing XML data, so I
think we can reuse this in windows platform as well. I also verified that
third_party/libxml compiles in Windows too. I renamed this file back to
bluetooth_service_record.{h|cc} and also BluetoothServiceRecord as well.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h:21: class
BluetoothSocketPosix : public base::RefCounted<BluetoothSocketPosix> {
On 2012/09/07 18:35:57, bryeung wrote:
> Please make the interface for this (as we discussed).

Done. For now I just used fd() as an interface API. When I add Windows
implementation I will rename this function.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter_dbus.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter_dbus.h:19: class
MockBluetoothAdapterDBus : public BluetoothAdapterDBus {
On 2012/09/07 18:35:57, bryeung wrote:
> The mocks should be shared for all platforms.

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device_dbus.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device_dbus.h:17: class
MockBluetoothDeviceDBus : public BluetoothDeviceDBus {
On 2012/09/07 18:35:57, bryeung wrote:
> The mocks should be shared for all platforms.

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:25: const
chromeos::BluetoothAdapterDBus& adapter() const {
On 2012/09/07 18:35:57, bryeung wrote:
> this should be using the generic interface instead

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:34: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h"
On 2012/09/07 18:35:57, bryeung wrote:
> this should be using the generic interface

Done.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:22: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_interface.h"
On 2012/09/07 18:35:57, bryeung wrote:
> As a warning: once the interfaces move out of the chromeos directory, we
should
> be changing this file so that it doesn't need the defined(OS_CHROMEOS)
sections
> anymore

Acknowledged.

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:13:
#include "chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h"
On 2012/09/07 18:35:57, bryeung wrote:
> should be using the interface

Done.

[bryeung, 2012-09-13 19:52:57]

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (left):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:155: static
scoped_refptr<BluetoothAdapter> DefaultAdapter();
this should probably still be in the interface

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:26: class BluetoothAdapter
: public base::RefCounted<BluetoothAdapter> {
If the interface is going to be refcounted, it will need a private destructor
and to declare base::RefCounted as a friend.

That said, I'm not sure it is possible to make an abstract class RefCounted.

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:37: bool present) = 0;
why are these pure virtual now?

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:38: // Adds and
removes observers for events on this bluetooth adapter,
In many places blocks of methods will be marked with the class they are
overridden from, instead of copying the comments.  I think that would be cleaner
here, but I see that Scott has done differently for the *Client overrides.

Scott?

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (left):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:209: bool
ExpectingPinCode() const { return !pincode_callback_.is_null(); }
I think the pairing stuff belongs in the interface.  Is there some reason it
can't be there?

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:187: // BluetoothSocket
object can outlive both this BluetoothDeviceDBus and the
should not be referencing BluetoothDeviceDBus

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:18: virtual int fd() const
= 0;
please at least leave a TODO that this will be replaced with an opaque id when
read/write calls are added

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h (right):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h:27:
MOCK_METHOD2(AdapterDiscoveringChanged,
why did this get line-broken?

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device.cc (right):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device.cc:11:
MockBluetoothDevice::MockBluetoothDevice(
why did this get whitespace changes?

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:386:
bluetooth_adapter_->GetDevice(address));
why can't this be a call on the interface?

[keybuk, 2012-09-13 23:57:30]

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (left):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:34: // generic "default
adapter" which may change depending on availability.
Why the removal of this comment?

We need to deal with the concept of default adapter on all platforms

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:86: // observer methods to
determine which adapter is issuing the event.
Why was this comment dropped?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:100: // where each XX is a
hexadecimal number.
Why was this comment dropped? Please don't remove documentation from the
interface!

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:108: // adapter is only
considered present if the address has been obtained.
Why was this comment dropped?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:116: // will be called. 
On failure, |error_callback| will be called.
Why do you hate documentation?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:155: static
scoped_refptr<BluetoothAdapter> DefaultAdapter();
Agreed, we still want the concept of "Default Adapter"

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:37: bool present) = 0;
Observer methods should remain as empty implementations, they're all optional
for a class to implement

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
rename to bluetooth_adapter_chromeos.cc

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
rename to bluetooth_adapter_chromeos.h

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:24: class
BluetoothDeviceDBus;
This class should not be named ...DBus, that's inconsistent with the rest of
Chrome.

chromeos::BluetoothDeviceChromeOs is probably sufficient and name the file
bluetooth_device_chromeos.{cc,h}

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:29: //
BluetoothAdapter using DBus for Linux/ChromeOS platform.
Chrome OS only currently

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:33: class
BluetoothAdapterDBus : public BluetoothAdapter,
This class should not be named ...DBus, that's inconsistent with the rest of
Chrome.

chromeos::BluetoothAdapterChromeOs is probably sufficient and name the file
bluetooth_adapter_chromeos.{cc,h}

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:38: // Adds and
removes observers for events on this bluetooth adapter,
Chrome style says this should be

// BluetoothAdapter override

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus_unittest.cc:156:
adapter_properties.address.name());
Why does this now need to be double-cast?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (left):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:39: // for devices coming
and going, as well as properties being updated.
Again, why have you removed all of this documentation?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:158: string16
GetAddressWithLocalizedDeviceTypeName() const;
Why has this method gone?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:161: bool IsSupported()
const;
Likewise, why has this method gone?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:169: bool IsVisible() const
{ return visible_; }
Why has this method gone?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:182: const ServiceList&
GetServices() const { return service_uuids_; }
Why has this method gone?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:208: // PIN Code to be
returned.
All of these methods need to be part of BluetoothDevice - the interface has to
deal with pairing

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.cc:65:
BluetoothDevice::DeviceType BluetoothDeviceDBus::GetDeviceType() const {
This method can probably go in the base class, since it's just decoded SIG
values

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
rename to bluetooth_device_chromeos.h

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:39: class
BluetoothDeviceDBus : public BluetoothDevice,
BluetoothDeviceChromeOs

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h:17: // platform.
If this is for Linux/ChromeOS then it should be ChromeOs not Posix

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:148: i !=
devices.end(); ++i) {
what happened here? (x2)

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:154: 
Why has this empty method been added?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:224:
adapter_->GetDevice(address));
Why does this need a static_cast?
This interface needs fixing so this is not necessary!

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:126: bool
discovering) OVERRIDE;
Why has this been added?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/chrome_brow...
File chrome/chrome_browser_chromeos.gypi (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/chrome_brow...
chrome/chrome_browser_chromeos.gypi:137:
'browser/chromeos/bluetooth/bluetooth_service_record_interface.h',
where is this file? I couldn't find it in the CL

[bryeung, 2012-09-14 12:00:21]

Scott: one question about naming (inline).

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:24: class
BluetoothDeviceDBus;
On 2012/09/13 23:57:30, keybuk wrote:
> This class should not be named ...DBus, that's inconsistent with the rest of
> Chrome.
> 
> chromeos::BluetoothDeviceChromeOs is probably sufficient and name the file
> bluetooth_device_chromeos.{cc,h}

Will this remain true even when we want to use these implementations on desktop
Linux?

[youngki, 2012-09-14 15:26:12]

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (left):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:155: static
scoped_refptr<BluetoothAdapter> DefaultAdapter();
On 2012/09/13 23:57:30, keybuk wrote:
> Agreed, we still want the concept of "Default Adapter"

Just discussed with Bryan offline and I will make
bluetooth_adapter_factory.{h|cc} that just contains this static method
DefaultAdapter and Create. And bluetooth_adapter_factory.cc will contain the
implementation in the following format:

#define CHROME_OS
  new_adapter = new BluetoothAdapterDBus;
#endif  // CHROME_OS
#define WINDOWS
  .......
#endif  // WINDOWS

Scott, let me know if you have other opinions on this; for now I will go ahead
with this implementation.

[keybuk, 2012-09-14 17:13:17]

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:24: class
BluetoothDeviceDBus;
Yes, even on Linux the implementation will likely be different because it
doesn't have the Chrome OS thread manager so that'll end up in
BluetoothDeviceLinux alongside BluetoothDeviceChromeOs

[keybuk, 2012-09-14 17:16:36]

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (left):

http://codereview.chromium.org/10899037/diff/13049/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:155: static
scoped_refptr<BluetoothAdapter> DefaultAdapter();
sgtm

[youngki, 2012-09-17 21:53:02]

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (left):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:34: // generic "default
adapter" which may change depending on availability.
On 2012/09/13 23:57:30, keybuk wrote:
> Why the removal of this comment?
> 
> We need to deal with the concept of default adapter on all platforms

Done. Initially I put the second paragraph into bluetooth_adapter_dbus.h. But
now with better understanding of this adapter, I think leaving all in
bluetooth_adapter.h is the right way.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:86: // observer methods to
determine which adapter is issuing the event.
On 2012/09/13 23:57:30, keybuk wrote:
> Why was this comment dropped?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:100: // where each XX is a
hexadecimal number.
On 2012/09/13 23:57:30, keybuk wrote:
> Why was this comment dropped? Please don't remove documentation from the
> interface!

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:108: // adapter is only
considered present if the address has been obtained.
On 2012/09/13 23:57:30, keybuk wrote:
> Why was this comment dropped?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:116: // will be called. 
On failure, |error_callback| will be called.
On 2012/09/13 23:57:30, keybuk wrote:
> Why do you hate documentation?

I thought those are implementation specific that's why I put them into
bluetooth_adapter_dbus.h. Now I think these comments all should stay in
interface.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:155: static
scoped_refptr<BluetoothAdapter> DefaultAdapter();
On 2012/09/14 17:16:36, keybuk wrote:
> sgtm

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:26: class BluetoothAdapter
: public base::RefCounted<BluetoothAdapter> {
On 2012/09/13 19:52:57, bryeung wrote:
> If the interface is going to be refcounted, it will need a private destructor
> and to declare base::RefCounted as a friend.
> 
> That said, I'm not sure it is possible to make an abstract class RefCounted.

I just made the destructor protected.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:37: bool present) = 0;
On 2012/09/13 23:57:30, keybuk wrote:
> Observer methods should remain as empty implementations, they're all optional
> for a class to implement

Put them back as empty implementations.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/09/13 23:57:30, keybuk wrote:
> rename to bluetooth_adapter_chromeos.cc

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/09/13 23:57:30, keybuk wrote:
> rename to bluetooth_adapter_chromeos.h

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:24: class
BluetoothDeviceDBus;
On 2012/09/14 17:13:17, keybuk wrote:
> Yes, even on Linux the implementation will likely be different because it
> doesn't have the Chrome OS thread manager so that'll end up in
> BluetoothDeviceLinux alongside BluetoothDeviceChromeOs

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:29: //
BluetoothAdapter using DBus for Linux/ChromeOS platform.
On 2012/09/13 23:57:30, keybuk wrote:
> Chrome OS only currently

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:33: class
BluetoothAdapterDBus : public BluetoothAdapter,
On 2012/09/13 23:57:30, keybuk wrote:
> This class should not be named ...DBus, that's inconsistent with the rest of
> Chrome.
> 
> chromeos::BluetoothAdapterChromeOs is probably sufficient and name the file
> bluetooth_adapter_chromeos.{cc,h}

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus.h:38: // Adds and
removes observers for events on this bluetooth adapter,
On 2012/09/13 23:57:30, keybuk wrote:
> Chrome style says this should be
> 
> // BluetoothAdapter override

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_dbus_unittest.cc:156:
adapter_properties.address.name());
On 2012/09/13 23:57:30, keybuk wrote:
> Why does this now need to be double-cast?

Because BluetoothAdapterClient::Observer is implemented by BluetoothAdapterDBus,
not BluetoothAdapter. Since DefaultAdapter() returns BluetoothAdapter, we have
to static_cast it to its subclass BluetoothAdapterDBus, then static_cast it
again to its superclass BluetoothAdapterClient::Observer.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (left):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:39: // for devices coming
and going, as well as properties being updated.
On 2012/09/13 23:57:30, keybuk wrote:
> Again, why have you removed all of this documentation?

I originally put them into BluetoothDeviceDBus.h. I put them back here now.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:158: string16
GetAddressWithLocalizedDeviceTypeName() const;
On 2012/09/13 23:57:30, keybuk wrote:
> Why has this method gone?

This function is only used by its implementation bluetooth_device_dbus.cc. Do we
still want this included in public interface?

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:161: bool IsSupported()
const;
On 2012/09/13 23:57:30, keybuk wrote:
> Likewise, why has this method gone?

This function is only used by bluetooth_adapter_chromeos.cc and 
bluetooth_device_chromeos.cc. I am not sure if we want to expose this to public
interface.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:169: bool IsVisible() const
{ return visible_; }
On 2012/09/13 23:57:30, keybuk wrote:
> Why has this method gone?

This function is only used by bluetooth_adapter_chromeos.cc and 
bluetooth_device_chromeos.cc. I am not sure if we want to expose this to public
interface.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:182: const ServiceList&
GetServices() const { return service_uuids_; }
On 2012/09/13 23:57:30, keybuk wrote:
> Why has this method gone?

This function is only used by bluetooth_adapter_chromeos.cc and 
bluetooth_device_chromeos.cc. I am not sure if we want to expose this to public
interface.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:208: // PIN Code to be
returned.
On 2012/09/13 23:57:30, keybuk wrote:
> All of these methods need to be part of BluetoothDevice - the interface has to
> deal with pairing 

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:209: bool
ExpectingPinCode() const { return !pincode_callback_.is_null(); }
On 2012/09/13 19:52:57, bryeung wrote:
> I think the pairing stuff belongs in the interface.  Is there some reason it
> can't be there?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:187: // BluetoothSocket
object can outlive both this BluetoothDeviceDBus and the
On 2012/09/13 19:52:57, bryeung wrote:
> should not be referencing BluetoothDeviceDBus

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.cc:65:
BluetoothDevice::DeviceType BluetoothDeviceDBus::GetDeviceType() const {
On 2012/09/13 23:57:30, keybuk wrote:
> This method can probably go in the base class, since it's just decoded SIG
> values

Done. I added bluetooth_device.cc to put the implementation. Also I moved enum
type and GetDeviceType() into protected visibility because this is only used by
its subclasses for now.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/09/13 23:57:30, keybuk wrote:
> rename to bluetooth_device_chromeos.h

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_dbus.h:39: class
BluetoothDeviceDBus : public BluetoothDevice,
On 2012/09/13 23:57:30, keybuk wrote:
> BluetoothDeviceChromeOs

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:18: virtual int fd() const
= 0;
On 2012/09/13 19:52:57, bryeung wrote:
> please at least leave a TODO that this will be replaced with an opaque id when
> read/write calls are added

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_socket_posix.h:17: // platform.
On 2012/09/13 23:57:30, keybuk wrote:
> If this is for Linux/ChromeOS then it should be ChromeOs not Posix

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h:27:
MOCK_METHOD2(AdapterDiscoveringChanged,
On 2012/09/13 19:52:57, bryeung wrote:
> why did this get line-broken?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_device.cc:11:
MockBluetoothDevice::MockBluetoothDevice(
On 2012/09/13 19:52:57, bryeung wrote:
> why did this get whitespace changes?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:386:
bluetooth_adapter_->GetDevice(address));
On 2012/09/13 19:52:57, bryeung wrote:
> why can't this be a call on the interface?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:148: i !=
devices.end(); ++i) {
On 2012/09/13 23:57:30, keybuk wrote:
> what happened here? (x2)

I aligned the second line to the one character next to the bracket. I believe
that's part of the C++ style guide (let me know if it's otherwise)

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:154: 
On 2012/09/13 23:57:30, keybuk wrote:
> Why has this empty method been added?

Removed.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:224:
adapter_->GetDevice(address));
On 2012/09/13 23:57:30, keybuk wrote:
> Why does this need a static_cast?
> This interface needs fixing so this is not necessary!

Done.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
(right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/browser/ui/...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h:126: bool
discovering) OVERRIDE;
On 2012/09/13 23:57:30, keybuk wrote:
> Why has this been added?

I added it when Observer was pure virtual; now I removed it after turning it
into empty implementation.

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/chrome_brow...
File chrome/chrome_browser_chromeos.gypi (right):

https://chromiumcodereview.appspot.com/10899037/diff/13049/chrome/chrome_brow...
chrome/chrome_browser_chromeos.gypi:137:
'browser/chromeos/bluetooth/bluetooth_service_record_interface.h',
On 2012/09/13 23:57:30, keybuk wrote:
> where is this file? I couldn't find it in the CL

Hm.. I am not sure why the compiler did not complain about this.. Anyways
removed.

[keybuk, 2012-09-17 22:19:43]

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:20: // The
BluetoothAdapter represents a local Bluetooth adapter which
nit: should "The " be removed, since " class" also has been?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:26: struct
BluetoothOutOfBandPairingData;
why does this need to be added?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:29: // for
ChromeOs platform.
nit: "for Chrome OS."

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:152:
BluetoothAdapterChromeOs* adapter_dbus =
nit: adapter_chromeos

(and each occurrence)

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:16: class
BluetoothAdapterFactory {
Would a "Factory" be an appropriate class to add a method that returned a list
of available adapters?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:25: // "hci0".
I'm not sure what to do about the "device name" bit - that's not cross-platform
at all!

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (left):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:58: };
Why did this enum go away? This is the return type of GetDeviceType() so needs
to be part of the public interface

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: DeviceType
GetDeviceType() const;
This needs to be part of the public interface!

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:161: bool IsSupported()
const;
ok for these to go, since they're internal to the CrOS implementation

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:169: bool IsVisible() const
{ return visible_; }
This needs to be part of the public interface!

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:182: const ServiceList&
GetServices() const { return service_uuids_; }
This needs to be part of the public interface!

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:219: }
This needs to be part of the public interface!

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:21: // The BluetoothDevice
represents a remote Bluetooth device, both
nit: drop "The " since " class" is also dropped?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:277: DeviceType
GetDeviceType() const;
should be public: not protected:

this may be used by the UI to draw appropriate icons, etc.

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:64: }
should move up to BluetoothDevice since it'll be the same implementation
everywhere?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:101: }
I'm tempted to say that this should move up into BluetoothDevice, since it'll be
the same implementation on all platforms

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:116: 
this should stay in BluetoothDeviceChromeOs

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:126: }
Can we not move all these up into BluetoothDevice?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:55: bool
IsVisible() const { return visible_; }
should be an override?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:65: const
ServiceList& GetServices() const { return service_uuids_; }
should be an override?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/b...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:89: }
should be an override?

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

https://codereview.chromium.org/10899037/diff/23051/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:19: class
BluetoothDevice;
Why did this get added?

[youngki, 2012-09-18 18:19:55]

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:20: // The
BluetoothAdapter represents a local Bluetooth adapter which
On 2012/09/17 22:19:43, keybuk wrote:
> nit: should "The " be removed, since " class" also has been?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:26: struct
BluetoothOutOfBandPairingData;
On 2012/09/17 22:19:43, keybuk wrote:
> why does this need to be added?

This struct is used as an argument to
BluetoothAdapterChromeOs::OnReadLocalData().

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:29: // for
ChromeOs platform.
On 2012/09/17 22:19:43, keybuk wrote:
> nit: "for Chrome OS."

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:152:
BluetoothAdapterChromeOs* adapter_dbus =
On 2012/09/17 22:19:43, keybuk wrote:
> nit: adapter_chromeos
> 
> (and each occurrence)

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:16: class
BluetoothAdapterFactory {
On 2012/09/17 22:19:43, keybuk wrote:
> Would a "Factory" be an appropriate class to add a method that returned a list
> of available adapters?

Is that a new method that you want to add? I think if that's a static method and
cannot be contained in the interface, then we can put it here. Also we can
change the name "Factory" to something else if you think this doesn't represent
the Factory pattern.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:25: // "hci0".
On 2012/09/17 22:19:43, keybuk wrote:
> I'm not sure what to do about the "device name" bit - that's not
cross-platform
> at all!

We could move this method to BluetoothAdapterChromeOs for now since nobody is
calling this method. Let me know if that's what we should do.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (left):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:58: };
On 2012/09/17 22:19:43, keybuk wrote:
> Why did this enum go away? This is the return type of GetDeviceType() so needs
> to be part of the public interface

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: DeviceType
GetDeviceType() const;
On 2012/09/17 22:19:43, keybuk wrote:
> This needs to be part of the public interface!

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:161: bool IsSupported()
const;
On 2012/09/17 22:19:43, keybuk wrote:
> ok for these to go, since they're internal to the CrOS implementation

Acknowledged.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:169: bool IsVisible() const
{ return visible_; }
On 2012/09/17 22:19:43, keybuk wrote:
> This needs to be part of the public interface!

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:182: const ServiceList&
GetServices() const { return service_uuids_; }
On 2012/09/17 22:19:43, keybuk wrote:
> This needs to be part of the public interface!

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:219: }
On 2012/09/17 22:19:43, keybuk wrote:
> This needs to be part of the public interface!

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:21: // The BluetoothDevice
represents a remote Bluetooth device, both
On 2012/09/17 22:19:43, keybuk wrote:
> nit: drop "The " since " class" is also dropped?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:277: DeviceType
GetDeviceType() const;
On 2012/09/17 22:19:43, keybuk wrote:
> should be public: not protected:
> 
> this may be used by the UI to draw appropriate icons, etc.

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:64: }
On 2012/09/17 22:19:43, keybuk wrote:
> should move up to BluetoothDevice since it'll be the same implementation
> everywhere?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:101: }
On 2012/09/17 22:19:43, keybuk wrote:
> I'm tempted to say that this should move up into BluetoothDevice, since it'll
be
> the same implementation on all platforms

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:116: 
On 2012/09/17 22:19:43, keybuk wrote:
> this should stay in BluetoothDeviceChromeOs

Yep IsSupported() is already Chrome OS specific.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.cc:126: }
On 2012/09/17 22:19:43, keybuk wrote:
> Can we not move all these up into BluetoothDevice?

Moved IsVisible(), IsBonded(), and IsConnected() to BluetoothDevice.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:55: bool
IsVisible() const { return visible_; }
On 2012/09/17 22:19:43, keybuk wrote:
> should be an override?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:65: const
ServiceList& GetServices() const { return service_uuids_; }
On 2012/09/17 22:19:43, keybuk wrote:
> should be an override?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:89: }
On 2012/09/17 22:19:43, keybuk wrote:
> should be an override?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:19: class
BluetoothDevice;
On 2012/09/17 22:19:43, keybuk wrote:
> Why did this get added?

Removed.

[keybuk1, 2012-09-18 18:42:29]

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:26: struct
BluetoothOutOfBandPairingData;
On 2012/09/18 18:19:56, youngki wrote:
> On 2012/09/17 22:19:43, keybuk wrote:
> > why does this need to be added?
> 
> This struct is used as an argument to
> BluetoothAdapterChromeOs::OnReadLocalData().

If the struct is being declared explicitly, do you still need the include of
bluetooth_out_of_band_client.h ?

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:16: class
BluetoothAdapterFactory {
On 2012/09/18 18:19:56, youngki wrote:
> On 2012/09/17 22:19:43, keybuk wrote:
> > Would a "Factory" be an appropriate class to add a method that returned a
list
> > of available adapters?
> 
> Is that a new method that you want to add? I think if that's a static method
and
> cannot be contained in the interface, then we can put it here. Also we can
> change the name "Factory" to something else if you think this doesn't
represent
> the Factory pattern.

yes, it's a method I have in a CL here that will be added; it sounds like it
belongs in this class, I'm not sure what pattern it represents or if "Factory"
is appropriate.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:25: // "hci0".
On 2012/09/18 18:19:56, youngki wrote:
> On 2012/09/17 22:19:43, keybuk wrote:
> > I'm not sure what to do about the "device name" bit - that's not
> cross-platform
> > at all!
> 
> We could move this method to BluetoothAdapterChromeOs for now since nobody is
> calling this method. Let me know if that's what we should do.

Actually I think we should just drop that from the comment, having a "create for
this address" method is almost certainly still useful!

[keybuk, 2012-09-18 18:53:44]

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:19: #include
"chromeos/dbus/bluetooth_out_of_band_client.h"
as commented in previous patch set, with the explicit struct below, is this
#include still needed?

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:19: #include
"chromeos/dbus/bluetooth_out_of_band_client.h"
still needed with the struct below?

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:38: bool
IsSupported() const;
probably should be private now?

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:12: // The BluetoothSocket
class represents a socket to a specific service on
"BluetoothSocket represents"

[bryeung, 2012-09-18 19:18:39]

looking nearly there

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:28: // generic "default
adapter" which may change depending on availability.
seems this last bit of comment may make more sense in the Factory now

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:179: const
dbus::ObjectPath& adapter_path, bool success) {
arguments on separate lines please

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:254: const
ErrorCallback& error_callback,
looks like whitespace is off here

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:377:
BluetoothAdapterChromeOs::DevicesMap::iterator iter = devices_.find(address);
Does this really need the class prefix?

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:536: const
dbus::ObjectPath& adapter_path, const std::string& address) {
arguments on separate lines please

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:78: private:
friends aren't inherited: you need

friend class base::RefCounted<BluetoothAdapterChromeOs>

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:56:
BluetoothAdapterFactory::DefaultAdapter();
This doesn't really make sense: you're assuming that the adapter you get back
will actually call into the mock methods but the factory could return you an
adapter for a different platform.

Maybe the factory should have methods like DefaultAdapterChromeOsForTesting()?

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:7: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h"
All of this ChromeOS stuff should be hidden behind an ifdef

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:25: // "hci0".
How will device names work in the multi-platform case?

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:40: //
BluetoothDevice override
I'd prefer one header that marks a section of BluetoothDevice overrides, instead
of marking every method (but I'm not that concerned if you prefer it this way
Scott).

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:148: i !=
devices.end(); ++i) {
is that actually the style guide?  I can't find it there.

[keybuk1, 2012-09-18 19:32:24]

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:40: //
BluetoothDevice override
I agree

[youngki, 2012-09-19 01:13:55]

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:26: struct
BluetoothOutOfBandPairingData;
On 2012/09/18 18:42:30, keybuk1 wrote:
> On 2012/09/18 18:19:56, youngki wrote:
> > On 2012/09/17 22:19:43, keybuk wrote:
> > > why does this need to be added?
> > 
> > This struct is used as an argument to
> > BluetoothAdapterChromeOs::OnReadLocalData().
> 
> If the struct is being declared explicitly, do you still need the include of
> bluetooth_out_of_band_client.h ?

Removed.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:16: class
BluetoothAdapterFactory {
On 2012/09/18 18:42:30, keybuk1 wrote:
> On 2012/09/18 18:19:56, youngki wrote:
> > On 2012/09/17 22:19:43, keybuk wrote:
> > > Would a "Factory" be an appropriate class to add a method that returned a
> list
> > > of available adapters?
> > 
> > Is that a new method that you want to add? I think if that's a static method
> and
> > cannot be contained in the interface, then we can put it here. Also we can
> > change the name "Factory" to something else if you think this doesn't
> represent
> > the Factory pattern.
> 
> yes, it's a method I have in a CL here that will be added; it sounds like it
> belongs in this class, I'm not sure what pattern it represents or if "Factory"
> is appropriate.

I think we could just leave the name "Factory" for now, and rename it later when
we come up with a better name.

https://chromiumcodereview.appspot.com/10899037/diff/23051/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:25: // "hci0".
On 2012/09/18 18:42:30, keybuk1 wrote:
> On 2012/09/18 18:19:56, youngki wrote:
> > On 2012/09/17 22:19:43, keybuk wrote:
> > > I'm not sure what to do about the "device name" bit - that's not
> > cross-platform
> > > at all!
> > 
> > We could move this method to BluetoothAdapterChromeOs for now since nobody
is
> > calling this method. Let me know if that's what we should do.
> 
> Actually I think we should just drop that from the comment, having a "create
for
> this address" method is almost certainly still useful!

Done.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:28: // generic "default
adapter" which may change depending on availability.
On 2012/09/18 19:18:39, bryeung wrote:
> seems this last bit of comment may make more sense in the Factory now

Done.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:179: const
dbus::ObjectPath& adapter_path, bool success) {
On 2012/09/18 19:18:39, bryeung wrote:
> arguments on separate lines please

Done.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:254: const
ErrorCallback& error_callback,
On 2012/09/18 19:18:39, bryeung wrote:
> looks like whitespace is off here

Done.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:377:
BluetoothAdapterChromeOs::DevicesMap::iterator iter = devices_.find(address);
On 2012/09/18 19:18:39, bryeung wrote:
> Does this really need the class prefix?

Removed.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc:536: const
dbus::ObjectPath& adapter_path, const std::string& address) {
On 2012/09/18 19:18:39, bryeung wrote:
> arguments on separate lines please

Done.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:19: #include
"chromeos/dbus/bluetooth_out_of_band_client.h"
On 2012/09/18 18:53:44, keybuk wrote:
> as commented in previous patch set, with the explicit struct below, is this
> #include still needed?

Removed.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:78: private:
On 2012/09/18 19:18:39, bryeung wrote:
> friends aren't inherited: you need
> 
> friend class base::RefCounted<BluetoothAdapterChromeOs>

As long as we only use ref_counted pointer for BluetoothAdapter and the
destructors are virtual, we don't need to friend
base::RefCounted<BluetoothAdapterChromeOs> because
BluetoothAdapterChromeOs::~BluetoothAdapterChromeOs() will be called first when
BluetoothAdapter::~BluetoothAdapter() is called by
base::RefCounted<BluetoothAdapter>().

Also the compiler complains that BluetoothAdapterChromeOs cannot inherit
base::RefCounted because its superclass BluetoothAdapter is already inheriting
it.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:56:
BluetoothAdapterFactory::DefaultAdapter();
On 2012/09/18 19:18:39, bryeung wrote:
> This doesn't really make sense: you're assuming that the adapter you get back
> will actually call into the mock methods but the factory could return you an
> adapter for a different platform.
> 
> Maybe the factory should have methods like DefaultAdapterChromeOsForTesting()?

I didn't do it on this CL but what I am planning on doing is to have chrome os
specific part guarded with #ifdef CHROMEOS ... #endif in
bluetooth_adapter_factory.cc. So it should look like:

#ifdef CHROMEOS
BluetoothAdapter* BluetoothAdapterFactory::DefaultAdapter() {
// Return BluetoothAdapterChromeOs
}
#endif

#ifdef WINDOWS
BluetoothAdapter* BluetoothAdapterFactory::DefaultAdapter() {
// Return BluetoothAdapterWindows
}
#endif

And this unittest is for bluetooth_adapter_chromeos only and as long as we test
this unittest under chromeos platform, BluetoothAdapterFactory::DefaultAdapter()
should only return BluetoothAdapterChromeOs.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:7: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h"
On 2012/09/18 19:18:39, bryeung wrote:
> All of this ChromeOS stuff should be hidden behind an ifdef

I was planning on doing it in a different CL when I move this into a common
directory because right now everything in this file is chrome os specific
anyways; but let me know if you want that change in the same CL.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:25: // "hci0".
On 2012/09/18 19:18:39, bryeung wrote:
> How will device names work in the multi-platform case?

I took Scott's suggestion and removed device name part from the comments. This
comment should just say that BluetoothAdapter is created using "address".

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:19: #include
"chromeos/dbus/bluetooth_out_of_band_client.h"
On 2012/09/18 18:53:44, keybuk wrote:
> still needed with the struct below?

Removed.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:38: bool
IsSupported() const;
On 2012/09/18 18:53:44, keybuk wrote:
> probably should be private now?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:40: //
BluetoothDevice override
On 2012/09/18 19:32:24, keybuk1 wrote:
> I agree

Done. I also made the same change on bluetooth_adapter_chromeos.h.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_socket.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_socket.h:12: // The BluetoothSocket
class represents a socket to a specific service on
On 2012/09/18 18:53:44, keybuk wrote:
> "BluetoothSocket represents"

Done.

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:148: i !=
devices.end(); ++i) {
On 2012/09/18 19:18:39, bryeung wrote:
> is that actually the style guide?  I can't find it there.

I also couldn't find it in chrome style guide; Also google3 style guide didn't
mention line breaking for loops..! although I always line-break it at the
parenthesis rather than 4 characters right to the previous line. Anyways I
reverted this change.

[bryeung, 2012-09-19 13:27:16]

Mostly just nits at this point.

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h (right):

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h:78: private:
On 2012/09/19 01:13:55, youngki wrote:
> On 2012/09/18 19:18:39, bryeung wrote:
> > friends aren't inherited: you need
> > 
> > friend class base::RefCounted<BluetoothAdapterChromeOs>
> 
> As long as we only use ref_counted pointer for BluetoothAdapter and the
> destructors are virtual, we don't need to friend
> base::RefCounted<BluetoothAdapterChromeOs> because
> BluetoothAdapterChromeOs::~BluetoothAdapterChromeOs() will be called first
when
> BluetoothAdapter::~BluetoothAdapter() is called by
> base::RefCounted<BluetoothAdapter>().
> 
> Also the compiler complains that BluetoothAdapterChromeOs cannot inherit
> base::RefCounted because its superclass BluetoothAdapter is already inheriting
> it.

Okay, makes sense.

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc (right):

http://codereview.chromium.org/10899037/diff/28001/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:7: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h"
On 2012/09/19 01:13:55, youngki wrote:
> On 2012/09/18 19:18:39, bryeung wrote:
> > All of this ChromeOS stuff should be hidden behind an ifdef
> 
> I was planning on doing it in a different CL when I move this into a common
> directory because right now everything in this file is chrome os specific
> anyways; but let me know if you want that change in the same CL.

Okay.  This class looks pretty weird without that change, but since it's coming
in the very near future I'm fine for this to land as is.

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:17: // instantiate
either a specific bluetooth adapter, or for the generic "default
s/for//

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:26: // Creates an
instance for a specific adapter named by |address|, which
Instead of "named by ..."  maybe "at address |address|."  (And then you don't
need the second line either.)

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: bool IsVisible() const
{ return visible_; }
why not virtual?

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:38: const
ServiceList& GetServices() const OVERRIDE;
missing virtual

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:39: virtual void
GetServiceRecords(const ServiceRecordsCallback& callback,
I think these would all be easier to read if they were wrapped like the
*OutOfBand* methods at the bottom.  (And the consistency would be nice too,
since you had to do it below.)

[youngki, 2012-09-19 19:35:23]

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/28001/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:7: #include
"chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos.h"
On 2012/09/19 13:27:16, bryeung wrote:
> On 2012/09/19 01:13:55, youngki wrote:
> > On 2012/09/18 19:18:39, bryeung wrote:
> > > All of this ChromeOS stuff should be hidden behind an ifdef
> > 
> > I was planning on doing it in a different CL when I move this into a common
> > directory because right now everything in this file is chrome os specific
> > anyways; but let me know if you want that change in the same CL.
> 
> Okay.  This class looks pretty weird without that change, but since it's
coming
> in the very near future I'm fine for this to land as is.

Added TODO at the top of the file.

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:17: // instantiate
either a specific bluetooth adapter, or for the generic "default
On 2012/09/19 13:27:16, bryeung wrote:
> s/for//

Done.

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.h:26: // Creates an
instance for a specific adapter named by |address|, which
On 2012/09/19 13:27:16, bryeung wrote:
> Instead of "named by ..."  maybe "at address |address|."  (And then you don't
> need the second line either.)

Done.

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: bool IsVisible() const
{ return visible_; }
On 2012/09/19 13:27:16, bryeung wrote:
> why not virtual?

Changed it to virtual. The original code didn't have this virtual, that's why I
left it as is.

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:38: const
ServiceList& GetServices() const OVERRIDE;
On 2012/09/19 13:27:16, bryeung wrote:
> missing virtual

Done.

https://chromiumcodereview.appspot.com/10899037/diff/33002/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device_chromeos.h:39: virtual void
GetServiceRecords(const ServiceRecordsCallback& callback,
On 2012/09/19 13:27:16, bryeung wrote:
> I think these would all be easier to read if they were wrapped like the
> *OutOfBand* methods at the bottom.  (And the consistency would be nice too,
> since you had to do it below.)

Done.

[bryeung, 2012-09-19 20:13:34]

lgtm

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: bool IsVisible() const
{ return visible_; }
On 2012/09/19 19:35:23, youngki wrote:
> On 2012/09/19 13:27:16, bryeung wrote:
> > why not virtual?
> 
> Changed it to virtual. The original code didn't have this virtual, that's why
I
> left it as is.

Might as well move the body into the cc file to match the rest now as well.

[youngki, 2012-09-19 20:22:08]

Scott, could you take a look?

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: bool IsVisible() const
{ return visible_; }
On 2012/09/19 20:13:35, bryeung wrote:
> On 2012/09/19 19:35:23, youngki wrote:
> > On 2012/09/19 13:27:16, bryeung wrote:
> > > why not virtual?
> > 
> > Changed it to virtual. The original code didn't have this virtual, that's
why
> I
> > left it as is.
> 
> Might as well move the body into the cc file to match the rest now as well.

Done.

[youngki, 2012-09-21 14:35:41]

Friendly ping. Scott, could you take a look?

On 2012/09/19 20:22:08, youngki wrote:
> Scott, could you take a look?
> 
>
http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
> File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):
> 
>
http://codereview.chromium.org/10899037/diff/33002/chrome/browser/chromeos/bl...
> chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: bool IsVisible()
const
> { return visible_; }
> On 2012/09/19 20:13:35, bryeung wrote:
> > On 2012/09/19 19:35:23, youngki wrote:
> > > On 2012/09/19 13:27:16, bryeung wrote:
> > > > why not virtual?
> > > 
> > > Changed it to virtual. The original code didn't have this virtual, that's
> why
> > I
> > > left it as is.
> > 
> > Might as well move the body into the cc file to match the rest now as well.
> 
> Done.

[keybuk, 2012-09-21 19:16:58]

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:154: 
I'm still uneasy about this ... we're betting without testing that the
BluetoothAdapter* returned is in fact a BluetoothAdapterChromeOs and not some
other object.

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:20:
default_adapter = LAZY_INSTANCE_INITIALIZER;
This seems weirdly platform-specific when it shouldn't need to be. Why isn't
this storing a chromeos::BluetoothAdapter

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:31:
default_adapter.Get()->TrackDefaultAdapter();
Is this using chromeos::BluetoothAdapter::weak_ptr_factory_ or
BluetoothAdapterChromeOs ?

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:39:
BluetoothAdapterChromeOs* adapter = new BluetoothAdapterChromeOs;
This is the only ChromeOs-specific line in this method

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:140: // TODO(keybuk):
examine protocol-specific connected state, such as Input
drop this todo, it won't go in this class anyway

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:305: bool connected_;
I'm trying to work out in my head whether this works having them protected: ->
they're updated by a friend class

[youngki, 2012-09-22 02:03:29]

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:154: 
On 2012/09/21 19:16:59, keybuk wrote:
> I'm still uneasy about this ... we're betting without testing that the
> BluetoothAdapter* returned is in fact a BluetoothAdapterChromeOs and not some
> other object.

I added ASSERT_TRUE(adapter_chromeos != NULL) to check that BluetoothAdapter*
returned is BluetoothAdapterChromeOs.

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:20:
default_adapter = LAZY_INSTANCE_INITIALIZER;
On 2012/09/21 19:16:59, keybuk wrote:
> This seems weirdly platform-specific when it shouldn't need to be. Why isn't
> this storing a chromeos::BluetoothAdapter

I was going to change it in the other CL, but I decided to change it now.

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:31:
default_adapter.Get()->TrackDefaultAdapter();
On 2012/09/21 19:16:59, keybuk wrote:
> Is this using chromeos::BluetoothAdapter::weak_ptr_factory_ or
> BluetoothAdapterChromeOs ?

TrackDefaultAdapter() is right now BluetoothAdapterChromeOs method.

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_factory.cc:39:
BluetoothAdapterChromeOs* adapter = new BluetoothAdapterChromeOs;
On 2012/09/21 19:16:59, keybuk wrote:
> This is the only ChromeOs-specific line in this method

I will guard it with #ifdef CHROME_OS in the next CL.

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.cc (right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.cc:140: // TODO(keybuk):
examine protocol-specific connected state, such as Input
On 2012/09/21 19:16:59, keybuk wrote:
> drop this todo, it won't go in this class anyway

Done.

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/37005/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:305: bool connected_;
On 2012/09/21 19:16:59, keybuk wrote:
> I'm trying to work out in my head whether this works having them protected: ->
> they're updated by a friend class

Hm.. At least I didn't get any compiler error.. So I guess having them protected
should be okay..?

[keybuk, 2012-09-22 02:13:26]

lgtm

[youngki, 2012-09-25 12:35:37]

Bryan,

Could you suggest who else I should get approvals from?

[youngki, 2012-09-25 15:39:38]

I took suggestion from the presubmit message and added @zelidrag and @sky as
reviewers.

Zelidrag and Scott, could you review this CL? This is a refactoring CL for
bluetooth API and has been reviewed and LGTM'd by @bryeung and @keybuk.

[sky, 2012-09-25 16:16:56]

I'm not familiar with this code at all. Are you sure you want me to review it?

  -Scott

On Tue, Sep 25, 2012 at 8:39 AM,  <youngki@chromium.org> wrote:
> I took suggestion from the presubmit message and added @zelidrag and @sky as
> reviewers.
>
> Zelidrag and Scott, could you review this CL? This is a refactoring CL for
> bluetooth API and has been reviewed and LGTM'd by @bryeung and @keybuk.
>
> https://chromiumcodereview.appspot.com/10899037/

[youngki, 2012-09-25 17:15:05]

Scott,

I saw that you are listed chrome/OWNERS. So could you review the following files
only?

chrome/chrome_browser_chromeos.gypi
chrome/chrome_tests.gypi

Also, Zel, could you review the following files?

chrome/browser/chromeos/system/ash_system_tray_delegate.cc
chromeos/chromeos.gyp
chromeos/dbus/bluetooth_out_of_band_client.h
chromeos/dbus/bluetooth_out_of_band_client.cc
chromeos/dbus/bluetooth_out_of_band_pairing_data.h
chromeos/dbus/mock_bluetooth_out_of_band_client.h


On 2012/09/25 16:16:56, sky wrote:
> I'm not familiar with this code at all. Are you sure you want me to review it?
> 
>   -Scott
> 
> On Tue, Sep 25, 2012 at 8:39 AM,  <mailto:youngki@chromium.org> wrote:
> > I took suggestion from the presubmit message and added @zelidrag and @sky as
> > reviewers.
> >
> > Zelidrag and Scott, could you review this CL? This is a refactoring CL for
> > bluetooth API and has been reviewed and LGTM'd by @bryeung and @keybuk.
> >
> > https://chromiumcodereview.appspot.com/10899037/

[youngki, 2012-09-25 17:20:36]

Adding more reviewers @miket and @nkostylev to get all the approvals needed..

Mike, could you review the following files only?

chrome/browser/chromeos/extensions/bluetooth_event_router.cc
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc

Nikita, could you review the following files only?

chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc

[youngki, 2012-09-25 17:34:52]

Removing Zel from reviewer and adding @satorux to the reviewer list per Bryan's
suggestion.

Satoru, could you review the following files?

chrome/browser/chromeos/system/ash_system_tray_delegate.cc
chromeos/chromeos.gyp
chromeos/dbus/bluetooth_out_of_band_client.h
chromeos/dbus/bluetooth_out_of_band_client.cc
chromeos/dbus/bluetooth_out_of_band_pairing_data.h
chromeos/dbus/mock_bluetooth_out_of_band_client.h


Thanks

[miket_OOO, 2012-09-25 17:59:53]

Reviewed only these:

chrome/browser/chromeos/extensions/bluetooth_event_router.cc
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc

LGTM

[sky, 2012-09-25 18:28:27]

https://chromiumcodereview.appspot.com/10899037/diff/53001/chrome/chrome_brow...
File chrome/chrome_browser_chromeos.gypi (right):

https://chromiumcodereview.appspot.com/10899037/diff/53001/chrome/chrome_brow...
chrome/chrome_browser_chromeos.gypi:130:
'browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc',
Why do any of these files need chromeos in the name? They are in the chromeos
directory.

[youngki, 2012-09-25 18:45:03]

https://chromiumcodereview.appspot.com/10899037/diff/53001/chrome/chrome_brow...
File chrome/chrome_browser_chromeos.gypi (right):

https://chromiumcodereview.appspot.com/10899037/diff/53001/chrome/chrome_brow...
chrome/chrome_browser_chromeos.gypi:130:
'browser/chromeos/bluetooth/bluetooth_adapter_chromeos.cc',
On 2012/09/25 18:28:27, sky wrote:
> Why do any of these files need chromeos in the name? They are in the chromeos
> directory.

We are planning on adding bluetooth_adapter_windows.* (Bluetooth Adapter
implementation for Windows) later on. In order to do so we have to rename
bluetooth_adapter.* to bluetooth_adapter_chromeos.*, then move those files out
of chromeos directory (in the next CL), then add bluetooth_adapter_windows.*.

[sky, 2012-09-25 22:26:41]

LGTM

[youngki, 2012-09-26 16:19:54]

Thanks Mike and Scott for reviewing the CL.

Satoru/ could you review the CL for the files in chromeos and
chrome/browser/chromeos/system/ash_system_tray_delegate.cc?

Nikita/ could you review the CL for the following files?

chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.h
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc

@bryeung, @keybuk/ FYI I just updated the CL by renaming BluetoothAdapterTest as
BluetoothAdapterChromeOsTest, and also fixed
bluetooth_adapter_devices_unittest.cc.

[Nikita (slow), 2012-09-26 16:23:49]

lgtm

[DaveMoore, 2012-09-26 17:40:57]

lgtm

[youngki, 2012-09-26 18:14:41]

Thanks Dave for reviewing for chromeos ownership.

I think I've got all the necessary approvals. I will make sure the CL passes all
the try bots and then submit it.

[youngki, 2012-09-27 18:38:01]

There were try bot testing failures related to bluetooth_apitest_chromeos.cc and
the reason was that I was using mock methods without any
instructions/implementations. I fixed it by moving more method implementations
from the BluetoothAdapterChromeOs class to the BluetoothAdapter class and adding
CleanUpOnMainThread() on BluetoothApiTest.

I will run try bots again and if they pass, then I will submit this CL.

[keybuk, 2012-09-27 18:41:12]

WAIT!

This is a massive change to this CL!

Please don't submit again without a second review, at least from me

[youngki, 2012-09-27 18:41:55]

On 2012/09/27 18:41:12, keybuk wrote:
> WAIT!
> 
> This is a massive change to this CL!
> 
> Please don't submit again without a second review, at least from me

Okay I will wait.

[youngki, 2012-10-01 16:19:23]

Ping. Bryan and Scott, could you give LGTM if everything looks okay?

[sky, 2012-10-01 17:09:38]

chrome/*.gyp LGTM

[bryeung, 2012-10-01 17:13:31]

Did you get a looks good from keybuk?  He was the one who objected originally.

[youngki, 2012-10-01 17:43:28]

Scott, I am thinking if submitting this CL today; could you take a look and LGTM
today if everything looks okay?

[keybuk, 2012-10-01 18:22:43]

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:90: virtual const
std::string& address() const;
Since this is a trivial implementation, why not keep it inline?

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:93: virtual const
std::string& name() const;
As above.

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:129: virtual
ConstDeviceList GetDevices() const = 0;
I'm confused as to why GetDevices() is implemented here but the const version is
not.

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:167:
ASSERT_TRUE(adapter_chromeos != NULL);
Why would this ever be NULL?

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: virtual bool
IsVisible() const;
Can this not stay as an inline definition?

[kevers, 2012-10-01 18:47:41]

On 2012/10/01 18:22:43, keybuk wrote:
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):
> 
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:90: virtual const
> std::string& address() const;
> Since this is a trivial implementation, why not keep it inline?
> 
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:93: virtual const
> std::string& name() const;
> As above.
> 
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:129: virtual
> ConstDeviceList GetDevices() const = 0;
> I'm confused as to why GetDevices() is implemented here but the const version
is
> not.
> 
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:167:
> ASSERT_TRUE(adapter_chromeos != NULL);
> Why would this ever be NULL?
> 
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):
> 
>
https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
> chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: virtual bool
> IsVisible() const;
> Can this not stay as an inline definition?

According to the C++ style guide, virtual functions should never be inlined.

[youngki, 2012-10-01 18:51:40]

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:90: virtual const
std::string& address() const;
On 2012/10/01 18:22:44, keybuk wrote:
> Since this is a trivial implementation, why not keep it inline?

Done.

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:93: virtual const
std::string& name() const;
On 2012/10/01 18:22:44, keybuk wrote:
> As above.

Done.

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:129: virtual
ConstDeviceList GetDevices() const = 0;
On 2012/10/01 18:22:44, keybuk wrote:
> I'm confused as to why GetDevices() is implemented here but the const version
is
> not.

GetDevices() is basically re-creating what's returned from the const version in
non-const vectors. And the const version relies on platform-specific code.

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:167:
ASSERT_TRUE(adapter_chromeos != NULL);
On 2012/10/01 18:22:44, keybuk wrote:
> Why would this ever be NULL?

You had a concern in the previous comment (See patch 'E') that you want to add a
test if BluetoothAdapter* returned is in fact a BluetoothAdapterChromeOs and not
some other object. This will be null if BluetoothAdapter* returned is not
BluetoothAdapterChromeOs*.

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:154: virtual bool
IsVisible() const;
On 2012/10/01 18:22:44, keybuk wrote:
> Can this not stay as an inline definition?

Done.

[youngki, 2012-10-01 19:08:37]

I actually took Kevin's suggestion and reverted the recent patch because the
style guide does really say that virtual functions cannot be inline. PTAL

[keybuk, 2012-10-01 20:34:37]

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:90: virtual const
std::string& address() const;
as kevers replied, since this is virtual, style guide says do not inline - so
keep it as it was :)

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:167:
ASSERT_TRUE(adapter_chromeos != NULL);
No it won't, a static_cast always succeeds - you're thinking of dynamic_cast

[youngki, 2012-10-01 21:03:27]

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:90: virtual const
std::string& address() const;
On 2012/10/01 20:34:37, keybuk wrote:
> as kevers replied, since this is virtual, style guide says do not inline - so
> keep it as it was :)

Done.

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10899037/diff/55009/chrome/browser/chr...
chrome/browser/chromeos/bluetooth/bluetooth_adapter_chromeos_unittest.cc:167:
ASSERT_TRUE(adapter_chromeos != NULL);
On 2012/10/01 20:34:37, keybuk wrote:
> No it won't, a static_cast always succeeds - you're thinking of dynamic_cast

You are right; I meant to use dynamic_cast. And actually it seems dynamic_cast
is not allowed since it throws compile error saying dynamic_cast not permitted
with -fno-rtti.

I think it's okay to leave the test without this check.. I think if
adapter.get() is something other than BluetoothAdapterChromeOs, this test will
fail on the next line because only BluetoothAdapterChromeOs inherits
BluetoothAdapterClient. I removed ASSERT_TRUE from this test and also
bluetooth_adapter_device_chromeos_unittest.cc.

[keybuk, 2012-10-01 21:11:32]

lgtm

[youngki, 2012-10-01 21:20:05]

Thanks for all the reviews..! I will run try-bots one more time and submit the
CL.

[commit-bot: I haz the power, 2012-10-01 23:27:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/youngki@chromium.org/10899037/72011

[commit-bot: I haz the power, 2012-10-02 01:56:47]

Change committed as 159615