Add display of available bluetooth devices, and mechanism for retrieving the list.

Add display of available bluetooth devices, and mechanism for retrieving the list.

BUG=Partial fix for chromium-os:20991.
TEST=Launch chromeOS with the command-line argument --enable-bluetooth, and switch to the System setttings page.  Press the 'Find devices' button.  A list of bluetooth devices appears.  Currently, the list of devices is faked.


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

[kevers, 2011-10-04 16:51:51]

Hi James,

Can you please review this CL.  The purpose of the change is to add a mechanism
for retrieving a list of Bluetooth devices and to populate the list in the
settings page for ChromeOS.

[James Hawkins, 2011-10-04 20:43:37]

http://codereview.chromium.org/8137003/diff/6003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:11181: Scanning...
This should likely not have ellipses after it.  Use a throbber instead.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/bluetooth_list_element.js
(right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:6: 
Remove blank line.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:18: HEADSET:
'headset',
Indentation should be 2 spaces.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:59: for (var
i = 0; i < devices.length; ++i) {
s/++i/i++/

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:67: * @param
{Object.<string,string>} device.  Description of the bluetooth
No period, one space between parameter name and start of description.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:85: var
supported = false;
Remove var.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:89:
supported = true;
return true;

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:93: return
supported;
return false;

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:134: //
textDiv holds icon, name and status text.
|textDiv|

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/system_options.html (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.html:66: <div id =
"bluetooth-finder-div">
s/div/container/

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.html:67: <button
id="bluetooth-find-devices" i18n-content=
Hmm, I think we don't wrap mid-attribute.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:50:
$('bluetooth-find-devices').disabled = $('enable-bluetooth-label') ?
Wrap after '='.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:108:
$('bluetooth-scanning-icon').hidden = true;
Are these hides animated?

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:117:
console.log('Scanning for bluetooth devices.');
Remove console spam.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:131:
chrome.send('findBluetoothDevices', []);
No need for final (empty) parameter.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:140: {
Opening brace goes on the same line as the method.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.h (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.h:40: //    
devices according to the System options page.
This indentation is inconsistent with the doc one method up.

[Vince Laviano, 2011-10-04 22:03:13]

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:178:
"bluetoothDeviceNotPaired");
There's a distinction between paired and connected. "Paired" means that an
association has been formed (i.e., that the device is known to us), while
"connected" means that the device is currently connected. It's possible for a
paired device to be unconnected. It might be out of range, or the owner might
have paired several devices of the same class for different scenarios and
currently be using a different one.

I think that there are additional device properties that will be of interest to
the UI. It might make sense to have the bluetooth code pass either a Device
object that it defines or a DictionaryValue full of device properties to this
callback rather than having an arg for each property. This would allow the API
to remain stable as the set of interesting properties changes.

[kevers, 2011-10-05 14:23:01]

http://codereview.chromium.org/8137003/diff/6003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:11181: Scanning...
On 2011/10/04 20:43:39, James Hawkins wrote:
> This should likely not have ellipses after it.  Use a throbber instead.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/bluetooth_list_element.js
(right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:6: 
On 2011/10/04 20:43:39, James Hawkins wrote:
> Remove blank line.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:18: HEADSET:
'headset',
On 2011/10/04 20:43:39, James Hawkins wrote:
> Indentation should be 2 spaces.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:59: for (var
i = 0; i < devices.length; ++i) {
On 2011/10/04 20:43:39, James Hawkins wrote:
> s/++i/i++/

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:67: * @param
{Object.<string,string>} device.  Description of the bluetooth
On 2011/10/04 20:43:39, James Hawkins wrote:
> No period, one space between parameter name and start of description.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:85: var
supported = false;
On 2011/10/04 20:43:39, James Hawkins wrote:
> Remove var.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:89:
supported = true;
On 2011/10/04 20:43:39, James Hawkins wrote:
> return true;

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:93: return
supported;
On 2011/10/04 20:43:39, James Hawkins wrote:
> return false;

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:134: //
textDiv holds icon, name and status text.
On 2011/10/04 20:43:39, James Hawkins wrote:
> |textDiv|

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/system_options.html (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.html:66: <div id =
"bluetooth-finder-div">
On 2011/10/04 20:43:39, James Hawkins wrote:
> s/div/container/

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.html:67: <button
id="bluetooth-find-devices" i18n-content=
On 2011/10/04 20:43:39, James Hawkins wrote:
> Hmm, I think we don't wrap mid-attribute.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:50:
$('bluetooth-find-devices').disabled = $('enable-bluetooth-label') ?
On 2011/10/04 20:43:39, James Hawkins wrote:
> Wrap after '='.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:108:
$('bluetooth-scanning-icon').hidden = true;
On 2011/10/04 20:43:39, James Hawkins wrote:
> Are these hides animated?

No, the transition is not animated.  Is there a suggested transition duration?

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:117:
console.log('Scanning for bluetooth devices.');
On 2011/10/04 20:43:39, James Hawkins wrote:
> Remove console spam.

Oops.  My bad.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:131:
chrome.send('findBluetoothDevices', []);
On 2011/10/04 20:43:39, James Hawkins wrote:
> No need for final (empty) parameter.

Done.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:140: {
On 2011/10/04 20:43:39, James Hawkins wrote:
> Opening brace goes on the same line as the method.

Exceeded 80 character line limit with the opening brace on the same line.  Is
linebreaking before the argument preferable in this case?

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:178:
"bluetoothDeviceNotPaired");
On 2011/10/04 22:03:14, Vince Laviano wrote:
> There's a distinction between paired and connected. "Paired" means that an
> association has been formed (i.e., that the device is known to us), while
> "connected" means that the device is currently connected. It's possible for a
> paired device to be unconnected. It might be out of range, or the owner might
> have paired several devices of the same class for different scenarios and
> currently be using a different one.
> 
> I think that there are additional device properties that will be of interest
to
> the UI. It might make sense to have the bluetooth code pass either a Device
> object that it defines or a DictionaryValue full of device properties to this
> callback rather than having an arg for each property. This would allow the API
> to remain stable as the set of interesting properties changes.

Agreed that passing in a dictionary is more flexible.  Got sloppy with the
generation of a fake list for UI testing.

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.h (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.h:40: //    
devices according to the System options page.
On 2011/10/04 20:43:39, James Hawkins wrote:
> This indentation is inconsistent with the doc one method up.

Done.

[kevers, 2011-10-05 20:32:39]

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:108:
$('bluetooth-scanning-icon').hidden = true;
On 2011/10/04 20:43:39, James Hawkins wrote:
> Are these hides animated?

The call to setTimeout is a temporary visual hack (for story-boarding) that will
be removed once the response is asynchronous.

[James Hawkins, 2011-10-05 20:35:28]

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:108:
$('bluetooth-scanning-icon').hidden = true;
On 2011/10/05 14:23:01, kevers wrote:
> On 2011/10/04 20:43:39, James Hawkins wrote:
> > Are these hides animated?
> 
> No, the transition is not animated.  Is there a suggested transition duration?

Yes, hides should be animated...I'm not sure about the suggested transition
duration. You can check the css files in resources for examples.

[kevers, 2011-10-06 14:51:25]

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8137003/diff/6003/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/system_options.js:108:
$('bluetooth-scanning-icon').hidden = true;
On 2011/10/05 20:35:28, James Hawkins wrote:
> On 2011/10/05 14:23:01, kevers wrote:
> > On 2011/10/04 20:43:39, James Hawkins wrote:
> > > Are these hides animated?
> > 
> > No, the transition is not animated.  Is there a suggested transition
duration?
> 
> Yes, hides should be animated...I'm not sure about the suggested transition
> duration. You can check the css files in resources for examples.

The hide now has a 0.25s fade animation.

[James Hawkins, 2011-10-06 17:20:45]

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

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:52: *
devices that are currently connected.  New devices are discovered
One space between sentences.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:80: *
@return {boolean} True if the device is supported.
s/True/true/

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:95:
SystemOptions.AddBluetoothDevice = function(device) {
Public methods should be at the bottom.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:95:
SystemOptions.AddBluetoothDevice = function(device) {
s/Add/add/

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:103:
SystemOptions.NotifyBluetoothSearchComplete = function() {
s/Notify/notify/

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:155:
el.removeEventListener('webkitTransitionEnd', f);
Can't this all be done in CSS?

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.h (right):

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_options_handler.h:49:
DISALLOW_COPY_AND_ASSIGN(SystemOptionsHandler);
DISALLOW..() goes at the bottom.

[kevers, 2011-10-06 20:24:41]

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

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:52: *
devices that are currently connected.  New devices are discovered
On 2011/10/06 17:20:45, James Hawkins wrote:
> One space between sentences.

Done.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:80: *
@return {boolean} True if the device is supported.
On 2011/10/06 17:20:45, James Hawkins wrote:
> s/True/true/

Done.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:95:
SystemOptions.AddBluetoothDevice = function(device) {
On 2011/10/06 17:20:45, James Hawkins wrote:
> Public methods should be at the bottom.

Done.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:95:
SystemOptions.AddBluetoothDevice = function(device) {
On 2011/10/06 17:20:45, James Hawkins wrote:
> s/Add/add/

Done.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:103:
SystemOptions.NotifyBluetoothSearchComplete = function() {
On 2011/10/06 17:20:45, James Hawkins wrote:
> s/Notify/notify/

Done.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:155:
el.removeEventListener('webkitTransitionEnd', f);
On 2011/10/06 17:20:45, James Hawkins wrote:
> Can't this all be done in CSS?

Can animate opacity but not the 'hidden' attribute.  In a previous review, I was
asked to use hidden to show/hide the content.  In this case, the logic could be
simplified by dropping the hidden attribute and relying solely on the addition
and removable the 'transparent' class.

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.h (right):

http://codereview.chromium.org/8137003/diff/13001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_options_handler.h:49:
DISALLOW_COPY_AND_ASSIGN(SystemOptionsHandler);
On 2011/10/06 17:20:45, James Hawkins wrote:
> DISALLOW..() goes at the bottom.

Done.

[James Hawkins, 2011-10-06 20:50:16]

LGTM with nits.

http://codereview.chromium.org/8137003/diff/16007/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8137003/diff/16007/chrome/app/generated_resour...
chrome/app/generated_resources.grd:11256: <message
name="IDS_OPTIONS_SETTINGS_BLUETOOTH_CONNECTED">
Indentation off by one here.

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

http://codereview.chromium.org/8137003/diff/16007/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:26: *
Remove blank line.

[kevers, 2011-10-07 20:37:32]

Hi Rick

As per your suggestion, I have removed the addition of an animated spinner icon
in favor of using an existing throbber class in CSS.  Can you please have a look
at the change.

http://codereview.chromium.org/8137003/diff/16007/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8137003/diff/16007/chrome/app/generated_resour...
chrome/app/generated_resources.grd:11256: <message
name="IDS_OPTIONS_SETTINGS_BLUETOOTH_CONNECTED">
On 2011/10/06 20:50:16, James Hawkins wrote:
> Indentation off by one here.

Done.

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

http://codereview.chromium.org/8137003/diff/16007/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/bluetooth_list_element.js:26: *
On 2011/10/06 20:50:16, James Hawkins wrote:
> Remove blank line.

Done.

[Rick Byers, 2011-10-07 20:54:21]

On 2011/10/07 20:37:32, kevers wrote:
> Hi Rick
> 
> As per your suggestion, I have removed the addition of an animated spinner
icon
> in favor of using an existing throbber class in CSS.  Can you please have a
look
> at the change.

Thanks Kevin!  It seems like there are way too many different spinners and
throbbers in our code already, nice not to add another <grin>.  The delta LGTM.

[commit-bot: I haz the power, 2011-10-11 15:30:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/8137003/24001

[commit-bot: I haz the power, 2011-10-11 18:12:25]

Change committed as 104915