Add bluetooth device type icons on MD system tray.

Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam

This CL also shows them on bluetooth device list on MD ash tray,
and makes the Bluetooth toggle button functional.

BUG=632128
TEST=manually tested with the flag --ash-md=experimental

Committed: https://crrev.com/0e63d97a3cae0c52121dc15c87d68337ba585d1d
Cr-Commit-Position: refs/heads/master@{#424371}

[fukino, 2016-09-29 16:24:24]

Description was changed from

==========
Add bluetooth device type icons on MD system tray.

BUG=632128
==========

to

==========
Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam
This CL also shows them on bluetooth device list on MD ash tray, and makes the
Bluetooth toggle button functional. 

BUG=632128
TEST=manually tested with the flag --ash-md=experimental
==========

[fukino, 2016-09-29 16:24:44]

Description was changed from

==========
Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam
This CL also shows them on bluetooth device list on MD ash tray, and makes the
Bluetooth toggle button functional. 

BUG=632128
TEST=manually tested with the flag --ash-md=experimental
==========

to

==========
Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam

This CL also shows them on bluetooth device list on MD ash tray,
and makes the Bluetooth toggle button functional. 

BUG=632128
TEST=manually tested with the flag --ash-md=experimental
==========

[fukino, 2016-09-29 16:27:35]

fukino@chromium.org changed reviewers:
+ tdanderson@chromium.org

[fukino, 2016-09-29 16:27:36]

Hi tdanderson@,
I'm sorry for a belated update. Could you take a look?

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:316: HoverHighlightView*
container = new HoverHighlightView(this);
This implementation to place icons/labels/etc... is temporary.
I'll update the implementation in a separate CL to match the design spec.

https://codereview.chromium.org/2381493005/diff/1/ash/resources/vector_icons/...
File ash/resources/vector_icons/system_menu_computer.icon (right):

https://codereview.chromium.org/2381493005/diff/1/ash/resources/vector_icons/...
ash/resources/vector_icons/system_menu_computer.icon:6: MOVE_TO, 32, 29,
I created icon files by 
1) Download SVGs from Seb's folder.
2) Optimize SVGs https://jakearchibald.github.io/svgomg/ using default options.
3) Convert them to *.icon using http://evanstade.github.io/skiafy/.
4) Manually remove the first path in *.icon files. The path is like as follows.
It looks unnecessary and I had to remove it to show the icons properly.

MOVE_TO, 0, 0,
R_H_LINE_TO, 40,
R_V_LINE_TO, 40,
H_LINE_TO, 0,
CLOSE,

I'm not confident with step 4. Is there something wrong?

[tdanderson, 2016-09-29 18:54:40]

Thanks, LGTM with the comments below.

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/common/system/ch...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/common/system/ch...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:29: #include
"ui/gfx/vector_icons/vector_icons.h"
is this #include needed?

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/common/system/ch...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:239: if
(toggle_bluetooth_)
nit: else if (since you'll never have both visible)?

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/common/system/ch...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:290:
device_map_[container] = list[i].address;
nit: factor out lines 290 and 294 to after the if-else (perhaps use HHV*
container = md ? ... : ...;

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/common/system/ch...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:299: HoverHighlightView*
AddScrollListItem(const base::string16& text,
As a side note, wherever it makes sense to do so, please add a "TODO: Remove
this code when material design is enabled by default. See crbug.com/614453" so
we don't leave any dead code behind.

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/common/system/ch...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:316: HoverHighlightView*
container = new HoverHighlightView(this);
On 2016/09/29 16:27:36, fukino wrote:
> This implementation to place icons/labels/etc... is temporary.
> I'll update the implementation in a separate CL to match the design spec.

Acknowledged. Whatever you do here is probably what we're going to use for the
rows across all other detailed views, and likely also for default views. Please
be sure to send those code reviews to me so we can look for opportunities to
share code.

The spec calls for a material design ripple to be added to these rows. I
recently introduced the SystemMenuButton class which defines an MD button with
hover and ripple. That may be useful to look at, but you may have to do
something slightly different if the items in the scroll list do not derive from
CustomButton (in particular, whatever class you use will have to derive from
InkDropHostView). If you have specific questions about the MD ripple, feel free
to reach out to bruthig@.

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/resources/vector...
File ash/resources/vector_icons/system_menu_computer.icon (right):

https://chromiumcodereview.appspot.com/2381493005/diff/1/ash/resources/vector...
ash/resources/vector_icons/system_menu_computer.icon:6: MOVE_TO, 32, 29,
On 2016/09/29 16:27:36, fukino wrote:
> I created icon files by 
> 1) Download SVGs from Seb's folder.
> 2) Optimize SVGs https://jakearchibald.github.io/svgomg/ using default
options.
> 3) Convert them to *.icon using http://evanstade.github.io/skiafy/.
> 4) Manually remove the first path in *.icon files. The path is like as
follows.
> It looks unnecessary and I had to remove it to show the icons properly.
> 
> MOVE_TO, 0, 0,
> R_H_LINE_TO, 40,
> R_V_LINE_TO, 40,
> H_LINE_TO, 0,
> CLOSE,
> 
> I'm not confident with step 4. Is there something wrong?

