Use vector icon for Ash shelf launcher

Use vector icon for Ash shelf launcher

Introduce new vector icons for the launcher in
the material design shelf, which is currently
off by default behind --ash-md. This CL also
adds support for drawing elliptical arcs in
.icon files, which are needed for this
particular icon.

BUG=595015, 605643, 617289
TEST=manual

Committed: https://crrev.com/aa185fbd0b0fcdf687f3752c41b3f6ef88380137
Cr-Commit-Position: refs/heads/master@{#398821}

[tdanderson, 2016-05-23 21:18:24]

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

[tdanderson, 2016-05-23 21:18:24]

Evan, can you please take a first look (and also see my inline comment)?

https://codereview.chromium.org/2002293002/diff/20001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2002293002/diff/20001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:29: const int kLauncherIconSize = 18;
So if I remember our conversation correctly, the vectorization process bakes
some padding into the .icon, and there is no easy way to strip this out or add
to it. In this case, I wanted the search icon to appear as 14px (length from one
side of the magnifying glass to the other without any padding) so I had to play
around with sizes to determine that using 18 would give me a 14px magnifying
glass + 2px baked-in padding to the left + 2px of baked-in padding to the right.
Does this sound like what your workflow would be, or am I missing something?

[Evan Stade, 2016-05-23 22:11:58]

omnibox_search.icon probably shouldn't be renamed because it's not the same as
https://icons.googleplex.com/#icon=ic_search (and the naming convention of
matching the md icon name would suggest that it is identical). The difference is
in the padding. I don't think there's been a case so far where we had to deal
with the same icon wanting two different levels of padding.

https://codereview.chromium.org/2002293002/diff/20001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2002293002/diff/20001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:29: const int kLauncherIconSize = 18;
On 2016/05/23 21:18:24, tdanderson wrote:
> So if I remember our conversation correctly, the vectorization process bakes
> some padding into the .icon, and there is no easy way to strip this out or add
> to it. In this case, I wanted the search icon to appear as 14px (length from
one
> side of the magnifying glass to the other without any padding) so I had to
play
> around with sizes to determine that using 18 would give me a 14px magnifying
> glass + 2px baked-in padding to the left + 2px of baked-in padding to the
right.
> Does this sound like what your workflow would be, or am I missing something?

Overall, yes. One way to add to the padding would be to set an empty border on a
views::ImageView. In this case, the centering code you have looks reasonable.

[tdanderson, 2016-05-23 23:14:08]

On 2016/05/23 22:11:58, Evan Stade wrote:
> omnibox_search.icon probably shouldn't be renamed because it's not the same as
> https://icons.googleplex.com/#icon=ic_search (and the naming convention of
> matching the md icon name would suggest that it is identical). The difference
is
> in the padding. I don't think there's been a case so far where we had to deal
> with the same icon wanting two different levels of padding.

As per offline conversation, I will not rename this file.

> 
>
https://codereview.chromium.org/2002293002/diff/20001/ash/shelf/app_list_butt...
> File ash/shelf/app_list_button.cc (right):
> 
>
https://codereview.chromium.org/2002293002/diff/20001/ash/shelf/app_list_butt...
> ash/shelf/app_list_button.cc:29: const int kLauncherIconSize = 18;
> On 2016/05/23 21:18:24, tdanderson wrote:
> > So if I remember our conversation correctly, the vectorization process bakes
> > some padding into the .icon, and there is no easy way to strip this out or
add
> > to it. In this case, I wanted the search icon to appear as 14px (length from
> one
> > side of the magnifying glass to the other without any padding) so I had to
> play
> > around with sizes to determine that using 18 would give me a 14px magnifying
> > glass + 2px baked-in padding to the left + 2px of baked-in padding to the
> right.
> > Does this sound like what your workflow would be, or am I missing something?
> 
> Overall, yes. One way to add to the padding would be to set an empty border on
a
> views::ImageView. In this case, the centering code you have looks reasonable.

