Moved bluetooth adapter files to device/bluetooth/.

Moved bluetooth adapter files from chrome/browser/chromeos/bluetooth/ to device/bluetooth/. device/bluetooth/ is a new directory to host the bluetooth related files.

I also changed the namespace from chromeos to bluetooth under new directory.

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

[youngki, 2012-10-07 23:33:02]

This is the CL to move the bluetooth adapter/device files to new directory:
device/bluetooth/.

Sorry about this large CL but I couldn't find a way to split them to smaller
ones as they have to move together.

I haven't removed #if defined(OS_CHROMEOS) from bluetooth_api.cc, but I will
make that change later in this CL or the next CL.

[keybuk, 2012-10-08 17:21:59]

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:14: namespace device_bluetooth {
this seems ugly; either

namespace bluetooth {

or

namespace device {
namespace bluetooth {

my pref is the first

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_devices_chromeos_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_devices_chromeos_unittest.cc:23: class
BluetoothAdapterDevicesChromeOsTest : public testing::Test {
I missed this before: this class should be BluetoothAdapterChromeOsDevicesTest

and the filename should be bluetooth_adapter_chromeos_devices_unittest.cc

Since we're renaming again, may as well get the name right

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_service_record_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_service_record_unittest.cc:10: #include
"chrome/common/chrome_paths.h"
You can't include chrome/ from device/

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_utils.h:13: namespace bluetooth_utils {
ugh,

namespace bluetooth {
namespace utils {

would be nicer

[youngki, 2012-10-09 14:37:53]

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:14: namespace device_bluetooth {
On 2012/10/08 17:21:59, keybuk wrote:
> this seems ugly; either
> 
> namespace bluetooth {
> 
> or
> 
> namespace device {
> namespace bluetooth {
> 
> my pref is the first

Done.

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_devices_chromeos_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_devices_chromeos_unittest.cc:23: class
BluetoothAdapterDevicesChromeOsTest : public testing::Test {
On 2012/10/08 17:21:59, keybuk wrote:
> I missed this before: this class should be BluetoothAdapterChromeOsDevicesTest
> 
> and the filename should be bluetooth_adapter_chromeos_devices_unittest.cc
> 
> Since we're renaming again, may as well get the name right

Done.

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_service_record_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_service_record_unittest.cc:10: #include
"chrome/common/chrome_paths.h"
On 2012/10/08 17:21:59, keybuk wrote:
> You can't include chrome/ from device/

This is necessary to set the path to test xml data correctly. The path is
registered by device/run_all_unittests.cc through ChromeTestSuite(). Maybe I
should make DeviceTestSuite() under device/ and move all xml data under device/,
but that might take even further refactoring to ChromeTestSuite. I could
certainly do it in this CL though if that's necessary, or I could do it in the
next CL.  At least I don't think there is any circular dependency in gyp files
currently.

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_utils.h:13: namespace bluetooth_utils {
On 2012/10/08 17:21:59, keybuk wrote:
> ugh,
> 
> namespace bluetooth {
> namespace utils {
> 
> would be nicer

Done.

[youngki, 2012-10-10 00:58:55]

I updated the CL by moving bluetooth_event_router out of chromeos directory and
removing ChromeOs dependency from bluetooth_api.* as much as possible.

I couldn't completely remove ChromeOs dependency from bluetooth_api.cc because
it depends on base/safe_strerror_posix.h. Please let me know if I could use any
other library.

For now I will use this CL to start working on the windows implementation.

[keybuk, 2012-10-10 01:16:32]

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_service_record_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_service_record_unittest.cc:10: #include
"chrome/common/chrome_paths.h"
it doesn't matter how necessary it is - it's illegal.

[youngki, 2012-10-10 14:44:33]

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_service_record_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_service_record_unittest.cc:10: #include
"chrome/common/chrome_paths.h"
On 2012/10/10 01:16:32, keybuk wrote:
> it doesn't matter how necessary it is - it's illegal.

Okay I will migrate the part of ChromeTestSuite into DeviceTestSuite. I will let
you know when that change is ready for review.

[bryeung, 2012-10-10 15:03:52]

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/DEPS...
chrome/browser/DEPS:8: "+device/bluetooth",
could this just be "+device"?

The other things we're going to put there (e.g. usb) will also be used by
chrome/browser.

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/chro...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/chro...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:222:
DCHECK(bluetooth_adapter_.get());
can we leave this CL as purely a refactoring for now?  (And add this in a
follow-up CL if necessary)

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:32: using
bluetooth::BluetoothAdapter;
I would also using bluetooth::BluetoothDevice here

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:40: const
bluetooth::BluetoothAdapter* GetAdapter(Profile* profile) {
you don't need both the using above and bluetooth:: prefix here.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:81: if
(GetAdapter(profile())) {
I'd prefer if adapter were assigned to a variable here, and the logic was
reversed (i.e. if (!adapter) return false).

Also, it would be nice to make a function to make this a bit more clear:

bool IsBluetoothSupported(Profile* profile) {
  return GetAdapter(profile()) != null;
}

That should make these early exit tests much more clear.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:139: if
(!GetAdapter(profile())) {
no braces

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:344: #if
defined(OS_CHROMEOS)
Is the rest of this function actually going to work on Windows?  Will we even
get a socket back on Windows?  I'd prefer for this to exit early with a "Not yet
implemented" error for the cases where it won't work.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:556:
BluetoothReadFunction::BluetoothReadFunction() {}
These should be moved to go with the rest of their classes now.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:23: using
bluetooth::BluetoothOutOfBandPairingData;
might as well add the mocks as well

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:120: const
bluetooth::BluetoothAdapter::ErrorCallback& error_callback) {
s/bluetooth:://

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:390:
bluetooth::BluetoothAdapter::ConstDeviceList devices;
s/bluetooth:://

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:425:
bluetooth::BluetoothAdapter::ConstDeviceList devices;
s/bluetooth:://

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:22: using
bluetooth::BluetoothSocket;
use all or none of the Bluetooth classes you're using.  None is probably better
here: it doesn't look that bad with the bluetooth:: prefixes.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:33: if (adapter_.get()) {
no braces

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:39: if (adapter_.get()) {
no braces

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.h:17: using
bluetooth::BluetoothSocket;
remove

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/extension_service.h:520: }
newline before #if

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/extension_service.h:853:
scoped_ptr<extensions::ExtensionBluetoothEventRouter> bluetooth_event_router_;
newline before #if

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

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/ui/w...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:23: using
bluetooth::BluetoothAdapter;
remove

https://chromiumcodereview.appspot.com/11075006/diff/7003/device/device.gyp
File device/device.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/device/device.gyp#n...
device/device.gyp:103: 'target_name': 'mock_bluetooth',
I'd prefer bluetooth_mocks

[youngki, 2012-10-10 18:34:56]

bluetooth_device.cc includes '#include "grit/generated_resources.h"' which
depends on '../chrome/chrome_resources.gyp:chrome_strings' in device.gyp. Is
this okay?

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_service_record_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_service_record_unittest.cc:10: #include
"chrome/common/chrome_paths.h"
On 2012/10/10 14:44:34, youngki wrote:
> On 2012/10/10 01:16:32, keybuk wrote:
> > it doesn't matter how necessary it is - it's illegal.
> 
> Okay I will migrate the part of ChromeTestSuite into DeviceTestSuite. I will
let
> you know when that change is ready for review.

DeviceTestSuite added and this include has been removed. Please take another
look.

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/DEPS...
chrome/browser/DEPS:8: "+device/bluetooth",
On 2012/10/10 15:03:53, bryeung wrote:
> could this just be "+device"?
> 
> The other things we're going to put there (e.g. usb) will also be used by
> chrome/browser.

Done.

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/chro...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/8002/chrome/browser/chro...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:222:
DCHECK(bluetooth_adapter_.get());
On 2012/10/10 15:03:53, bryeung wrote:
> can we leave this CL as purely a refactoring for now?  (And add this in a
> follow-up CL if necessary)

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:32: using
bluetooth::BluetoothAdapter;
On 2012/10/10 15:03:53, bryeung wrote:
> I would also using bluetooth::BluetoothDevice here

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:40: const
bluetooth::BluetoothAdapter* GetAdapter(Profile* profile) {
On 2012/10/10 15:03:53, bryeung wrote:
> you don't need both the using above and bluetooth:: prefix here.

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:81: if
(GetAdapter(profile())) {
On 2012/10/10 15:03:53, bryeung wrote:
> I'd prefer if adapter were assigned to a variable here, and the logic was
> reversed (i.e. if (!adapter) return false).
> 
> Also, it would be nice to make a function to make this a bit more clear:
> 
> bool IsBluetoothSupported(Profile* profile) {
>   return GetAdapter(profile()) != null;
> }
> 
> That should make these early exit tests much more clear.

Done. That looks much better.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:139: if
(!GetAdapter(profile())) {
On 2012/10/10 15:03:53, bryeung wrote:
> no braces

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:344: #if
defined(OS_CHROMEOS)
On 2012/10/10 15:03:53, bryeung wrote:
> Is the rest of this function actually going to work on Windows?  Will we even
> get a socket back on Windows?  I'd prefer for this to exit early with a "Not
yet
> implemented" error for the cases where it won't work.

I made an early return if socket_.get() is null. Also should I use LOG(WARNING)
<< "Not yet implemented"? I am not sure what's the logging policy for chromium;
right now I didn't put any log but let me know how I should log this.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:556:
BluetoothReadFunction::BluetoothReadFunction() {}
On 2012/10/10 15:03:53, bryeung wrote:
> These should be moved to go with the rest of their classes now.

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:23: using
bluetooth::BluetoothOutOfBandPairingData;
On 2012/10/10 15:03:53, bryeung wrote:
> might as well add the mocks as well

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:120: const
bluetooth::BluetoothAdapter::ErrorCallback& error_callback) {
On 2012/10/10 15:03:53, bryeung wrote:
> s/bluetooth:://

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:390:
bluetooth::BluetoothAdapter::ConstDeviceList devices;
On 2012/10/10 15:03:53, bryeung wrote:
> s/bluetooth:://

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:425:
bluetooth::BluetoothAdapter::ConstDeviceList devices;
On 2012/10/10 15:03:53, bryeung wrote:
> s/bluetooth:://

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:22: using
bluetooth::BluetoothSocket;
On 2012/10/10 15:03:53, bryeung wrote:
> use all or none of the Bluetooth classes you're using.  None is probably
better
> here: it doesn't look that bad with the bluetooth:: prefixes.

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:33: if (adapter_.get()) {
On 2012/10/10 15:03:53, bryeung wrote:
> no braces

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:39: if (adapter_.get()) {
On 2012/10/10 15:03:53, bryeung wrote:
> no braces

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.h:17: using
bluetooth::BluetoothSocket;
On 2012/10/10 15:03:53, bryeung wrote:
> remove

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/extension_service.h:520: }
On 2012/10/10 15:03:53, bryeung wrote:
> newline before #if

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/exte...
chrome/browser/extensions/extension_service.h:853:
scoped_ptr<extensions::ExtensionBluetoothEventRouter> bluetooth_event_router_;
On 2012/10/10 15:03:53, bryeung wrote:
> newline before #if

Done.

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

https://chromiumcodereview.appspot.com/11075006/diff/7003/chrome/browser/ui/w...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:23: using
bluetooth::BluetoothAdapter;
On 2012/10/10 15:03:53, bryeung wrote:
> remove

Done.

https://chromiumcodereview.appspot.com/11075006/diff/7003/device/device.gyp
File device/device.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/7003/device/device.gyp#n...
device/device.gyp:103: 'target_name': 'mock_bluetooth',
On 2012/10/10 15:03:53, bryeung wrote:
> I'd prefer bluetooth_mocks 

Done.

[bryeung, 2012-10-11 15:15:39]

This is looking good, except I'm not sure I know all of the rules for what can
depend on what.

Scott: do you know of a document that lays it out somewhere?

http://codereview.chromium.org/11075006/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

http://codereview.chromium.org/11075006/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:85: if
(!IsBluetoothSupported(profile()))
These should be calling SetError as well, with an error string of "This
operation is not supported on your platform".

I'll refactor this to clean it up a bit after this change lands (to help prevent
it from getting any bigger).  For now, just put the SetError with each early
exit.

http://codereview.chromium.org/11075006/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:247:
scoped_refptr<bluetooth::BluetoothSocket> socket) {
Should be using bluetooth::BluetoothSocket in this file as well.

http://codereview.chromium.org/11075006/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.h (right):

http://codereview.chromium.org/11075006/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/bluetooth/bluetooth_api.h:18: class
BluetoothSocket;
please remove this (it's not needed, nor was it before, but might as well clean
it up while things are getting moved)

http://codereview.chromium.org/11075006/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc
(right):

http://codereview.chromium.org/11075006/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:43: new
testing::StrictMock<MockBluetoothAdapter>(kAdapterAddress, kName);
why the wrapping change?

http://codereview.chromium.org/11075006/diff/14001/chromeos/dbus/DEPS
File chromeos/dbus/DEPS (right):

http://codereview.chromium.org/11075006/diff/14001/chromeos/dbus/DEPS#newcode3
chromeos/dbus/DEPS:3: "+device/bluetooth/bluetooth_out_of_band_pairing_data.h",
do any other deps files include a specific .h like this?  I think
"+device/bluetooth", is probably okay

http://codereview.chromium.org/11075006/diff/14001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/11075006/diff/14001/content/content_tests.gypi...
content/content_tests.gypi:16: '../media/media.gyp:media',
don't see how this is related to this CL

[youngki, 2012-10-11 16:21:18]

Scott, do you know the answer to the question I had in the previous review:

bluetooth_device.cc includes '#include "grit/generated_resources.h"' which
depends on '../chrome/chrome_resources.gyp:chrome_strings' in device.gyp. Is
this okay?

Or is there a documentation about the dependency rules that can answer this?

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

https://chromiumcodereview.appspot.com/11075006/diff/14001/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:85: if
(!IsBluetoothSupported(profile()))
On 2012/10/11 15:15:39, bryeung wrote:
> These should be calling SetError as well, with an error string of "This
> operation is not supported on your platform".
> 
> I'll refactor this to clean it up a bit after this change lands (to help
prevent
> it from getting any bigger).  For now, just put the SetError with each early
> exit.

Done.

https://chromiumcodereview.appspot.com/11075006/diff/14001/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:247:
scoped_refptr<bluetooth::BluetoothSocket> socket) {
On 2012/10/11 15:15:39, bryeung wrote:
> Should be using bluetooth::BluetoothSocket in this file as well.

Done.

https://chromiumcodereview.appspot.com/11075006/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/14001/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_api.h:18: class
BluetoothSocket;
On 2012/10/11 15:15:39, bryeung wrote:
> please remove this (it's not needed, nor was it before, but might as well
clean
> it up while things are getting moved)

BluetoothSocket is being used as a scoped_refptr in this file. So instead of
removing this, I removed '#include "device/bluetooth/bluetooth_socket.h"'
instead.

https://chromiumcodereview.appspot.com/11075006/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/11075006/diff/14001/chrome/browser/ext...
chrome/browser/extensions/api/bluetooth/bluetooth_apitest_chromeos.cc:43: new
testing::StrictMock<MockBluetoothAdapter>(kAdapterAddress, kName);
On 2012/10/11 15:15:39, bryeung wrote:
> why the wrapping change?

Done.

https://chromiumcodereview.appspot.com/11075006/diff/14001/chromeos/dbus/DEPS
File chromeos/dbus/DEPS (right):

https://chromiumcodereview.appspot.com/11075006/diff/14001/chromeos/dbus/DEPS...
chromeos/dbus/DEPS:3: "+device/bluetooth/bluetooth_out_of_band_pairing_data.h",
On 2012/10/11 15:15:39, bryeung wrote:
> do any other deps files include a specific .h like this?  I think
> "+device/bluetooth", is probably okay

At least content/DEPS includes specific .h files like this. Also I feel
uncomfortable including the entire device/bluetooth here because device/DEPS
already includes dbus directory.

https://chromiumcodereview.appspot.com/11075006/diff/14001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/11075006/diff/14001/content/content_te...
content/content_tests.gypi:16: '../media/media.gyp:media',
On 2012/10/11 15:15:39, bryeung wrote:
> don't see how this is related to this CL

content/public/test/content_test_suite_base.cc includes media/base/media.h but
this gyp file was missing that dependency. Without this this change won't build
because device_test_suite includes content_test_suite_base.h but device.gyp does
not depend on media.gyp. Also I think content.gyp should include media.gyp
anyways since content_test_suite_base.cc is using it.

[bryeung, 2012-10-11 17:00:00]

https://chromiumcodereview.appspot.com/11075006/diff/14001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/11075006/diff/14001/content/content_te...
content/content_tests.gypi:16: '../media/media.gyp:media',
On 2012/10/11 16:21:18, youngki wrote:
> On 2012/10/11 15:15:39, bryeung wrote:
> > don't see how this is related to this CL
> 
> content/public/test/content_test_suite_base.cc includes media/base/media.h but
> this gyp file was missing that dependency. Without this this change won't
build
> because device_test_suite includes content_test_suite_base.h but device.gyp
does
> not depend on media.gyp. Also I think content.gyp should include media.gyp
> anyways since content_test_suite_base.cc is using it.

In that case, I'd recommend taking this one line change into a separate CL and
sending it for review ASAP.

[youngki, 2012-10-11 17:28:52]

https://chromiumcodereview.appspot.com/11075006/diff/14001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/11075006/diff/14001/content/content_te...
content/content_tests.gypi:16: '../media/media.gyp:media',
On 2012/10/11 17:00:00, bryeung wrote:
> On 2012/10/11 16:21:18, youngki wrote:
> > On 2012/10/11 15:15:39, bryeung wrote:
> > > don't see how this is related to this CL
> > 
> > content/public/test/content_test_suite_base.cc includes media/base/media.h
but
> > this gyp file was missing that dependency. Without this this change won't
> build
> > because device_test_suite includes content_test_suite_base.h but device.gyp
> does
> > not depend on media.gyp. Also I think content.gyp should include media.gyp
> > anyways since content_test_suite_base.cc is using it.
> 
> In that case, I'd recommend taking this one line change into a separate CL and
> sending it for review ASAP.

Review sent as https://codereview.chromium.org/11066124/

[youngki, 2012-10-11 23:06:12]

I updated the CL with some fix to make this compile on Windows. This includes
changing uint8_t to uint8 because uint8_t is not supported in Windows, adding
more #if defined(OS_CHROMEOS) ... #endif, and removing
bluetooth_out_of_band_pairing_data target because it doesn't produce any lib or
obj due to lack of cc file (thus throws error on windows). Please take a look.

[keybuk, 2012-10-12 00:13:18]

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos.cc:26: namespace bluetooth {
should this be namespace bluetooth or namespace chromeos?

This is the only file you use "using" to avoid typing chromeos:: - either use
"using" or "chromeos::" everywhere

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos.h:21: namespace bluetooth {
should this namespace change? this is still chromeos specific code

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos_devices_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos_devices_unittest.cc:21: namespace
bluetooth {
right or wrong namespace?

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos_unittest.cc:20: namespace bluetooth
{
namespace?

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_device_chromeos.cc:38: namespace bluetooth {
namespace?

again pick either explicit chromeos:: or using everywhere - don't mix

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_device_chromeos.h:21: namespace bluetooth {
namespace?

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_out_of_band_pairing_data.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_out_of_band_pairing_data.h:22: uint8
randomizer[kBluetoothOutOfBandPairingDataSize];
is there a uint8 on non-Windows?  uint8_t is the C standard type

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_socket_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_socket_chromeos.cc:21: namespace bluetooth {
namespace?

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_socket_chromeos.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_socket_chromeos.h:14: namespace bluetooth {
namespace?

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_utils.h:24: #endif
Where is this called from?

[youngki, 2012-10-12 01:50:12]

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos.cc:26: namespace bluetooth {
On 2012/10/12 00:13:18, keybuk wrote:
> should this be namespace bluetooth or namespace chromeos?
> 
> This is the only file you use "using" to avoid typing chromeos:: - either use
> "using" or "chromeos::" everywhere

I changed bluetooth_adapter_chromeos, bluetooth_device_chromeos,
bluetooth_socket_chromeos, bluetooth_adapter_chromeos_unittest, and
bluetooth_adapter_chromeos_devices_unittest to have chromeos namespace instead
of bluetooth namespace.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos.h:21: namespace bluetooth {
On 2012/10/12 00:13:18, keybuk wrote:
> should this namespace change? this is still chromeos specific code

Done.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos_devices_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos_devices_unittest.cc:21: namespace
bluetooth {
On 2012/10/12 00:13:18, keybuk wrote:
> right or wrong namespace?

Done.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos_unittest.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos_unittest.cc:20: namespace bluetooth
{
On 2012/10/12 00:13:18, keybuk wrote:
> namespace?

Done.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_device_chromeos.cc:38: namespace bluetooth {
On 2012/10/12 00:13:18, keybuk wrote:
> namespace?
> 
> again pick either explicit chromeos:: or using everywhere - don't mix

Done.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_device_chromeos.h:21: namespace bluetooth {
On 2012/10/12 00:13:18, keybuk wrote:
> namespace?

Done.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_out_of_band_pairing_data.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_out_of_band_pairing_data.h:22: uint8
randomizer[kBluetoothOutOfBandPairingDataSize];
On 2012/10/12 00:13:18, keybuk wrote:
> is there a uint8 on non-Windows?  uint8_t is the C standard type

At least uint8 is defined in base/basictypes.h.

"uint8_t: undeclared identifier" is the error message I've got. And after quick
google search it looks like uint8_t is not supported in Windows.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_socket_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_socket_chromeos.cc:21: namespace bluetooth {
On 2012/10/12 00:13:18, keybuk wrote:
> namespace?

Done.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_socket_chromeos.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_socket_chromeos.h:14: namespace bluetooth {
On 2012/10/12 00:13:18, keybuk wrote:
> namespace?

Done.

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
File device/bluetooth/bluetooth_utils.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/19001/device/bluetooth/b...
device/bluetooth/bluetooth_utils.h:24: #endif
On 2012/10/12 00:13:18, keybuk wrote:
> Where is this called from?

Right now only bluetooth_socket_chromeos.cc and bluetooth_utils_unittest.cc are
calling it.

[youngki, 2012-10-12 02:14:48]

Also I marked most of BluetoothReadFunction::Work() and
BluetoothWriteFunction::Work() implementation in bluetooth_api.cc with "#if
defined(OS_CHROMEOS)" flag for now because they cause a lot of errors on
Windows. I will look into this in my other CL but for now let's mark those for
ChromeOs only.

[keybuk, 2012-10-12 03:40:12]

lgtm

[bryeung, 2012-10-12 14:05:51]

lgtm

https://chromiumcodereview.appspot.com/11075006/diff/10004/chrome/app/generat...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/11075006/diff/10004/chrome/app/generat...
chrome/app/generated_resources.grd:14502: Audio (<ph
name="ADDRESS">$1<ex>12:34:56:78:9A:BC</ex></ph>)
can you make sure this whitespace issue gets fixed in a follow-up CL?

[youngki, 2012-10-15 18:30:03]

Fixed try bot failures that this CL has caused. I am currently running the try
bots again, but I believe the current failures are not related to this CL.

https://chromiumcodereview.appspot.com/11075006/diff/10004/chrome/app/generat...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/11075006/diff/10004/chrome/app/generat...
chrome/app/generated_resources.grd:14502: Audio (<ph
name="ADDRESS">$1<ex>12:34:56:78:9A:BC</ex></ph>)
On 2012/10/12 14:05:51, bryeung wrote:
> can you make sure this whitespace issue gets fixed in a follow-up CL?

Done.

[darin (slow to review), 2012-10-17 20:06:54]

https://chromiumcodereview.appspot.com/11075006/diff/40001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/40001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter.h:14: namespace bluetooth {
perhaps the namespace should be "device" to match src/device?  see for example
src/content and src/net.  plus, your classes already have Bluetooth as a prefix,
so the bluetooth namespace seems redundant.

[youngki, 2012-10-17 21:30:30]

Thanks so much for the review Darin. I addressed your comment, PTAL.

https://chromiumcodereview.appspot.com/11075006/diff/40001/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/40001/device/bluetooth/b...
device/bluetooth/bluetooth_adapter.h:14: namespace bluetooth {
On 2012/10/17 20:06:54, darin wrote:
> perhaps the namespace should be "device" to match src/device?  see for example
> src/content and src/net.  plus, your classes already have Bluetooth as a
prefix,
> so the bluetooth namespace seems redundant.

Done.

[darin (slow to review), 2012-10-18 06:53:18]

would it make sense to have a bluetooth folder under test/data/?

https://chromiumcodereview.appspot.com/11075006/diff/51002/build/all.gyp
File build/all.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/51002/build/all.gyp#newc...
build/all.gyp:460: '../device/device.gyp:bluetooth_unit_tests',
sorry, one more nit.  i think this target should be named device_unittests.  use
"device" as the prefix to match the top-level directory name, and no "_" between
"unit" and "tests" to match other similar targets.

[darin (slow to review), 2012-10-18 06:55:58]

https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp
File device/device.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp#...
device/device.gyp:10: 'target_name': 'bluetooth',
it feels like we should create just a single 'device' library instead of
separate 'bluetooth', 'usb', and 'serial' libraries.  wdyt?

[bryeung, 2012-10-18 11:05:49]

https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp
File device/device.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp#...
device/device.gyp:10: 'target_name': 'bluetooth',
On 2012/10/18 06:55:58, darin wrote:
> it feels like we should create just a single 'device' library instead of
> separate 'bluetooth', 'usb', and 'serial' libraries.  wdyt?

Can you explain the motivation a bit more?

Personally, I think it's better to have the individual libraries so it is
possible to tell more precisely what something depends on.  e.g. ash probably
will only ever need the bluetooth portion from device/, and that would be clear
from the gyp file if we provide separate libraries.

[youngki, 2012-10-18 13:51:38]

On 2012/10/18 11:05:49, bryeung wrote:
> https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp
> File device/device.gyp (right):
> 
>
https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp#...
> device/device.gyp:10: 'target_name': 'bluetooth',
> On 2012/10/18 06:55:58, darin wrote:
> > it feels like we should create just a single 'device' library instead of
> > separate 'bluetooth', 'usb', and 'serial' libraries.  wdyt?
> 
> Can you explain the motivation a bit more?
> 
> Personally, I think it's better to have the individual libraries so it is
> possible to tell more precisely what something depends on.  e.g. ash probably
> will only ever need the bluetooth portion from device/, and that would be
clear
> from the gyp file if we provide separate libraries.

Darin, let me know what you think about Bryan's comment. I will wait until the
decision is made.

[youngki, 2012-10-18 14:04:34]

Moved all the bluetooth related test data into src/device/test/data/bluetooth/.
PTAL

https://chromiumcodereview.appspot.com/11075006/diff/51002/build/all.gyp
File build/all.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/51002/build/all.gyp#newc...
build/all.gyp:460: '../device/device.gyp:bluetooth_unit_tests',
On 2012/10/18 06:53:18, darin wrote:
> sorry, one more nit.  i think this target should be named device_unittests. 
use
> "device" as the prefix to match the top-level directory name, and no "_"
between
> "unit" and "tests" to match other similar targets.

Done.

https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp
File device/device.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp#...
device/device.gyp:10: 'target_name': 'bluetooth',
On 2012/10/18 11:05:49, bryeung wrote:
> On 2012/10/18 06:55:58, darin wrote:
> > it feels like we should create just a single 'device' library instead of
> > separate 'bluetooth', 'usb', and 'serial' libraries.  wdyt?
> 
> Can you explain the motivation a bit more?
> 
> Personally, I think it's better to have the individual libraries so it is
> possible to tell more precisely what something depends on.  e.g. ash probably
> will only ever need the bluetooth portion from device/, and that would be
clear
> from the gyp file if we provide separate libraries.

Darin, let me know what you think about Bryan's comment. I will wait until the
decision is made.

[darin (slow to review), 2012-10-18 16:59:53]

On Thu, Oct 18, 2012 at 4:05 AM, <bryeung@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/11075006/diff/**
>
51002/device/device.gyp<https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp>
> File device/device.gyp (right):
>
> https://chromiumcodereview.**appspot.com/11075006/diff/**
>
51002/device/device.gyp#**newcode10<https://chromiumcodereview.appspot.com/11075006/diff/51002/device/device.gyp#newcode10>
> device/device.gyp:10: 'target_name': 'bluetooth',
> On 2012/10/18 06:55:58, darin wrote:
>
>> it feels like we should create just a single 'device' library instead
>>
> of
>
>> separate 'bluetooth', 'usb', and 'serial' libraries.  wdyt?
>>
>
> Can you explain the motivation a bit more?
>
> Personally, I think it's better to have the individual libraries so it
> is possible to tell more precisely what something depends on.  e.g. ash
> probably will only ever need the bluetooth portion from device/, and
> that would be clear from the gyp file if we provide separate libraries.
>
>
OK.  Then, please use library names of the form
device_{bluetooth,serial,usb}.

Do you think we'll end up with a device_base library for common code?

-Darin




>
https://chromiumcodereview.**appspot.com/11075006/<https://chromiumcodereview...
>

[youngki, 2012-10-18 17:51:46]

Done. Renamed 'bluetooth' target as 'device_bluetooth'.

Right now all the code in device/bluetooth/ looks bluetooth-specific. Maybe
bluetooth_utils.{h|cc} might be reusable, but it's hard to tell right now. But
if there are codes to be reused, we will put them into 'device_common' target.

[darin (slow to review), 2012-10-18 17:55:17]

OK, LGTM

On Thu, Oct 18, 2012 at 10:51 AM, <youngki@chromium.org> wrote:

> Done. Renamed 'bluetooth' target as 'device_bluetooth'.
>
> Right now all the code in device/bluetooth/ looks bluetooth-specific. Maybe
> bluetooth_utils.{h|cc} might be reusable, but it's hard to tell right now.
> But
> if there are codes to be reused, we will put them into 'device_common'
> target.
>

I'd probably lean toward device_base.  That is a bit more canonical.  See
net/base
and ui/base.  The content/common/ directory is meant to hold code that runs
in
more than one child process.

-Darin




>
>
https://chromiumcodereview.**appspot.com/11075006/<https://chromiumcodereview...
>

[stevenjb, 2012-10-18 18:25:13]

OWNER lgtm for chrome/browser/chromeos/ and chromeos/

[youngki, 2012-10-18 18:29:44]

Thanks for the quick review Steven..! I appreciate it.

Now I think I got all the approvals needed. I will make sure all the try bots
pass and put this into commit queue.

[youngki, 2012-10-18 20:04:17]

Right now only win_aura is failing, but I believe that's not from this CL
because I ran the same try bot without my change and got the same failure.. So I
will go ahead and put this CL into commit queue.

[commit-bot: I haz the power, 2012-10-18 20:08:36]

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

[commit-bot: I haz the power, 2012-10-18 22:15:25]

Change committed as 162811

[darin (slow to review), 2012-10-18 23:32:52]

It'd also be nice to put a brief post on chromium-dev@ announcing the
addition of this new top-level directory.  Even though we don't always do
that, I think it would be nice to give folks a heads up.

-Darin



On Thu, Oct 18, 2012 at 3:15 PM, <commit-bot@chromium.org> wrote:

> Change committed as 162811
>
>
https://chromiumcodereview.**appspot.com/11075006/<https://chromiumcodereview...
>

[youngki, 2012-10-19 16:04:41]

Yep, I just sent out a note to chromium-dev:
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...

On Thu, Oct 18, 2012 at 7:32 PM, Darin Fisher <darin@chromium.org> wrote:

> It'd also be nice to put a brief post on chromium-dev@ announcing the
> addition of this new top-level directory.  Even though we don't always do
> that, I think it would be nice to give folks a heads up.
>
> -Darin
>
>
>
> On Thu, Oct 18, 2012 at 3:15 PM, <commit-bot@chromium.org> wrote:
>
>> Change committed as 162811
>>
>>
https://chromiumcodereview.**appspot.com/11075006/<https://chromiumcodereview...
>>
>
>

[tfarina, 2012-10-20 02:08:42]

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/bluetooth/b...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/bluetooth/b...
device/bluetooth/bluetooth_adapter_chromeos.h:35: class BluetoothAdapterChromeOs
This class should have probably be named BluetoothAdapterChromeOS,
it's Operating System, not Operating system ;)

Notice that the comment about writes 'Chrome OS'.

And the numbers don't lie:

$ g gs ChromeOS | wc -l
1001

$ g gs ChromeOs | wc -l
324

[tfarina, 2012-10-20 02:09:30]

On Fri, Oct 19, 2012 at 11:08 PM,  <tfarina@chromium.org> wrote:
> Notice that the comment about writes 'Chrome OS'.
>
s/about/above

-- 
Thiago

[tfarina, 2013-02-17 16:42:48]

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/device.gyp
File device/device.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/device.gyp#...
device/device.gyp:6: 'variables': {
why did you add this if you are not creating any variable?

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/device.gyp#...
device/device.gyp:13: '../chrome/chrome_resources.gyp:chrome_strings',
it would be cool to create your own resources file for this, instead of keep
depending on src/chrome/ for this.

Do you plan to get rid of this dep in future? May be file a bug for this?

[youngki, 2013-02-20 21:23:24]

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/device.gyp
File device/device.gyp (right):

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/device.gyp#...
device/device.gyp:6: 'variables': {
On 2013/02/17 16:42:49, tfarina wrote:
> why did you add this if you are not creating any variable?

http://code.google.com/searchframe#OAMlx_jo-ck/src/device/device.gyp&l=7

https://chromiumcodereview.appspot.com/11075006/diff/56003/device/device.gyp#...
device/device.gyp:13: '../chrome/chrome_resources.gyp:chrome_strings',
On 2013/02/17 16:42:49, tfarina wrote:
> it would be cool to create your own resources file for this, instead of keep
> depending on src/chrome/ for this.
> 
> Do you plan to get rid of this dep in future? May be file a bug for this?

created a bug: https://code.google.com/p/chromium/issues/detail?id=177286