ash: Add a bluetooth entry in the uber tray.

ash: Add a bluetooth entry in the uber tray.

BUG=110130
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128002

[sadrul, 2012-03-20 21:40:24]

This simply prints out the list of devices on the console. But I wanted to put
it up for review to get early feedback on the way the BluetoothAdapter is being
used.

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:240:
BluetoothAdapter::DeviceList devices = bluetooth_adapter_->GetDevices();
Is 'GetDevice' an expensive operation? If it is, then I would optimize
NotifyRefreshBluetooth below to make sure the tray-item does not have to query
the list of all devices everytime. (I will probably do that anyway, since we get
enough information in the observer-callbacks, but I took the easiest path to get
something working).

[keybuk, 2012-03-20 21:59:58]

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:240:
BluetoothAdapter::DeviceList devices = bluetooth_adapter_->GetDevices();
It's expensive only in that it makes a vector of pointers for you.

It involves no round trips to the adapter, and only returns cached information.
I would not add a second-level cache of your own, this function is sufficient
for that :)

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:580: // Not necessary
to do anything here?
You may want to do cutesy flashing animations or something maybe, but no, in
general you don't need this and can just omit the function (the default
implementation is provided)

[sadrul, 2012-03-20 22:02:37]

Thanks for the quick review! I will update CL to actually do something useful,
and ping again fore review.

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:240:
BluetoothAdapter::DeviceList devices = bluetooth_adapter_->GetDevices();
On 2012/03/20 21:59:58, keybuk wrote:
> It's expensive only in that it makes a vector of pointers for you.
> 
> It involves no round trips to the adapter, and only returns cached
information.
> I would not add a second-level cache of your own, this function is sufficient
> for that :)

Excellent!

http://codereview.chromium.org/9753019/diff/2001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:580: // Not necessary
to do anything here?
On 2012/03/20 21:59:58, keybuk wrote:
> You may want to do cutesy flashing animations or something maybe, but no, in
> general you don't need this and can just omit the function (the default
> implementation is provided)

I will leave a TODO to cutesify :)

[sadrul, 2012-03-21 05:41:30]

Hi Ben (ash/, browser/ui/, ui):

This CL became larger than I had expected. It includes refactoring in a couple
of places:

 * Move FixedWidthImageView, ViewClickListener, HoverHighlightView out of
tray_network into tray_utils, so that these can be used in both tray_network and
tray_bluetooth (and tray_ime in the near future).

 * Refactor the functionality to add the '>' chevron in TrayItemMore, and use it
for network and bluetooth (and ime in the near future).

I can split this into smaller pieces if you would prefer that way.

keybuk@: Please review the changes in
chrome/browser/chromeos/system/ash_system_tray_delegate.cc

Left to do:
 * Show the list of devices in the popup. All the necessary plumbing is already