[tdanderson, 2016-05-23 23:15:17]

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

[tdanderson, 2016-05-23 23:15:17]

oshima@, can you please take a look?

[oshima, 2016-05-23 23:41:33]

https://codereview.chromium.org/2002293002/diff/40001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2002293002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:130:
gfx::CreateVectorIcon(gfx::VectorIconId::OMNIBOX_SEARCH,
This conceptually creates chrome -> ash dependency, doesn't it?

[Evan Stade, 2016-05-23 23:51:34]

https://codereview.chromium.org/2002293002/diff/40001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/2002293002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.cc:130:
gfx::CreateVectorIcon(gfx::VectorIconId::OMNIBOX_SEARCH,
On 2016/05/23 23:41:33, oshima wrote:
> This conceptually creates chrome -> ash dependency, doesn't it?

I agree this is not perfect. Technically ui/ already contains the word
"omnibox", and this is relying on something ui/ knows about. But omnibox it's
just a word, and I can't think of a better name for this. The full description
would be "SEARCH ICON WITH PADDING CHOSEN FOR OMNIBOX". I don't think it should
be called SEARCH for reasons mentioned upthread.

[tdanderson, 2016-05-24 21:45:20]

Description was changed from

==========
Use vector icon for Ash shelf launcher

Use the already-vectorized search icon for the
Ash shelf launcher, and delete the unused
PNG assets. Also rename the omnibox_search.icon
vector icon file to the more generic name of
search.icon.

BUG=595015,605643
TEST=manual
==========

to

==========
Use vector icon for Ash shelf launcher

Use the already-vectorized search icon for the
Ash shelf launcher, and delete the unused
PNG assets. Also rename the omnibox_search.icon
vector icon file to the more generic name of
search.icon.

BUG=595015,605643
TEST=manual
==========

[oshima, 2016-05-26 22:09:55]

On 2016/05/23 23:51:34, Evan Stade wrote:
>
https://codereview.chromium.org/2002293002/diff/40001/ash/shelf/app_list_butt...
> File ash/shelf/app_list_button.cc (right):
> 
>
https://codereview.chromium.org/2002293002/diff/40001/ash/shelf/app_list_butt...
> ash/shelf/app_list_button.cc:130:
> gfx::CreateVectorIcon(gfx::VectorIconId::OMNIBOX_SEARCH,
> On 2016/05/23 23:41:33, oshima wrote:
> > This conceptually creates chrome -> ash dependency, doesn't it?
> 
> I agree this is not perfect. Technically ui/ already contains the word
> "omnibox", and this is relying on something ui/ knows about. But omnibox it's
> just a word, and I can't think of a better name for this. The full description
> would be "SEARCH ICON WITH PADDING CHOSEN FOR OMNIBOX". I don't think it
should
> be called SEARCH for reasons mentioned upthread.

We've been trying to fix ui -> chrome dependency issue, so it's sad to see we're
going back.
I don't want to block it, so lgtm, but with mus world, chrome, apps, ash (and
potentially wm) will be separate clients, so we should have a plan to make it
cleaner.

[Evan Stade, 2016-06-02 00:06:15]

lgtm

[tdanderson, 2016-06-06 16:45:22]

Description was changed from

==========
Use vector icon for Ash shelf launcher

Use the already-vectorized search icon for the
Ash shelf launcher, and delete the unused
PNG assets. Also rename the omnibox_search.icon
vector icon file to the more generic name of
search.icon.

BUG=595015,605643
TEST=manual
==========

to

==========
Use vector icon for Ash shelf launcher

Introduce new vector icons for the launcher in
the material design shelf, which is currently
off by default behind --ash-md. This CL also
adds support for drawing elliptical arcs in
.icon files, which are needed for this
particular icon.

BUG=595015,605643,617289
TEST=manual
==========

[tdanderson, 2016-06-06 16:47:17]

Oshima and Evan, please take another look. After working with design I'm
introducing new .icon files and adding support for drawing elliptical arcs.

[tdanderson, 2016-06-07 14:04:49]

On 2016/06/06 16:47:17, tdanderson wrote:
> Oshima and Evan, please take another look. After working with design I'm
> introducing new .icon files and adding support for drawing elliptical arcs.

oshima->jamescook who is now in the ash/ owners file and will be doing the MD
shelf reviews

[tdanderson, 2016-06-07 14:05:11]

tdanderson@chromium.org changed reviewers:
+ jamescook@chromium.org
- oshima@chromium.org

[tdanderson, 2016-06-07 14:05:11]

James, can you please take a look?

[James Cook, 2016-06-07 15:51:52]

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
File ash/common/shelf/shelf_constants.cc (right):

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
ash/common/shelf/shelf_constants.cc:16: const SkColor kShelfIconColor =
SK_ColorWHITE;
nit: Either no blank line above, for consistency, or space them all out.

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
File ash/common/shelf/shelf_constants.h (right):

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
ash/common/shelf/shelf_constants.h:33: // The color of the icons used in the
shelf (launcher, notifications, etc).
nit: foreground color? background color?

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/shelf/app_li...
File ash/shelf/app_list_button.cc (right):

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/shelf/app_li...
ash/shelf/app_list_button.cc:119: const gfx::ImageSkia& forground_image =
nit: for_e_ground

Can you also fix forground_bounds while you're here?

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/shelf/app_li...
ash/shelf/app_list_button.cc:121: ?
CreateVectorIcon(gfx::VectorIconId::SHELF_LAUNCHER, kShelfIconColor)
Given that the class is called AppList, and the legacy icons are called APPLIST,
and there's a bunch of code in ui/app_list, do you really want the new icon id
to be LAUNCHER?

[tdanderson, 2016-06-08 17:29:26]

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

[tdanderson, 2016-06-08 17:29:27]

James, your comments have been addressed, please take another look.

Sadrul, looks like Evan is OOO this week. Can you please take a look at the
changes in ui/gfx ?

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
File ash/common/shelf/shelf_constants.cc (right):

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
ash/common/shelf/shelf_constants.cc:16: const SkColor kShelfIconColor =
SK_ColorWHITE;
On 2016/06/07 15:51:52, James Cook wrote:
> nit: Either no blank line above, for consistency, or space them all out.

Done.

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
File ash/common/shelf/shelf_constants.h (right):

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/common/shelf...
ash/common/shelf/shelf_constants.h:33: // The color of the icons used in the
shelf (launcher, notifications, etc).
On 2016/06/07 15:51:52, James Cook wrote:
> nit: foreground color? background color?

Done.

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/shelf/app_li...
File ash/shelf/app_list_button.cc (right):

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/shelf/app_li...
ash/shelf/app_list_button.cc:119: const gfx::ImageSkia& forground_image =
On 2016/06/07 15:51:52, James Cook wrote:
> nit: for_e_ground
> 
> Can you also fix forground_bounds while you're here?

Done.

https://chromiumcodereview.appspot.com/2002293002/diff/60001/ash/shelf/app_li...
ash/shelf/app_list_button.cc:121: ?
CreateVectorIcon(gfx::VectorIconId::SHELF_LAUNCHER, kShelfIconColor)
On 2016/06/07 15:51:52, James Cook wrote:
> Given that the class is called AppList, and the legacy icons are called
APPLIST,
> and there's a bunch of code in ui/app_list, do you really want the new icon id
> to be LAUNCHER?

No, I guess I don't. Changed.

[sadrul, 2016-06-08 17:41:47]

lgtm

[James Cook, 2016-06-08 18:05:00]

lgtm

[tdanderson, 2016-06-08 23:38:24]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-06-08 23:38:24]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
estade@chromium.org, sadrul@chromium.org, jamescook@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2002293002/#ps100001 (title: "rebase")

[commit-bot: I haz the power, 2016-06-08 23:38:51]

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

[commit-bot: I haz the power, 2016-06-09 00:58:26]

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

[commit-bot: I haz the power, 2016-06-09 00:58:27]

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

[tdanderson, 2016-06-09 01:40:59]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2016-06-09 01:41:21]

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

