Vectorize the remaining material design shelf icons

Vectorize the remaining material design shelf icons

Create vectorized versions of the icons used
in the material design shelf:

* Overflow arrow
* Keyboard
* Notifications bell
* Overview mode

BUG=595015
TEST=manual

Committed: https://crrev.com/4f0032c7bb4d9e4eba2180926e106284ff2bb58d
Cr-Commit-Position: refs/heads/master@{#399092}

[tdanderson, 2016-06-09 18:48:48]

tdanderson@chromium.org changed reviewers:
+ jamescook@chromium.org, sadrul@chromium.org

[tdanderson, 2016-06-09 18:48:49]

James, can you please take a look at ash/ ?

Sadrul, can you please take a look at ui/ ?

[James Cook, 2016-06-09 19:24:48]

I think you should try to avoid loading both the MD and non-MD icons where you
can.

https://codereview.chromium.org/2051663005/diff/1/ash/shelf/overflow_button.cc
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/shelf/overflow_button.c...
ash/shelf/overflow_button.cc:35: bottom_image_md_ =
I would only load the icon when MD is enabled.

https://codereview.chromium.org/2051663005/diff/1/ash/system/chromeos/virtual...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/system/chromeos/virtual...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:34: gfx::ImageSkia
image_md =
ditto

https://codereview.chromium.org/2051663005/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.cc:46: gfx::ImageSkia image_md =
ditto

https://codereview.chromium.org/2051663005/diff/1/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:128: gfx::ImageSkia
image_md = CreateVectorIcon(
ditto

https://codereview.chromium.org/2051663005/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2051663005/diff/1/ui/gfx/BUILD.gn#newcode518
ui/gfx/BUILD.gn:518: "vector_icons/shelf_keyboard.1x.icon",
Just a question for my edification: Why are there special 1x versions of the
icons? Do they need special tuning (like TrueType used to do for small font
sizes)?

[tdanderson, 2016-06-09 22:36:33]

James, please take another look.

https://codereview.chromium.org/2051663005/diff/1/ash/shelf/overflow_button.cc
File ash/shelf/overflow_button.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/shelf/overflow_button.c...
ash/shelf/overflow_button.cc:35: bottom_image_md_ =
On 2016/06/09 19:24:48, James Cook wrote:
> I would only load the icon when MD is enabled.

Done.

https://codereview.chromium.org/2051663005/diff/1/ash/system/chromeos/virtual...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/system/chromeos/virtual...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:34: gfx::ImageSkia
image_md =
On 2016/06/09 19:24:48, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2051663005/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.cc:46: gfx::ImageSkia image_md =
On 2016/06/09 19:24:48, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2051663005/diff/1/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/1/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:128: gfx::ImageSkia
image_md = CreateVectorIcon(
On 2016/06/09 19:24:48, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2051663005/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2051663005/diff/1/ui/gfx/BUILD.gn#newcode518
ui/gfx/BUILD.gn:518: "vector_icons/shelf_keyboard.1x.icon",
On 2016/06/09 19:24:48, James Cook wrote:
> Just a question for my edification: Why are there special 1x versions of the
> icons? Do they need special tuning (like TrueType used to do for small font
> sizes)?

Yes, that's right - some small icons need special tuning to look good at 1x.

[James Cook, 2016-06-09 23:00:09]

LGTM with nit

https://codereview.chromium.org/2051663005/diff/20001/ash/system/chromeos/vir...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/20001/ash/system/chromeos/vir...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:37:
button_->SetImage(views::CustomButton::STATE_NORMAL, &image_md);
see below

https://codereview.chromium.org/2051663005/diff/20001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/20001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:49: icon_->SetImage(&image_md);
Can you just call icon_->SetImage(image_id) to make it obvious that the SetImage
call won't do something bad with a pointer to a local variable?

(I think SetImage(const ImageSkia*) does the right thing here, but there's a
version that takes a const ImageSkia& that would be more clear.)

[sadrul, 2016-06-09 23:26:05]

lgtm

[tdanderson, 2016-06-09 23:51:41]

https://codereview.chromium.org/2051663005/diff/20001/ash/system/chromeos/vir...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/20001/ash/system/chromeos/vir...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:37:
button_->SetImage(views::CustomButton::STATE_NORMAL, &image_md);
On 2016/06/09 23:00:09, James Cook wrote:
> see below

In this case I'm keeping it as-is since ImageButton only has a version of
SetImage() that accepts a const ImageSkia* parameter (calls to
ImageButton::SetImage() elsewhere in the codebase seem to follow the same
pattern).

If you'd like, in a follow up CL I could try introducing an overloaded version
of SetImage() to ImageButton, or I could make |image_md| into a member variable
of VirtualKeyboardTray. Just let me know if you think either of those would be
worthwhile.

https://codereview.chromium.org/2051663005/diff/20001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/20001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:49: icon_->SetImage(&image_md);
On 2016/06/09 23:00:09, James Cook wrote:
> Can you just call icon_->SetImage(image_id) to make it obvious that the
SetImage
> call won't do something bad with a pointer to a local variable?
> 
> (I think SetImage(const ImageSkia*) does the right thing here, but there's a
> version that takes a const ImageSkia& that would be more clear.)

Done.

[tdanderson, 2016-06-09 23:51:45]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-06-09 23:51:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2051663005/#ps40001
(title: "nit addressed")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2051663005/40001

[James Cook, 2016-06-10 00:14:40]

https://codereview.chromium.org/2051663005/diff/20001/ash/system/chromeos/vir...
File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right):

https://codereview.chromium.org/2051663005/diff/20001/ash/system/chromeos/vir...
ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:37:
button_->SetImage(views::CustomButton::STATE_NORMAL, &image_md);
On 2016/06/09 23:51:41, tdanderson wrote:
> On 2016/06/09 23:00:09, James Cook wrote:
> > see below
> 
> In this case I'm keeping it as-is since ImageButton only has a version of
> SetImage() that accepts a const ImageSkia* parameter (calls to
> ImageButton::SetImage() elsewhere in the codebase seem to follow the same
> pattern).
> 
> If you'd like, in a follow up CL I could try introducing an overloaded version
> of SetImage() to ImageButton, or I could make |image_md| into a member
variable
> of VirtualKeyboardTray. Just let me know if you think either of those would be
> worthwhile.

That's OK, it's fine as it is. There seems to be a lot of variation in the code
base between const ImageSkia* and const ImageSkia&, but that's an issue for
another day.

[commit-bot: I haz the power, 2016-06-10 01:54:47]

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

[commit-bot: I haz the power, 2016-06-10 01:54:48]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[tdanderson, 2016-06-10 04:01:28]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2016-06-10 04:02:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2051663005/40001

[commit-bot: I haz the power, 2016-06-10 04:33:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-10 04:33:29]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 04:35:43]

Description was changed from

==========
Vectorize the remaining material design shelf icons

Create vectorized versions of the icons used
in the material design shelf:

* Overflow arrow
* Keyboard
* Notifications bell
* Overview mode

BUG=595015
TEST=manual
==========

to

==========
Vectorize the remaining material design shelf icons

Create vectorized versions of the icons used
in the material design shelf:

* Overflow arrow
* Keyboard
* Notifications bell
* Overview mode

BUG=595015
TEST=manual

Committed: https://crrev.com/4f0032c7bb4d9e4eba2180926e106284ff2bb58d
Cr-Commit-Position: refs/heads/master@{#399092}
==========

[commit-bot: I haz the power, 2016-06-10 04:35:44]

Patchset 3 (id:??) landed as
https://crrev.com/4f0032c7bb4d9e4eba2180926e106284ff2bb58d
Cr-Commit-Position: refs/heads/master@{#399092}