Chrome OS: Attach bluetooth UI to device discovery API

Chrome OS: Attach bluetooth UI to device discovery API

BUG=chromium-os:22140
TEST=Visit system settings page, click "Find devices" button, observe UI and read chrome logs.

Change-Id: Idb0aa1650663f9c95d4c44194d6fa3b7e660d668


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

[Yufeng Shen (Slow to review), 2011-10-20 22:19:26]

http://codereview.chromium.org/8233042/diff/15001/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/bluetooth_list_element.js
(right):

http://codereview.chromium.org/8233042/diff/15001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:20: MOUSE:
'mouse',
can't apply cleanly
the , in the " MOUSE: 'mouse', " should not be there

[Vince Laviano, 2011-10-20 22:30:35]

http://codereview.chromium.org/8233042/diff/15001/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/bluetooth_list_element.js
(right):

http://codereview.chromium.org/8233042/diff/15001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:20: MOUSE:
'mouse',
On 2011/10/20 22:19:26, miletus wrote:
> can't apply cleanly
> the , in the " MOUSE: 'mouse', " should not be there

The , needs to be there to separate the MOUSE enum element from the following
PHONE enum element.

kevers@ made some changes to this code in ToT after my most recent rebase that
are probably causing a merge conflict when you try to apply this patch.

I'll upload a new patchset rebased to ToT.

[tfarina, 2011-10-23 02:15:43]

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:7: 
#pragma once

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:20: virtual const
std::string& address() const = 0;
please don't use unix_hacker style when naming pure virtual functions.

See http://www.chromium.org/developers/coding-style Inline functions

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:35: virtual const
DictionaryValue& dictionary_rep() const = 0;
please forward declare this instead of including base/values.h

[Vince Laviano, 2011-10-24 22:26:28]

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_device.h (right):

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:7: 
On 2011/10/23 02:15:44, tfarina wrote:
> #pragma once

Done.

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:20: virtual const
std::string& address() const = 0;
On 2011/10/23 02:15:44, tfarina wrote:
> please don't use unix_hacker style when naming pure virtual functions.
> 
> See http://www.chromium.org/developers/coding-style Inline functions

Done.

http://codereview.chromium.org/8233042/diff/27004/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_device.h:35: virtual const
DictionaryValue& dictionary_rep() const = 0;
On 2011/10/23 02:15:44, tfarina wrote:
> please forward declare this instead of including base/values.h

Done.

[Vince Laviano, 2011-10-25 00:29:02]

I split out the dbus layer code for separate review at
http://codereview.chromium.org/8375042/.

[Vince Laviano, 2011-10-26 01:59:43]

Next, splitting out the core bluetooth code for separate review at
http://codereview.chromium.org/8394030/

When that's in and we've rebased again, this CL should consist of UI layer
changes only.

[Vince Laviano, 2011-10-27 01:56:56]

Hi guys,

This CL hooks up the bluetooth_options_handler to the bluetooth device discovery
API that we've recently committed to Chrome (behind the --enable-bluetooth
flag).

The addition of COMPUTER and PHONE to the device type list in
bluetooth_list_element.js is temporary. It helps with demoing device discovery
since most discoverable devices fall into those categories.

Please take a look.

[kevers, 2011-10-27 16:14:43]

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:27:
VLOG(1) << "BluetoothOptionsHandler dtor";
Do we intend to keep this comment in the final release?  Some discussion on
various forums concerned with the amount of bloat that these message contribute
to the final product.  Should we use DVLOG instead, or add a TODO to remove once
further into the development cycle?

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:79:
VLOG(1) << "Initialize";
Message appears to be too generic.

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:131:
LOG(WARNING) << "unexpected default adapter change from \""
Seems to be a developing trend to migrate away from LOG(WARNING), replacing with
VLOG.  Consider mapping VLOG(1) -> VLOG(2) and LOG(WARNING) -> VLOG(1).

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:162:
LOG(INFO) << "DefaultAdapterChanged: no default bluetooth adapter";
VLOG(2)?

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:200: if
((default_adapter == NULL && !default_adapter_id_.empty()) ||
Lines 200-207 appear to duplicate lines 128-135.  Please create a method for
adapter validation.

[Vince Laviano, 2011-10-27 20:15:53]

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:27:
VLOG(1) << "BluetoothOptionsHandler dtor";
On 2011/10/27 16:14:43, kevers wrote:
> Do we intend to keep this comment in the final release?  Some discussion on
> various forums concerned with the amount of bloat that these message
contribute
> to the final product.  Should we use DVLOG instead, or add a TODO to remove
once
> further into the development cycle?

Removed.

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:79:
VLOG(1) << "Initialize";
On 2011/10/27 16:14:43, kevers wrote:
> Message appears to be too generic.

Removed.

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:131:
LOG(WARNING) << "unexpected default adapter change from \""
On 2011/10/27 16:14:43, kevers wrote:
> Seems to be a developing trend to migrate away from LOG(WARNING), replacing
with
> VLOG.  Consider mapping VLOG(1) -> VLOG(2) and LOG(WARNING) -> VLOG(1).

Done.

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:162:
LOG(INFO) << "DefaultAdapterChanged: no default bluetooth adapter";
On 2011/10/27 16:14:43, kevers wrote:
> VLOG(2)?

Done.

http://codereview.chromium.org/8233042/diff/41001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:200: if
((default_adapter == NULL && !default_adapter_id_.empty()) ||
On 2011/10/27 16:14:43, kevers wrote:
> Lines 200-207 appear to duplicate lines 128-135.  Please create a method for
> adapter validation.

Done.

[kevers, 2011-10-27 20:23:35]

lgtm

[commit-bot: I haz the power, 2011-10-27 20:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vlaviano@chromium.org/8233042/41002

[commit-bot: I haz the power, 2011-10-27 22:46:16]

Change committed as 107640