[commit-bot: I haz the power, 2016-06-09 04:29:56]

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

[commit-bot: I haz the power, 2016-06-09 04:29:57]

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

[tdanderson, 2016-06-09 08:42:42]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2016-06-09 08:43:00]

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

[commit-bot: I haz the power, 2016-06-09 09:18:27]

Description was changed from

==========
Use vector icon for Ash shelf launcher

Introduce new vector icons for the launcher in
the material design shelf, which is currently
off by default behind --ash-md. This CL also
adds support for drawing elliptical arcs in
.icon files, which are needed for this
particular icon.

BUG=595015,605643,617289
TEST=manual
==========

to

==========
Use vector icon for Ash shelf launcher

Introduce new vector icons for the launcher in
the material design shelf, which is currently
off by default behind --ash-md. This CL also
adds support for drawing elliptical arcs in
.icon files, which are needed for this
particular icon.

BUG=595015,605643,617289
TEST=manual
==========

[commit-bot: I haz the power, 2016-06-09 09:18:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-09 09:19:43]

Description was changed from

==========
Use vector icon for Ash shelf launcher

Introduce new vector icons for the launcher in
the material design shelf, which is currently
off by default behind --ash-md. This CL also
adds support for drawing elliptical arcs in
.icon files, which are needed for this
particular icon.