there, though. So this will be simple.
 * Show bluetooth settings page. This too should be simple.

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:71: void
BluetoothPowerFailure() {
Is it necessary to do anything here?

[keybuk, 2012-03-21 14:50:32]

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:137: virtual
~SystemTrayDelegate() {
Since you have a BluetoothAdapter instance and have registered yourself as an
observer, you need:

  bluetooth_adapter_->RemoveObserver(this)

here

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:377:
base::Bind(&BluetoothPowerFailure));
I couldn't see the definition of BluetoothPowerFailure anywhere

[sadrul, 2012-03-21 14:55:40]

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:137: virtual
~SystemTrayDelegate() {
On 2012/03/21 14:50:32, keybuk wrote:
> Since you have a BluetoothAdapter instance and have registered yourself as an
> observer, you need:
> 
>   bluetooth_adapter_->RemoveObserver(this)
> 
> here

I didn't do it since bluetooth_adapter_ gets destroyed here anyway. But I will
add.

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:377:
base::Bind(&BluetoothPowerFailure));
On 2012/03/21 14:50:32, keybuk wrote:
> I couldn't see the definition of BluetoothPowerFailure anywhere

It's in line 71.

[keybuk, 2012-03-21 14:58:57]

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:71: void
BluetoothPowerFailure() {
you should probably present an error to the user to tell them an action they
tried to perform failed

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:137: virtual
~SystemTrayDelegate() {
sure, but it's safer to do so, since the CreateDefaultAdapter() method might
change in implementation to return refptrs or similar later

[Ben Goodger (Google), 2012-03-21 15:52:13]

http://codereview.chromium.org/9753019/diff/9001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9753019/diff/9001/ash/shell.cc#newcode323
ash/shell.cc:323: virtual BluetoothDeviceList GetAvailableBluetoothDevices()
OVERRIDE {
what is this?

http://codereview.chromium.org/9753019/diff/9001/ash/system/bluetooth/tray_bl...
File ash/system/bluetooth/tray_bluetooth.cc (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/bluetooth/tray_bl...
ash/system/bluetooth/tray_bluetooth.cc:137: if (sender == header_) {
nit no braces

http://codereview.chromium.org/9753019/diff/9001/ash/system/bluetooth/tray_bl...
ash/system/bluetooth/tray_bluetooth.cc:143: } else {
que?

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/system_tray_...
File ash/system/tray/system_tray_delegate.h (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/system_tray_...
ash/system/tray/system_tray_delegate.h:123: virtual BluetoothDeviceList
GetAvailableBluetoothDevices() = 0;
virtual void GetAvailableBluetoothDevices(BluetoothDeviceList* devices) = 0;

?

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_item_mo...
File ash/system/tray/tray_item_more.cc (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_item_mo...
ash/system/tray/tray_item_more.cc:28: IDR_AURA_UBER_TRAY_MORE).ToSkBitmap());
weird indent

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_utils.h
File ash/system/tray/tray_utils.h (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_utils.h...
ash/system/tray/tray_utils.h:5: #ifndef ASH_SYSTEM_TRAY_TRAY_UTILS_H_
tray_views.h

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray_accessibility.h
File ash/system/tray_accessibility.h (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray_accessibilit...
ash/system/tray_accessibility.h:34: virtual bool InitialVisibility() OVERRIDE;
GetInitialVisibility

[sadrul, 2012-03-21 15:52:34]

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:71: void
BluetoothPowerFailure() {
On 2012/03/21 14:58:57, keybuk wrote:
> you should probably present an error to the user to tell them an action they
> tried to perform failed

I have added a TODO here. I will discuss with the UX folks no how to show the
error.

http://codereview.chromium.org/9753019/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:137: virtual
~SystemTrayDelegate() {
On 2012/03/21 14:58:57, keybuk wrote:
> sure, but it's safer to do so, since the CreateDefaultAdapter() method might
> change in implementation to return refptrs or similar later

Aha, good point! Done.

[sadrul, 2012-03-21 16:16:41]

Addressed all the comments. PTAL.

http://codereview.chromium.org/9753019/diff/9001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9753019/diff/9001/ash/shell.cc#newcode323
ash/shell.cc:323: virtual BluetoothDeviceList GetAvailableBluetoothDevices()
OVERRIDE {
On 2012/03/21 15:52:14, Ben Goodger (Google) wrote:
> what is this?

It would return the list of 'available' (currently connected, or previously
connected and remembered, as I understand it) bluetooth devices.

http://codereview.chromium.org/9753019/diff/9001/ash/system/bluetooth/tray_bl...
File ash/system/bluetooth/tray_bluetooth.cc (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/bluetooth/tray_bl...
ash/system/bluetooth/tray_bluetooth.cc:143: } else {
On 2012/03/21 15:52:14, Ben Goodger (Google) wrote:
> que?

Added a TODO here. This will be a block of code. So I left the braces in this
whole block.

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/system_tray_...
File ash/system/tray/system_tray_delegate.h (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/system_tray_...
ash/system/tray/system_tray_delegate.h:123: virtual BluetoothDeviceList
GetAvailableBluetoothDevices() = 0;
On 2012/03/21 15:52:14, Ben Goodger (Google) wrote:
> virtual void GetAvailableBluetoothDevices(BluetoothDeviceList* devices) = 0;
> 
> ?

Done. (also did so for IMEInfoList below)

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_item_mo...
File ash/system/tray/tray_item_more.cc (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_item_mo...
ash/system/tray/tray_item_more.cc:28: IDR_AURA_UBER_TRAY_MORE).ToSkBitmap());
On 2012/03/21 15:52:14, Ben Goodger (Google) wrote:
> weird indent

Fixed.

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_utils.h
File ash/system/tray/tray_utils.h (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray/tray_utils.h...
ash/system/tray/tray_utils.h:5: #ifndef ASH_SYSTEM_TRAY_TRAY_UTILS_H_
On 2012/03/21 15:52:14, Ben Goodger (Google) wrote:
> tray_views.h

Done.

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray_accessibility.h
File ash/system/tray_accessibility.h (right):

http://codereview.chromium.org/9753019/diff/9001/ash/system/tray_accessibilit...
ash/system/tray_accessibility.h:34: virtual bool InitialVisibility() OVERRIDE;
On 2012/03/21 15:52:14, Ben Goodger (Google) wrote:
> GetInitialVisibility

Done.

[Ben Goodger (Google), 2012-03-21 16:31:20]

ash/general views stuff LGTM

[sadrul, 2012-03-21 17:11:25]

@keybuk: I also fixed the 'Add device' callback to explicitly start
device-discovery. PTAL.

[keybuk, 2012-03-21 17:16:17]

lgtm