Add Bluetooth device type icons on the device list in system tray.
This CL does:
- Adds device type icons on BT device list in system tray.
- Shows checkmark on each device type icon when the device is connected.
The design spec is attached in the bug.
BUG=595942
Description was changed from ========== Add Bluetooth device type icons on the device list in ...
4 years, 7 months ago
(2016-04-27 19:16:59 UTC)
#1
Description was changed from
==========
Add Bluetooth device type icons on the device list in system tray.
BUG=
==========
to
==========
Add Bluetooth device type icons on the device list in system tray.
This CL does:
- Adds device type icons on BT device list in system tray.
- Shows checkmark on each device type icon when the device is connected.
The design spec is attached in the bug.
BUG=595942
==========
On 2016/04/27 20:31:28, oshima wrote: > q for varkha@. are we going to switch to ...
4 years, 7 months ago
(2016-04-27 21:17:57 UTC)
#6
On 2016/04/27 20:31:28, oshima wrote:
> q for varkha@. are we going to switch to vector graphics on ash as well?
Yes, that's a plan. http://crbug.com/595015 is one example. So the new assets
here will need to go through a vectorizing script.
On 2016/04/27 21:17:57, varkha wrote: > On 2016/04/27 20:31:28, oshima wrote: > > q for ...
4 years, 7 months ago
(2016-04-27 23:26:18 UTC)
#8
On 2016/04/27 21:17:57, varkha wrote:
> On 2016/04/27 20:31:28, oshima wrote:
> > q for varkha@. are we going to switch to vector graphics on ash as well?
>
> Yes, that's a plan. http://crbug.com/595015 is one example. So the new assets
> here will need to go through a vectorizing script.
do you think we should start doing it now?
On 2016/04/27 23:26:18, oshima wrote: > On 2016/04/27 21:17:57, varkha wrote: > > On 2016/04/27 ...
4 years, 7 months ago
(2016-04-27 23:28:45 UTC)
#10
On 2016/04/27 23:26:18, oshima wrote:
> On 2016/04/27 21:17:57, varkha wrote:
> > On 2016/04/27 20:31:28, oshima wrote:
> > > q for varkha@. are we going to switch to vector graphics on ash as well?
> >
> > Yes, that's a plan. http://crbug.com/595015 is one example. So the new
assets
> > here will need to go through a vectorizing script.
>
> do you think we should start doing it now?
I think tdanderson@ (cc'ed) was planning on doing this once the ash-md flag
lands.
He is away this week. It is a script so it should not matter too much how many
assets we convert but I haven't done this before so I might be wrong.
4 years, 7 months ago
(2016-04-28 00:02:27 UTC)
#11
Thanks!
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
On 2016/04/27 20:31:27, oshima wrote:
> ash can depends on /chromeos
>
> What's the reason to define the same enum on ash and chromeos?
In the Patch Set 1 of this CL, I directly referred
device::BluetootyDevice::DeviceType in ash/system/tray/system_tray_delegate.h.
But I got following error in analyze step of try bots.
=================================
ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not allowed.
#include "device/bluetooth/bluetooth_device.h"
^----------------------------------
It is not in any dependency of
//ash:ash
The include file is in the target(s):
//device/bluetooth:bluetooth
which should somehow be reachable.
=================================
And I misinterpreted it. I thought ash couldn't depend on
device/bluetooth/bluetooth_device.h...
Now I think I should have modified GN file. I'll try to fix it.
4 years, 7 months ago
(2016-04-28 00:17:16 UTC)
#12
On 2016/04/28 00:02:27, fukino wrote:
> Thanks!
>
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
>
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> On 2016/04/27 20:31:27, oshima wrote:
> > ash can depends on /chromeos
> >
> > What's the reason to define the same enum on ash and chromeos?
>
> In the Patch Set 1 of this CL, I directly referred
> device::BluetootyDevice::DeviceType in ash/system/tray/system_tray_delegate.h.
> But I got following error in analyze step of try bots.
> =================================
> ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not allowed.
> #include "device/bluetooth/bluetooth_device.h"
> ^----------------------------------
> It is not in any dependency of
> //ash:ash
> The include file is in the target(s):
> //device/bluetooth:bluetooth
> which should somehow be reachable.
> =================================
>
> And I misinterpreted it. I thought ash couldn't depend on
> device/bluetooth/bluetooth_device.h...
> Now I think I should have modified GN file. I'll try to fix it.
ah, that's because chromeos specific system tray should be in
ash/system/chromeos.
fukino
On 2016/04/28 00:17:16, oshima wrote: > On 2016/04/28 00:02:27, fukino wrote: > > Thanks! > ...
4 years, 7 months ago
(2016-04-28 02:49:39 UTC)
#13
On 2016/04/28 00:17:16, oshima wrote:
> On 2016/04/28 00:02:27, fukino wrote:
> > Thanks!
> >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > On 2016/04/27 20:31:27, oshima wrote:
> > > ash can depends on /chromeos
> > >
> > > What's the reason to define the same enum on ash and chromeos?
> >
> > In the Patch Set 1 of this CL, I directly referred
> > device::BluetootyDevice::DeviceType in
ash/system/tray/system_tray_delegate.h.
> > But I got following error in analyze step of try bots.
> > =================================
> > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
allowed.
> > #include "device/bluetooth/bluetooth_device.h"
> > ^----------------------------------
> > It is not in any dependency of
> > //ash:ash
> > The include file is in the target(s):
> > //device/bluetooth:bluetooth
> > which should somehow be reachable.
> > =================================
> >
> > And I misinterpreted it. I thought ash couldn't depend on
> > device/bluetooth/bluetooth_device.h...
> > Now I think I should have modified GN file. I'll try to fix it.
>
> ah, that's because chromeos specific system tray should be in
> ash/system/chromeos.
OK.
I'll move ash/system/bluetooth to ash/system/chromeos/bluetooth in a separate
CL, and rebase this CL after that.
tdanderson
On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote: > On 2016/04/28 00:17:16, oshima wrote: ...
4 years, 7 months ago
(2016-04-29 17:03:54 UTC)
#14
On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote:
> On 2016/04/28 00:17:16, oshima wrote:
> > On 2016/04/28 00:02:27, fukino wrote:
> > > Thanks!
> > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > On 2016/04/27 20:31:27, oshima wrote:
> > > > ash can depends on /chromeos
> > > >
> > > > What's the reason to define the same enum on ash and chromeos?
> > >
> > > In the Patch Set 1 of this CL, I directly referred
> > > device::BluetootyDevice::DeviceType in
> ash/system/tray/system_tray_delegate.h.
> > > But I got following error in analyze step of try bots.
> > > =================================
> > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
> allowed.
> > > #include "device/bluetooth/bluetooth_device.h"
> > > ^----------------------------------
> > > It is not in any dependency of
> > > //ash:ash
> > > The include file is in the target(s):
> > > //device/bluetooth:bluetooth
> > > which should somehow be reachable.
> > > =================================
> > >
> > > And I misinterpreted it. I thought ash couldn't depend on
> > > device/bluetooth/bluetooth_device.h...
> > > Now I think I should have modified GN file. I'll try to fix it.
> >
> > ah, that's because chromeos specific system tray should be in
> > ash/system/chromeos.
>
> OK.
> I'll move ash/system/bluetooth to ash/system/chromeos/bluetooth in a separate
> CL, and rebase this CL after that.
estade@ has created a write-up on how to vectorize icons:
http://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui
I haven't yet vectorized any for other parts of the ChromeOS UI, but
I was planning on putting them within a new subdirectory of
ui/gfx/vector_icons
oshima
On 2016/04/29 17:03:54, tdanderson wrote: > On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote: ...
4 years, 7 months ago
(2016-04-29 17:11:15 UTC)
#15
On 2016/04/29 17:03:54, tdanderson wrote:
> On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote:
> > On 2016/04/28 00:17:16, oshima wrote:
> > > On 2016/04/28 00:02:27, fukino wrote:
> > > > Thanks!
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > > On 2016/04/27 20:31:27, oshima wrote:
> > > > > ash can depends on /chromeos
> > > > >
> > > > > What's the reason to define the same enum on ash and chromeos?
> > > >
> > > > In the Patch Set 1 of this CL, I directly referred
> > > > device::BluetootyDevice::DeviceType in
> > ash/system/tray/system_tray_delegate.h.
> > > > But I got following error in analyze step of try bots.
> > > > =================================
> > > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
> > allowed.
> > > > #include "device/bluetooth/bluetooth_device.h"
> > > > ^----------------------------------
> > > > It is not in any dependency of
> > > > //ash:ash
> > > > The include file is in the target(s):
> > > > //device/bluetooth:bluetooth
> > > > which should somehow be reachable.
> > > > =================================
> > > >
> > > > And I misinterpreted it. I thought ash couldn't depend on
> > > > device/bluetooth/bluetooth_device.h...
> > > > Now I think I should have modified GN file. I'll try to fix it.
> > >
> > > ah, that's because chromeos specific system tray should be in
> > > ash/system/chromeos.
> >
> > OK.
> > I'll move ash/system/bluetooth to ash/system/chromeos/bluetooth in a
separate
> > CL, and rebase this CL after that.
>
> estade@ has created a write-up on how to vectorize icons:
>
http://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui
>
> I haven't yet vectorized any for other parts of the ChromeOS UI, but
> I was planning on putting them within a new subdirectory of
> ui/gfx/vector_icons
What's the long term plan to manage these icons? Putting all icons in one place
sounds bad (even if you have subdir like ui/gfx/vector_icons/ash)
tdanderson
On 2016/04/29 17:11:15, oshima wrote: > On 2016/04/29 17:03:54, tdanderson wrote: > > On 2016/04/28 ...
4 years, 7 months ago
(2016-04-29 17:19:43 UTC)
#16
On 2016/04/29 17:11:15, oshima wrote:
> On 2016/04/29 17:03:54, tdanderson wrote:
> > On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote:
> > > On 2016/04/28 00:17:16, oshima wrote:
> > > > On 2016/04/28 00:02:27, fukino wrote:
> > > > > Thanks!
> > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > > > On 2016/04/27 20:31:27, oshima wrote:
> > > > > > ash can depends on /chromeos
> > > > > >
> > > > > > What's the reason to define the same enum on ash and chromeos?
> > > > >
> > > > > In the Patch Set 1 of this CL, I directly referred
> > > > > device::BluetootyDevice::DeviceType in
> > > ash/system/tray/system_tray_delegate.h.
> > > > > But I got following error in analyze step of try bots.
> > > > > =================================
> > > > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
> > > allowed.
> > > > > #include "device/bluetooth/bluetooth_device.h"
> > > > > ^----------------------------------
> > > > > It is not in any dependency of
> > > > > //ash:ash
> > > > > The include file is in the target(s):
> > > > > //device/bluetooth:bluetooth
> > > > > which should somehow be reachable.
> > > > > =================================
> > > > >
> > > > > And I misinterpreted it. I thought ash couldn't depend on
> > > > > device/bluetooth/bluetooth_device.h...
> > > > > Now I think I should have modified GN file. I'll try to fix it.
> > > >
> > > > ah, that's because chromeos specific system tray should be in
> > > > ash/system/chromeos.
> > >
> > > OK.
> > > I'll move ash/system/bluetooth to ash/system/chromeos/bluetooth in a
> separate
> > > CL, and rebase this CL after that.
> >
> > estade@ has created a write-up on how to vectorize icons:
> >
>
http://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui
> >
> > I haven't yet vectorized any for other parts of the ChromeOS UI, but
> > I was planning on putting them within a new subdirectory of
> > ui/gfx/vector_icons
>
> What's the long term plan to manage these icons? Putting all icons in one
place
> sounds bad (even if you have subdir like ui/gfx/vector_icons/ash)
+Evan, I see you added a TODO to better organize the icons. Did you have
a plan in mind?
I agree that dumping all of the ash stuff into a single subdirectory would get
messy. My plan was to by default mimic the existing directory structure of
ash/resources/default_{100,200}_percent/, and possibly doing a bit of
reorganizations in cases where it clearly made sense to do so.
ui/gfx/vector_icons/
Evan Stade
On 2016/04/29 17:19:43, tdanderson wrote: > On 2016/04/29 17:11:15, oshima wrote: > > On 2016/04/29 ...
4 years, 7 months ago
(2016-04-29 19:56:56 UTC)
#17
On 2016/04/29 17:19:43, tdanderson wrote:
> On 2016/04/29 17:11:15, oshima wrote:
> > On 2016/04/29 17:03:54, tdanderson wrote:
> > > On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote:
> > > > On 2016/04/28 00:17:16, oshima wrote:
> > > > > On 2016/04/28 00:02:27, fukino wrote:
> > > > > > Thanks!
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > > > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > > > > On 2016/04/27 20:31:27, oshima wrote:
> > > > > > > ash can depends on /chromeos
> > > > > > >
> > > > > > > What's the reason to define the same enum on ash and chromeos?
> > > > > >
> > > > > > In the Patch Set 1 of this CL, I directly referred
> > > > > > device::BluetootyDevice::DeviceType in
> > > > ash/system/tray/system_tray_delegate.h.
> > > > > > But I got following error in analyze step of try bots.
> > > > > > =================================
> > > > > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
> > > > allowed.
> > > > > > #include "device/bluetooth/bluetooth_device.h"
> > > > > > ^----------------------------------
> > > > > > It is not in any dependency of
> > > > > > //ash:ash
> > > > > > The include file is in the target(s):
> > > > > > //device/bluetooth:bluetooth
> > > > > > which should somehow be reachable.
> > > > > > =================================
> > > > > >
> > > > > > And I misinterpreted it. I thought ash couldn't depend on
> > > > > > device/bluetooth/bluetooth_device.h...
> > > > > > Now I think I should have modified GN file. I'll try to fix it.
> > > > >
> > > > > ah, that's because chromeos specific system tray should be in
> > > > > ash/system/chromeos.
> > > >
> > > > OK.
> > > > I'll move ash/system/bluetooth to ash/system/chromeos/bluetooth in a
> > separate
> > > > CL, and rebase this CL after that.
> > >
> > > estade@ has created a write-up on how to vectorize icons:
> > >
> >
>
http://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui
> > >
> > > I haven't yet vectorized any for other parts of the ChromeOS UI, but
> > > I was planning on putting them within a new subdirectory of
> > > ui/gfx/vector_icons
> >
> > What's the long term plan to manage these icons? Putting all icons in one
> place
> > sounds bad (even if you have subdir like ui/gfx/vector_icons/ash)
>
> +Evan, I see you added a TODO to better organize the icons. Did you have
> a plan in mind?
>
> I agree that dumping all of the ash stuff into a single subdirectory would get
> messy. My plan was to by default mimic the existing directory structure of
> ash/resources/default_{100,200}_percent/, and possibly doing a bit of
> reorganizations in cases where it clearly made sense to do so.
>
> ui/gfx/vector_icons/
There is some discussion here:
https://bugs.chromium.org/p/chromium/issues/detail?id=535386
It looks like we should be moving the .icon list from BUILD.gn to its own file.
I think subdirectories such as ui/gfx/vector_icons/ash/ with additional icon
list files would make sense. oshima@, why does this sound bad? The way we have
raster assets scattered across a million directories has made it a much bigger
ordeal to
a) find/update all the icons Chrome uses
b) discover or avoid duplicates
and I don't understand the purported benefit.
oshima
On 2016/04/29 19:56:56, Evan Stade wrote: > On 2016/04/29 17:19:43, tdanderson wrote: > > On ...
4 years, 7 months ago
(2016-04-29 20:38:06 UTC)
#18
On 2016/04/29 19:56:56, Evan Stade wrote:
> On 2016/04/29 17:19:43, tdanderson wrote:
> > On 2016/04/29 17:11:15, oshima wrote:
> > > On 2016/04/29 17:03:54, tdanderson wrote:
> > > > On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote:
> > > > > On 2016/04/28 00:17:16, oshima wrote:
> > > > > > On 2016/04/28 00:02:27, fukino wrote:
> > > > > > > Thanks!
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
(right):
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > > > > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > > > > > On 2016/04/27 20:31:27, oshima wrote:
> > > > > > > > ash can depends on /chromeos
> > > > > > > >
> > > > > > > > What's the reason to define the same enum on ash and chromeos?
> > > > > > >
> > > > > > > In the Patch Set 1 of this CL, I directly referred
> > > > > > > device::BluetootyDevice::DeviceType in
> > > > > ash/system/tray/system_tray_delegate.h.
> > > > > > > But I got following error in analyze step of try bots.
> > > > > > > =================================
> > > > > > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include
not
> > > > > allowed.
> > > > > > > #include "device/bluetooth/bluetooth_device.h"
> > > > > > > ^----------------------------------
> > > > > > > It is not in any dependency of
> > > > > > > //ash:ash
> > > > > > > The include file is in the target(s):
> > > > > > > //device/bluetooth:bluetooth
> > > > > > > which should somehow be reachable.
> > > > > > > =================================
> > > > > > >
> > > > > > > And I misinterpreted it. I thought ash couldn't depend on
> > > > > > > device/bluetooth/bluetooth_device.h...
> > > > > > > Now I think I should have modified GN file. I'll try to fix it.
> > > > > >
> > > > > > ah, that's because chromeos specific system tray should be in
> > > > > > ash/system/chromeos.
> > > > >
> > > > > OK.
> > > > > I'll move ash/system/bluetooth to ash/system/chromeos/bluetooth in a
> > > separate
> > > > > CL, and rebase this CL after that.
> > > >
> > > > estade@ has created a write-up on how to vectorize icons:
> > > >
> > >
> >
>
http://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui
> > > >
> > > > I haven't yet vectorized any for other parts of the ChromeOS UI, but
> > > > I was planning on putting them within a new subdirectory of
> > > > ui/gfx/vector_icons
> > >
> > > What's the long term plan to manage these icons? Putting all icons in one
> > place
> > > sounds bad (even if you have subdir like ui/gfx/vector_icons/ash)
> >
> > +Evan, I see you added a TODO to better organize the icons. Did you have
> > a plan in mind?
> >
> > I agree that dumping all of the ash stuff into a single subdirectory would
get
> > messy. My plan was to by default mimic the existing directory structure of
> > ash/resources/default_{100,200}_percent/, and possibly doing a bit of
> > reorganizations in cases where it clearly made sense to do so.
> >
> > ui/gfx/vector_icons/
>
> There is some discussion here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=535386
>
> It looks like we should be moving the .icon list from BUILD.gn to its own
file.
> I think subdirectories such as ui/gfx/vector_icons/ash/ with additional icon
> list files would make sense. oshima@, why does this sound bad? The way we have
> raster assets scattered across a million directories has made it a much bigger
> ordeal to
>
> a) find/update all the icons Chrome uses
> b) discover or avoid duplicates
>
> and I don't understand the purported benefit.
It's layering violation. ui/gfx shouldn't know about ash (or other components/
etc).
It's possible to have src/resources | src/vector_icons, although I still prefer
to put them under ash (but I can be convinced the other way).
Regarding duplicates, having them under one directory may not help that much. At
least, it didn't and
we had similar icons even in one directory like in chrome/app/theme. I think
it's better to have a tool for that.
fukino
On 2016/04/28 00:17:16, oshima wrote: > On 2016/04/28 00:02:27, fukino wrote: > > Thanks! > ...
4 years, 7 months ago
(2016-05-26 23:40:39 UTC)
#19
On 2016/04/28 00:17:16, oshima wrote:
> On 2016/04/28 00:02:27, fukino wrote:
> > Thanks!
> >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > On 2016/04/27 20:31:27, oshima wrote:
> > > ash can depends on /chromeos
> > >
> > > What's the reason to define the same enum on ash and chromeos?
> >
> > In the Patch Set 1 of this CL, I directly referred
> > device::BluetootyDevice::DeviceType in
ash/system/tray/system_tray_delegate.h.
> > But I got following error in analyze step of try bots.
> > =================================
> > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
allowed.
> > #include "device/bluetooth/bluetooth_device.h"
> > ^----------------------------------
> > It is not in any dependency of
> > //ash:ash
> > The include file is in the target(s):
> > //device/bluetooth:bluetooth
> > which should somehow be reachable.
> > =================================
> >
> > And I misinterpreted it. I thought ash couldn't depend on
> > device/bluetooth/bluetooth_device.h...
> > Now I think I should have modified GN file. I'll try to fix it.
>
> ah, that's because chromeos specific system tray should be in
> ash/system/chromeos.
I moved BT-related system tray stuff under chromeos and used #ifdef(OS_CHROMEOS)
to include "device/bluetooth/bluetooth_device.h" in system_tray_delegate.h.
https://codereview.chromium.org/1931563002/diff/40001/ash/system/tray/system_...
However, I still get the same error described above.
Should I add "+device/bluetooth" to ash/system/DEPS? (Is it OK?)
oshima
On 2016/05/26 23:40:39, fukino wrote: > On 2016/04/28 00:17:16, oshima wrote: > > On 2016/04/28 ...
4 years, 7 months ago
(2016-05-26 23:50:47 UTC)
#20
On 2016/05/26 23:40:39, fukino wrote:
> On 2016/04/28 00:17:16, oshima wrote:
> > On 2016/04/28 00:02:27, fukino wrote:
> > > Thanks!
> > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > On 2016/04/27 20:31:27, oshima wrote:
> > > > ash can depends on /chromeos
> > > >
> > > > What's the reason to define the same enum on ash and chromeos?
> > >
> > > In the Patch Set 1 of this CL, I directly referred
> > > device::BluetootyDevice::DeviceType in
> ash/system/tray/system_tray_delegate.h.
> > > But I got following error in analyze step of try bots.
> > > =================================
> > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
> allowed.
> > > #include "device/bluetooth/bluetooth_device.h"
> > > ^----------------------------------
> > > It is not in any dependency of
> > > //ash:ash
> > > The include file is in the target(s):
> > > //device/bluetooth:bluetooth
> > > which should somehow be reachable.
> > > =================================
> > >
> > > And I misinterpreted it. I thought ash couldn't depend on
> > > device/bluetooth/bluetooth_device.h...
> > > Now I think I should have modified GN file. I'll try to fix it.
> >
> > ah, that's because chromeos specific system tray should be in
> > ash/system/chromeos.
>
> I moved BT-related system tray stuff under chromeos and used
#ifdef(OS_CHROMEOS)
> to include "device/bluetooth/bluetooth_device.h" in system_tray_delegate.h.
>
https://codereview.chromium.org/1931563002/diff/40001/ash/system/tray/system_...
>
> However, I still get the same error described above.
> Should I add "+device/bluetooth" to ash/system/DEPS? (Is it OK?)
That means you didn't move all. You need to move all that depends on bluetooth
to chromeos/.
fukino
On 2016/05/26 23:50:47, oshima wrote: > On 2016/05/26 23:40:39, fukino wrote: > > On 2016/04/28 ...
4 years, 7 months ago
(2016-05-27 01:08:54 UTC)
#21
On 2016/05/26 23:50:47, oshima wrote:
> On 2016/05/26 23:40:39, fukino wrote:
> > On 2016/04/28 00:17:16, oshima wrote:
> > > On 2016/04/28 00:02:27, fukino wrote:
> > > > Thanks!
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > > On 2016/04/27 20:31:27, oshima wrote:
> > > > > ash can depends on /chromeos
> > > > >
> > > > > What's the reason to define the same enum on ash and chromeos?
> > > >
> > > > In the Patch Set 1 of this CL, I directly referred
> > > > device::BluetootyDevice::DeviceType in
> > ash/system/tray/system_tray_delegate.h.
> > > > But I got following error in analyze step of try bots.
> > > > =================================
> > > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
> > allowed.
> > > > #include "device/bluetooth/bluetooth_device.h"
> > > > ^----------------------------------
> > > > It is not in any dependency of
> > > > //ash:ash
> > > > The include file is in the target(s):
> > > > //device/bluetooth:bluetooth
> > > > which should somehow be reachable.
> > > > =================================
> > > >
> > > > And I misinterpreted it. I thought ash couldn't depend on
> > > > device/bluetooth/bluetooth_device.h...
> > > > Now I think I should have modified GN file. I'll try to fix it.
> > >
> > > ah, that's because chromeos specific system tray should be in
> > > ash/system/chromeos.
> >
> > I moved BT-related system tray stuff under chromeos and used
> #ifdef(OS_CHROMEOS)
> > to include "device/bluetooth/bluetooth_device.h" in system_tray_delegate.h.
> >
>
https://codereview.chromium.org/1931563002/diff/40001/ash/system/tray/system_...
> >
> > However, I still get the same error described above.
> > Should I add "+device/bluetooth" to ash/system/DEPS? (Is it OK?)
>
> That means you didn't move all. You need to move all that depends on bluetooth
> to chromeos/.
Do you mean we should remove definitions of BluetoothDeviceInfo and
BluetoothDeviceList from system_tray_delegate.h?
tdanderson
On 2016/05/27 01:08:54, fukino wrote: > On 2016/05/26 23:50:47, oshima wrote: > > On 2016/05/26 ...
4 years, 6 months ago
(2016-06-09 18:54:18 UTC)
#22
On 2016/05/27 01:08:54, fukino wrote:
> On 2016/05/26 23:50:47, oshima wrote:
> > On 2016/05/26 23:40:39, fukino wrote:
> > > On 2016/04/28 00:17:16, oshima wrote:
> > > > On 2016/04/28 00:02:27, fukino wrote:
> > > > > Thanks!
> > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219: return
> > > > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > > > On 2016/04/27 20:31:27, oshima wrote:
> > > > > > ash can depends on /chromeos
> > > > > >
> > > > > > What's the reason to define the same enum on ash and chromeos?
> > > > >
> > > > > In the Patch Set 1 of this CL, I directly referred
> > > > > device::BluetootyDevice::DeviceType in
> > > ash/system/tray/system_tray_delegate.h.
> > > > > But I got following error in analyze step of try bots.
> > > > > =================================
> > > > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include not
> > > allowed.
> > > > > #include "device/bluetooth/bluetooth_device.h"
> > > > > ^----------------------------------
> > > > > It is not in any dependency of
> > > > > //ash:ash
> > > > > The include file is in the target(s):
> > > > > //device/bluetooth:bluetooth
> > > > > which should somehow be reachable.
> > > > > =================================
> > > > >
> > > > > And I misinterpreted it. I thought ash couldn't depend on
> > > > > device/bluetooth/bluetooth_device.h...
> > > > > Now I think I should have modified GN file. I'll try to fix it.
> > > >
> > > > ah, that's because chromeos specific system tray should be in
> > > > ash/system/chromeos.
> > >
> > > I moved BT-related system tray stuff under chromeos and used
> > #ifdef(OS_CHROMEOS)
> > > to include "device/bluetooth/bluetooth_device.h" in
system_tray_delegate.h.
> > >
> >
>
https://codereview.chromium.org/1931563002/diff/40001/ash/system/tray/system_...
> > >
> > > However, I still get the same error described above.
> > > Should I add "+device/bluetooth" to ash/system/DEPS? (Is it OK?)
> >
> > That means you didn't move all. You need to move all that depends on
bluetooth
> > to chromeos/.
>
> Do you mean we should remove definitions of BluetoothDeviceInfo and
> BluetoothDeviceList from system_tray_delegate.h?
FYI I am vectorizing the keyboard icon here:
https://codereview.chromium.org/2051663005/
Though the icon I am adding is on a 16x16 pixel grid and it looks like the
one you're adding is 20x20. You can try to reuse mine by stretching it
to 20 in a call to CreateVectorIcon() but this may not look good.
4 years, 6 months ago
(2016-06-22 19:48:16 UTC)
#24
Pending for now as the feature is punted.
tdanderson
As an update to the long-term plan of organizing vector icons into subdirectories, I have ...
4 years, 5 months ago
(2016-07-08 16:15:26 UTC)
#25
As an update to the long-term plan of organizing vector icons into
subdirectories, I have a CL in progress here:
https://codereview.chromium.org/2129683002/
On Fri, Apr 29, 2016 at 1:19 PM <tdanderson@chromium.org> wrote:
> On 2016/04/29 17:11:15, oshima wrote:
> > On 2016/04/29 17:03:54, tdanderson wrote:
> > > On 2016/04/28 02:49:39, fukino (OOO til May 8) wrote:
> > > > On 2016/04/28 00:17:16, oshima wrote:
> > > > > On 2016/04/28 00:02:27, fukino wrote:
> > > > > > Thanks!
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
> (right):
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
https://codereview.chromium.org/1931563002/diff/20001/chrome/browser/ui/ash/s...
> > > > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:219:
> return
> > > > > > ash::BluetoothDeviceInfo::DEVICE_UNKNOWN;
> > > > > > On 2016/04/27 20:31:27, oshima wrote:
> > > > > > > ash can depends on /chromeos
> > > > > > >
> > > > > > > What's the reason to define the same enum on ash and chromeos?
> > > > > >
> > > > > > In the Patch Set 1 of this CL, I directly referred
> > > > > > device::BluetootyDevice::DeviceType in
> > > > ash/system/tray/system_tray_delegate.h.
> > > > > > But I got following error in analyze step of try bots.
> > > > > > =================================
> > > > > > ERROR at //ash/system/tray/system_tray_delegate.h:19:11: Include
> not
> > > > allowed.
> > > > > > #include "device/bluetooth/bluetooth_device.h"
> > > > > > ^----------------------------------
> > > > > > It is not in any dependency of
> > > > > > //ash:ash
> > > > > > The include file is in the target(s):
> > > > > > //device/bluetooth:bluetooth
> > > > > > which should somehow be reachable.
> > > > > > =================================
> > > > > >
> > > > > > And I misinterpreted it. I thought ash couldn't depend on
> > > > > > device/bluetooth/bluetooth_device.h...
> > > > > > Now I think I should have modified GN file. I'll try to fix it.
> > > > >
> > > > > ah, that's because chromeos specific system tray should be in
> > > > > ash/system/chromeos.
> > > >
> > > > OK.
> > > > I'll move ash/system/bluetooth to ash/system/chromeos/bluetooth in a
> > separate
> > > > CL, and rebase this CL after that.
> > >
> > > estade@ has created a write-up on how to vectorize icons:
> > >
> >
>
>
http://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui
> > >
> > > I haven't yet vectorized any for other parts of the ChromeOS UI, but
> > > I was planning on putting them within a new subdirectory of
> > > ui/gfx/vector_icons
> >
> > What's the long term plan to manage these icons? Putting all icons in one
> place
> > sounds bad (even if you have subdir like ui/gfx/vector_icons/ash)
>
> +Evan, I see you added a TODO to better organize the icons. Did you have
> a plan in mind?
>
> I agree that dumping all of the ash stuff into a single subdirectory would
> get
> messy. My plan was to by default mimic the existing directory structure of
> ash/resources/default_{100,200}_percent/, and possibly doing a bit of
> reorganizations in cases where it clearly made sense to do so.
>
> ui/gfx/vector_icons/
>
> https://codereview.chromium.org/1931563002/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Issue 1931563002: Add Bluetooth device type icons on the device list in system tray.
(Closed)
Created 4 years, 7 months ago by fukino
Modified 4 years, 1 month ago
Reviewers:
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 4
Chromium Code Reviews
Issue
1931563002:
Add Bluetooth device type icons on the device list in system tray. (Closed)