BUG=595015,605643,617289
TEST=manual
==========

to

==========
Use vector icon for Ash shelf launcher

Introduce new vector icons for the launcher in
the material design shelf, which is currently
off by default behind --ash-md. This CL also
adds support for drawing elliptical arcs in
.icon files, which are needed for this
particular icon.

BUG=595015,605643,617289
TEST=manual

Committed: https://crrev.com/aa185fbd0b0fcdf687f3752c41b3f6ef88380137
Cr-Commit-Position: refs/heads/master@{#398821}
==========

[commit-bot: I haz the power, 2016-06-09 09:19:44]

Patchset 6 (id:??) landed as
https://crrev.com/aa185fbd0b0fcdf687f3752c41b3f6ef88380137
Cr-Commit-Position: refs/heads/master@{#398821}

[Evan Stade, 2016-06-10 20:42:22]

https://chromiumcodereview.appspot.com/2002293002/diff/100001/ui/gfx/paint_ve...
File ui/gfx/paint_vector_icon.cc (right):

https://chromiumcodereview.appspot.com/2002293002/diff/100001/ui/gfx/paint_ve...
ui/gfx/paint_vector_icon.cc:161: arc_sweep_flag ? SkPath::kCW_Direction :
SkPath::kCCW_Direction,
dang this arc thing is a crazy command.

Can we share the crazy between R_ARC_TO and ARC_TO? Just split on the function
that's called (using fn ptrs?)

[tdanderson, 2016-06-15 16:15:41]

On 2016/06/10 20:42:22, Evan Stade wrote:
>
https://chromiumcodereview.appspot.com/2002293002/diff/100001/ui/gfx/paint_ve...
> File ui/gfx/paint_vector_icon.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2002293002/diff/100001/ui/gfx/paint_ve...
> ui/gfx/paint_vector_icon.cc:161: arc_sweep_flag ? SkPath::kCW_Direction :
> SkPath::kCCW_Direction,
> dang this arc thing is a crazy command.
> 
> Can we share the crazy between R_ARC_TO and ARC_TO? Just split on the function
> that's called (using fn ptrs?)

Evan, yes I can do this: crbug.com/620345 to track