Yes, step 4 is correct - without it you probably noticed that the colors were
inverted. See "Why is the outside of my icon being filled instead of the
inside?" in
https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui

https://chromiumcodereview.appspot.com/2381493005/diff/1/chrome/browser/ui/as...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://chromiumcodereview.appspot.com/2381493005/diff/1/chrome/browser/ui/as...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:173: switch (device_type)
{
Doesn't the compiler complain that you're not handling of the BluetoothDevice
enum values in this switch?

https://chromiumcodereview.appspot.com/2381493005/diff/1/chrome/browser/ui/as...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:189: case
device::BluetoothDevice::DEVICE_TABLET:
Are you sure you want to use the mouse icon for tablet (or are you adding the
tablet icon in a follow-up CL)?

[fukino, 2016-09-30 02:43:49]

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:29: #include
"ui/gfx/vector_icons/vector_icons.h"
On 2016/09/29 18:54:39, tdanderson wrote:
> is this #include needed?

Removed. Thanks!

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:239: if
(toggle_bluetooth_)
On 2016/09/29 18:54:39, tdanderson wrote:
> nit: else if (since you'll never have both visible)?

Done.

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:290:
device_map_[container] = list[i].address;
On 2016/09/29 18:54:39, tdanderson wrote:
> nit: factor out lines 290 and 294 to after the if-else (perhaps use HHV*
> container = md ? ... : ...;

Done. I used "md ? ... : ...";

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:299: HoverHighlightView*
AddScrollListItem(const base::string16& text,
On 2016/09/29 18:54:39, tdanderson wrote:
> As a side note, wherever it makes sense to do so, please add a "TODO: Remove
> this code when material design is enabled by default. See crbug.com/614453" so
> we don't leave any dead code behind.

Done.

https://codereview.chromium.org/2381493005/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:316: HoverHighlightView*
container = new HoverHighlightView(this);
On 2016/09/29 18:54:39, tdanderson wrote:
> On 2016/09/29 16:27:36, fukino wrote:
> > This implementation to place icons/labels/etc... is temporary.
> > I'll update the implementation in a separate CL to match the design spec.
> 
> Acknowledged. Whatever you do here is probably what we're going to use for the
> rows across all other detailed views, and likely also for default views.
Please
> be sure to send those code reviews to me so we can look for opportunities to
> share code.
> 
> The spec calls for a material design ripple to be added to these rows. I
> recently introduced the SystemMenuButton class which defines an MD button with
> hover and ripple. That may be useful to look at, but you may have to do
> something slightly different if the items in the scroll list do not derive
from
> CustomButton (in particular, whatever class you use will have to derive from
> InkDropHostView). If you have specific questions about the MD ripple, feel
free
> to reach out to bruthig@.

Thanks. I'll update this design next, and I'll make sure to send the reviews to
you.

https://codereview.chromium.org/2381493005/diff/1/ash/resources/vector_icons/...
File ash/resources/vector_icons/system_menu_computer.icon (right):

https://codereview.chromium.org/2381493005/diff/1/ash/resources/vector_icons/...
ash/resources/vector_icons/system_menu_computer.icon:6: MOVE_TO, 32, 29,
On 2016/09/29 18:54:39, tdanderson wrote:
> On 2016/09/29 16:27:36, fukino wrote:
> > I created icon files by 
> > 1) Download SVGs from Seb's folder.
> > 2) Optimize SVGs https://jakearchibald.github.io/svgomg/ using default
> options.
> > 3) Convert them to *.icon using http://evanstade.github.io/skiafy/.
> > 4) Manually remove the first path in *.icon files. The path is like as
> follows.
> > It looks unnecessary and I had to remove it to show the icons properly.
> > 
> > MOVE_TO, 0, 0,
> > R_H_LINE_TO, 40,
> > R_V_LINE_TO, 40,
> > H_LINE_TO, 0,
> > CLOSE,
> > 
> > I'm not confident with step 4. Is there something wrong?
> 
> Yes, step 4 is correct - without it you probably noticed that the colors were
> inverted. See "Why is the outside of my icon being filled instead of the
> inside?" in
>
https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui

Thank you for the information.
I didn't noticed that it was already documented. Sorry!

https://codereview.chromium.org/2381493005/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2381493005/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:173: switch (device_type)
{
On 2016/09/29 18:54:40, tdanderson wrote:
> Doesn't the compiler complain that you're not handling of the BluetoothDevice
> enum values in this switch?

I didn't see any errors.
I assume device::BluetoothDevice::DEVICE_* as the BluetoothDevice enum values.
Do I misunderstand something...?

https://codereview.chromium.org/2381493005/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:189: case
device::BluetoothDevice::DEVICE_TABLET:
On 2016/09/29 18:54:40, tdanderson wrote:
> Are you sure you want to use the mouse icon for tablet (or are you adding the
> tablet icon in a follow-up CL)?

Oh, there was a mistake on the spec.
https://docs.google.com/spreadsheets/d/1Hkpv9T27Veo4OVooAhVkfo5jJ6zMt5nEbvyJW...
I added the tablet icon.
Thank you for pointing this out!

[fukino, 2016-09-30 02:44:48]

fukino@chromium.org changed reviewers:
+ oshima@chromium.org

[fukino, 2016-09-30 02:44:48]

oshima san, could you take a look at
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc ?

[oshima, 2016-09-30 04:37:22]

please address my comment, then lgtm

https://codereview.chromium.org/2381493005/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2381493005/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:194: return
ash::kSystemMenuBluetoothIcon;
instead of returning in defualt, return outside of switch.

handle UNKNOWN, with error/warning message.

[fukino, 2016-09-30 06:51:33]

Thank you!

https://codereview.chromium.org/2381493005/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2381493005/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:194: return
ash::kSystemMenuBluetoothIcon;
On 2016/09/30 04:37:22, oshima wrote:
> instead of returning in defualt, return outside of switch.
> 
> handle UNKNOWN, with error/warning message.

Done.
Handled with a warning message.

[fukino, 2016-09-30 06:51:58]

The CQ bit was checked by fukino@chromium.org

[fukino, 2016-09-30 06:51:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2381493005/#ps40001
(title: "Handle UNKNOWN_DEVICE, and return default BT icon outside switch
statement.")

[commit-bot: I haz the power, 2016-09-30 06:52:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 06:56:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 06:56:34]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fukino, 2016-09-30 07:39:14]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 07:39:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 08:12:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 08:12:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fukino, 2016-09-30 14:31:32]

I got a link error for ash::kSystemMenuComputerIcon etc...

I think we should refer these icons in /ash/.../tray_bluetooth.cc to have all
design information in files under /ash.
To do so, ash::BluetoothDeviceInfo should have device type instead of the actual
image in 
https://codereview.chromium.org/2381493005/diff/60001/ash/common/system/tray/...

However, ash can be built in non-ChromeOS environment, so we can not directly
refer to device::BluetoothDevice::DeviceType in system_tray_delegate.h as
//device/bluetooth is build only on ChromeOS.

What should I do to fix this situation?
One possible solution might be that BluetoothDeviceInfo has its own enum for
device type (instead of //device/bluetooth's enum), like I did in
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
But I guess there are better solutions?

[tdanderson, 2016-09-30 17:27:01]

On 2016/09/30 14:31:32, fukino wrote:
> I got a link error for ash::kSystemMenuComputerIcon etc...
> 
> I think we should refer these icons in /ash/.../tray_bluetooth.cc to have all
> design information in files under /ash.
> To do so, ash::BluetoothDeviceInfo should have device type instead of the
actual
> image in 
>
https://codereview.chromium.org/2381493005/diff/60001/ash/common/system/tray/...
> 

+1, I like the idea of storing the device type instead of the image itself.

> However, ash can be built in non-ChromeOS environment, so we can not directly
> refer to device::BluetoothDevice::DeviceType in system_tray_delegate.h as
> //device/bluetooth is build only on ChromeOS.
> 
> What should I do to fix this situation?
> One possible solution might be that BluetoothDeviceInfo has its own enum for
> device type (instead of //device/bluetooth's enum), like I did in
>
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
> But I guess there are better solutions?

Instead of having multiple enums that are essentially identical, is there
somewhere
common where you can move the enum so that we only need to have one of them?
(base/ seems like overkill, maybe oshima@ can suggest something better?)

Failing any other ideas, I'd be ok with your proposal.

[fukino, 2016-10-04 12:28:54]

On 2016/09/30 17:27:01, tdanderson wrote:
> On 2016/09/30 14:31:32, fukino wrote:
> > I got a link error for ash::kSystemMenuComputerIcon etc...
> > 
> > I think we should refer these icons in /ash/.../tray_bluetooth.cc to have
all
> > design information in files under /ash.
> > To do so, ash::BluetoothDeviceInfo should have device type instead of the
> actual
> > image in 
> >
>
https://codereview.chromium.org/2381493005/diff/60001/ash/common/system/tray/...
> > 
> 
> +1, I like the idea of storing the device type instead of the image itself.
> 
> > However, ash can be built in non-ChromeOS environment, so we can not
directly
> > refer to device::BluetoothDevice::DeviceType in system_tray_delegate.h as
> > //device/bluetooth is build only on ChromeOS.
> > 
> > What should I do to fix this situation?
> > One possible solution might be that BluetoothDeviceInfo has its own enum for
> > device type (instead of //device/bluetooth's enum), like I did in
> >
>
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
> > But I guess there are better solutions?
> 
> Instead of having multiple enums that are essentially identical, is there
> somewhere
> common where you can move the enum so that we only need to have one of them?
> (base/ seems like overkill, maybe oshima@ can suggest something better?)
> 
> Failing any other ideas, I'd be ok with your proposal.

oshima@ san, could you suggest me the place where we can move the device type
enum from device/bluetooth, or can we have own enum for device type in
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
?

[oshima, 2016-10-04 18:04:30]

On 2016/10/04 12:28:54, fukino wrote:
> On 2016/09/30 17:27:01, tdanderson wrote:
> > On 2016/09/30 14:31:32, fukino wrote:
> > > I got a link error for ash::kSystemMenuComputerIcon etc...
> > > 

That's because these symbols aren't exported.

> > > I think we should refer these icons in /ash/.../tray_bluetooth.cc to have
> all
> > > design information in files under /ash.
> > > To do so, ash::BluetoothDeviceInfo should have device type instead of the
> > actual
> > > image in 
> > >
> >
>
https://codereview.chromium.org/2381493005/diff/60001/ash/common/system/tray/...
> > > 
> > 
> > +1, I like the idea of storing the device type instead of the image itself.
> > 
> > > However, ash can be built in non-ChromeOS environment, so we can not
> directly
> > > refer to device::BluetoothDevice::DeviceType in system_tray_delegate.h as
> > > //device/bluetooth is build only on ChromeOS.
> > > 
> > > What should I do to fix this situation?
> > > One possible solution might be that BluetoothDeviceInfo has its own enum
for
> > > device type (instead of //device/bluetooth's enum), like I did in
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
> > > But I guess there are better solutions?
> > 
> > Instead of having multiple enums that are essentially identical, is there
> > somewhere
> > common where you can move the enum so that we only need to have one of them?
> > (base/ seems like overkill, maybe oshima@ can suggest something better?)
> > 
> > Failing any other ideas, I'd be ok with your proposal.
> 
> oshima@ san, could you suggest me the place where we can move the device type
> enum from device/bluetooth, or can we have own enum for device type in
>
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
> ?

Just switch to enum class (I probably have to move it out from BluetoothDevice
class), then you can refer to it using forward decl in
the interface.

[oshima, 2016-10-04 19:23:57]

On 2016/10/04 18:04:30, oshima wrote:
> On 2016/10/04 12:28:54, fukino wrote:
> > On 2016/09/30 17:27:01, tdanderson wrote:
> > > On 2016/09/30 14:31:32, fukino wrote:
> > > > I got a link error for ash::kSystemMenuComputerIcon etc...
> > > > 
> 
> That's because these symbols aren't exported.
> 
> > > > I think we should refer these icons in /ash/.../tray_bluetooth.cc to
have
> > all
> > > > design information in files under /ash.
> > > > To do so, ash::BluetoothDeviceInfo should have device type instead of
the
> > > actual
> > > > image in 
> > > >
> > >
> >
>
https://codereview.chromium.org/2381493005/diff/60001/ash/common/system/tray/...
> > > > 
> > > 
> > > +1, I like the idea of storing the device type instead of the image
itself.
> > > 
> > > > However, ash can be built in non-ChromeOS environment, so we can not
> > directly
> > > > refer to device::BluetoothDevice::DeviceType in system_tray_delegate.h
as
> > > > //device/bluetooth is build only on ChromeOS.
> > > > 
> > > > What should I do to fix this situation?
> > > > One possible solution might be that BluetoothDeviceInfo has its own enum
> for
> > > > device type (instead of //device/bluetooth's enum), like I did in
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
> > > > But I guess there are better solutions?
> > > 
> > > Instead of having multiple enums that are essentially identical, is there
> > > somewhere
> > > common where you can move the enum so that we only need to have one of
them?
> > > (base/ seems like overkill, maybe oshima@ can suggest something better?)
> > > 
> > > Failing any other ideas, I'd be ok with your proposal.
> > 
> > oshima@ san, could you suggest me the place where we can move the device
type
> > enum from device/bluetooth, or can we have own enum for device type in
> >
>
https://codereview.chromium.org/1931563002/diff/20001/ash/system/tray/system_...
> > ?
> 
> Just switch to enum class (I probably have to move it out from BluetoothDevice
s/I/we
> class), then you can refer to it using forward decl in
> the interface.

[oshima, 2016-10-04 21:00:59]

https://codereview.chromium.org/2381493005/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2381493005/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:197: break;
remove default:

[fukino, 2016-10-05 02:52:32]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 02:52:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 03:18:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 03:18:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fukino, 2016-10-05 05:14:42]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 05:14:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fukino, 2016-10-05 06:30:30]

> Just switch to enum class (I probably have to move it out from BluetoothDevice
> class), then you can refer to it using forward decl in
> the interface.

It worked. Thank you so much!!

[fukino, 2016-10-05 06:34:45]

https://codereview.chromium.org/2381493005/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2381493005/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:197: break;
On 2016/10/04 21:00:59, oshima wrote:
> remove default:

Done in ash/common/system/chromeos/bluetooth/tray_bluetooth.cc.

https://codereview.chromium.org/2381493005/diff/120001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_common.h (right):

https://codereview.chromium.org/2381493005/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_common.h:31: UNKNOWN,
BLUETOOTH_DEVICE_TYPE_UNKNOWN is more consistent with values of
BluetoothTransport in this file.

However, it doesn't look common in chromium to add prefixes for enum class
members, so I simply used "UNKNOWN", "COMPUTER", etc...

[fukino, 2016-10-05 06:51:59]

fukino@chromium.org changed reviewers:
+ isherman@chromium.org, ortuno@chromium.org, stevenjb@chromium.org

[fukino, 2016-10-05 06:51:59]

Hi ortuno@, stevenjb@, isherman@,

I moved the inner enum BluetoothDevice::DeviceType out of BluetoothDevice class
so that we can refer it using forward declaration in
ash/common/system/tray/system_tray_delegate.h.
Could you take a look at changes for it?

ortuno@: PTAL at device/bluetooth/*
stevenjb@: PTAL at extensions/browser/api/bluetooth/*
isherman@: PTAL at chrome/browser/metrics/chromeos_metrics_provider.cc and
components/proximity_auth/bluetooth_connection_finder_unittest.cc.

Thanks!

[ortuno, 2016-10-05 06:59:11]

Please separate the bluetooth changes from the ash changes.

https://codereview.chromium.org/2381493005/diff/120001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_common.h (right):

https://codereview.chromium.org/2381493005/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_common.h:31: UNKNOWN,
On 2016/10/05 at 06:34:45, fukino wrote:
> BLUETOOTH_DEVICE_TYPE_UNKNOWN is more consistent with values of
BluetoothTransport in this file.
> 
> However, it doesn't look common in chromium to add prefixes for enum class
members, so I simply used "UNKNOWN", "COMPUTER", etc...

Makes sense. I think we added the prefix to BluetoothTransport because it's only
an enum rather than an enum class.

[fukino, 2016-10-05 07:32:57]

On 2016/10/05 06:59:11, ortuno wrote:
> Please separate the bluetooth changes from the ash changes.
> 
>
https://codereview.chromium.org/2381493005/diff/120001/device/bluetooth/bluet...
> File device/bluetooth/bluetooth_common.h (right):
> 
>
https://codereview.chromium.org/2381493005/diff/120001/device/bluetooth/bluet...
> device/bluetooth/bluetooth_common.h:31: UNKNOWN,
> On 2016/10/05 at 06:34:45, fukino wrote:
> > BLUETOOTH_DEVICE_TYPE_UNKNOWN is more consistent with values of
> BluetoothTransport in this file.
> > 
> > However, it doesn't look common in chromium to add prefixes for enum class
> members, so I simply used "UNKNOWN", "COMPUTER", etc...
> 
> Makes sense. I think we added the prefix to BluetoothTransport because it's
only
> an enum rather than an enum class.

Thanks!
I separated the bluetooth changes as
https://codereview.chromium.org/2394473005/.
I'll rebase this CL after the bluetooth changes land.

[fukino, 2016-10-05 07:33:20]

fukino@chromium.org changed reviewers:
- isherman@chromium.org, ortuno@chromium.org, stevenjb@chromium.org

[commit-bot: I haz the power, 2016-10-05 08:26:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 08:26:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fukino, 2016-10-06 00:50:17]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 00:51:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fukino, 2016-10-06 01:01:35]

I rebased this CL.
tdanderson@, could you take a look at ash/common/system/ again? I moved the code
to load icons from 	chrome/browser/ui/ash/ to ash/common/system/.

[commit-bot: I haz the power, 2016-10-06 01:56:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 01:56:36]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-10-06 14:12:12]

still lgtm for patch set 8

https://chromiumcodereview.appspot.com/2381493005/diff/140001/ash/common/syst...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://chromiumcodereview.appspot.com/2381493005/diff/140001/ash/common/syst...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:98: LOG(WARNING) <<
"Unknown device type icon for Bluetooth was requested.";
Would a NOTREACHED() be more appropriate here?

https://chromiumcodereview.appspot.com/2381493005/diff/140001/ash/common/syst...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:323:
GetBluetoothDeviceIcon(list[i].device_type), ash::kMenuIconColor);
nit: ash:: not needed

[fukino, 2016-10-06 14:45:30]

https://codereview.chromium.org/2381493005/diff/140001/ash/common/system/chro...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2381493005/diff/140001/ash/common/system/chro...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:98: LOG(WARNING) <<
"Unknown device type icon for Bluetooth was requested.";
On 2016/10/06 14:12:12, tdanderson wrote:
> Would a NOTREACHED() be more appropriate here?

I'm not sure if it is critical that a BT device is classified as UNKNOWN.
(Maybe it can occur when a new BT spec is added and Chrome is still not
updated?)

As the classification login in device/bluetooth doesn't treat it as an error, I
use WARNING here.
https://codesearch.chromium.org/chromium/src/device/bluetooth/bluetooth_devic...

[tdanderson, 2016-10-06 14:47:16]

https://codereview.chromium.org/2381493005/diff/140001/ash/common/system/chro...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2381493005/diff/140001/ash/common/system/chro...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:98: LOG(WARNING) <<
"Unknown device type icon for Bluetooth was requested.";
On 2016/10/06 14:45:30, fukino wrote:
> On 2016/10/06 14:12:12, tdanderson wrote:
> > Would a NOTREACHED() be more appropriate here?
> 
> I'm not sure if it is critical that a BT device is classified as UNKNOWN.
> (Maybe it can occur when a new BT spec is added and Chrome is still not
> updated?)
> 
> As the classification login in device/bluetooth doesn't treat it as an error,
I
> use WARNING here.
>
https://codesearch.chromium.org/chromium/src/device/bluetooth/bluetooth_devic...

OK, sg

[fukino, 2016-10-07 00:19:16]

Thank you!

https://codereview.chromium.org/2381493005/diff/140001/ash/common/system/chro...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2381493005/diff/140001/ash/common/system/chro...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:323:
GetBluetoothDeviceIcon(list[i].device_type), ash::kMenuIconColor);
On 2016/10/06 14:12:12, tdanderson wrote:
> nit: ash:: not needed

Done.

[fukino, 2016-10-07 00:19:23]

The CQ bit was checked by fukino@chromium.org

[fukino, 2016-10-07 00:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2381493005/#ps160001
(title: "Remove unnecessary ash::")

[commit-bot: I haz the power, 2016-10-07 00:19:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 00:23:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 00:23:03]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fukino, 2016-10-07 01:19:22]

The CQ bit was checked by fukino@chromium.org

[fukino, 2016-10-07 01:19:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2381493005/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-07 01:19:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 02:47:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 02:47:09]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fukino, 2016-10-11 03:55:30]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2016-10-11 03:55:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 04:55:02]

Description was changed from

==========
Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam

This CL also shows them on bluetooth device list on MD ash tray,
and makes the Bluetooth toggle button functional. 

BUG=632128
TEST=manually tested with the flag --ash-md=experimental
==========

to

==========
Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam

This CL also shows them on bluetooth device list on MD ash tray,
and makes the Bluetooth toggle button functional. 

BUG=632128
TEST=manually tested with the flag --ash-md=experimental
==========

[commit-bot: I haz the power, 2016-10-11 04:55:03]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-10-11 04:56:26]

Description was changed from

==========
Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam

This CL also shows them on bluetooth device list on MD ash tray,
and makes the Bluetooth toggle button functional. 

BUG=632128
TEST=manually tested with the flag --ash-md=experimental
==========

to

==========
Add bluetooth device type icons on MD system tray.

Adds following icons which are used in Bluetooth detailed view.
* Computer
* Gamepad
* Headset
* Mouse
* Phone
* Videocam

This CL also shows them on bluetooth device list on MD ash tray,
and makes the Bluetooth toggle button functional.

BUG=632128
TEST=manually tested with the flag --ash-md=experimental

Committed: https://crrev.com/0e63d97a3cae0c52121dc15c87d68337ba585d1d
Cr-Commit-Position: refs/heads/master@{#424371}
==========

[commit-bot: I haz the power, 2016-10-11 04:56:28]

Patchset 10 (id:??) landed as
https://crrev.com/0e63d97a3cae0c52121dc15c87d68337ba585d1d
Cr-Commit-Position: refs/heads/master@{#424371}