For some vector icons, get the size from the vector definition.

For some vector icons, get the size from the vector definition.

Sometimes various icons intended to be shown in the same place in the UI have different dimensions in their .icon definition, so those call sites can't be updated. For example, GetTabAlertIndicatorImage.

This patch is probably not comprehensive but it covers many CreateVectorIcon cases that are possible to update.

BUG=505953
Committed: https://crrev.com/3b7f55d085ee8bb37b7124d18778a7b24428c1ff
Cr-Commit-Position: refs/heads/master@{#389952}

[Evan Stade, 2015-10-13 19:20:00]

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

[Evan Stade, 2015-10-13 19:20:00]

As previously discussed, I think it makes sense to treat the primary UI icons
differently from the secondary ones. Basically, any icon with a 1x.icon variant
counts as "primary".

[sky, 2015-10-13 20:15:06]

+pkasting
LGTM, but wait for Peter to be happy (he may have strong opinions on the name).

https://codereview.chromium.org/1406613002/diff/1/ui/gfx/paint_vector_icon.cc
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/1406613002/diff/1/ui/gfx/paint_vector_icon.cc...
ui/gfx/paint_vector_icon.cc:399: #ifndef NDEBUG
nit: if !defined(NDEBUG)

[Evan Stade, 2015-10-14 20:59:21]

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

[Evan Stade, 2015-10-14 20:59:21]

+pkasting

[Peter Kasting, 2015-10-14 23:39:04]

I don't think introducing the concept of a "primary" versus "secondary" icon
into the codebase is a good idea.  It's hard for me to understand the
distinction and I can't think of a benefit for the caller in doing so.

I don't see what we lose by simply having two versions of the creation function,
one which forces a size and one which infers the size.  This is what I'm
planning to implement per the thread where you said you wanted to "see this
change".  If you want to partially implement this by making a change sort of
like the one here except omitting terminology like "primary" and then let me
make the rest of the change to convert as many other icons over as possible as
well, I'm fine with that.

[Evan Stade, 2015-10-14 23:44:46]

On 2015/10/14 23:39:04, Peter Kasting wrote:
> I don't think introducing the concept of a "primary" versus "secondary" icon
> into the codebase is a good idea.  It's hard for me to understand the
> distinction and I can't think of a benefit for the caller in doing so.

"primary" is one that Sebastien cares about enough to create a separate 1x
specification which assumes a particular rendering size.

> 
> I don't see what we lose by simply having two versions of the creation
function,
> one which forces a size and one which infers the size.  This is what I'm
> planning to implement per the thread where you said you wanted to "see this
> change".  If you want to partially implement this by making a change sort of
> like the one here except omitting terminology like "primary" and then let me
> make the rest of the change to convert as many other icons over as possible as
> well, I'm fine with that.

Nope, I still disagree that the secondary icons want this change. I would be
willing to omit "primary" but I would not be willing to drop the 1x/2x DCHECK.

[Peter Kasting, 2015-10-15 00:05:54]

On 2015/10/14 23:44:46, Evan Stade wrote:
> Nope, I still disagree that the secondary icons want this change. I would be
> willing to omit "primary" but I would not be willing to drop the 1x/2x DCHECK.

You already told me you were willing to see what the change looked like, and I
told you I'd do it.  I don't know what's changed since then.  But in that entire
original conversation, the idea of distinguishing icons with "1x versions" in
terms of whether this functionality would be available didn't come up.  So I'm
not sure where it's coming from now.

Both sky and I wanted to see the change made for all icons, not just some icons.
 If you're not willing to write it that's fine, but in that case I'll write it
and we can take a look once it's done.

Incidentally, the rules on what icons Sebastien has created a 1x version for are
completely arbitrary, and they don't even necessarily match what's in our tree. 
In all the tab and tabstrip work I've been doing, for example, I've had a
complete mixture of 1x-only, 2x-only, 1x-and-2x, and "sometimes one and
sometimes the other based on what I asked when".  In the end for the tab close
buttons I did I made the decision to put in 1x versions purely because I thought
that was the general pattern we were following.  I don't believe Sebastien has a
product distinction in his head for these cases, and if there is one it's never
been mentioned to me, nor have I followed it.

So if you're basing something on that, I think you're basing it on a fictitious
distinction.

[Evan Stade, 2015-10-20 00:29:26]

On 2015/10/15 00:05:54, Peter Kasting wrote:
> On 2015/10/14 23:44:46, Evan Stade wrote:
> > Nope, I still disagree that the secondary icons want this change. I would be
> > willing to omit "primary" but I would not be willing to drop the 1x/2x
DCHECK.
> 
> You already told me you were willing to see what the change looked like, and I
> told you I'd do it.  I don't know what's changed since then.  But in that
entire
> original conversation, the idea of distinguishing icons with "1x versions" in
> terms of whether this functionality would be available didn't come up.  So I'm
> not sure where it's coming from now.

Yes, and I'm still willing. In the previous conversation, I said:

"""
One place where I do think it makes sense to not pass a size in C++ is for any
.icon that has a .1x.icon version, because those are hand-tweaked for display at
a very specific size, and we probably can't scale them well.
"""

so that's what I was referencing.

> 
> Both sky and I wanted to see the change made for all icons, not just some
icons.
>  If you're not willing to write it that's fine, but in that case I'll write it
> and we can take a look once it's done.
> 
> Incidentally, the rules on what icons Sebastien has created a 1x version for
are
> completely arbitrary, and they don't even necessarily match what's in our
tree. 
> In all the tab and tabstrip work I've been doing, for example, I've had a
> complete mixture of 1x-only, 2x-only, 1x-and-2x, and "sometimes one and
> sometimes the other based on what I asked when".  In the end for the tab close
> buttons I did I made the decision to put in 1x versions purely because I
thought
> that was the general pattern we were following.  I don't believe Sebastien has
a
> product distinction in his head for these cases, and if there is one it's
never
> been mentioned to me, nor have I followed it.

In that case you should ask him about it. "Primary" is his term, not mine. I've
talked with him extensively on this subject, for example, here are some of his
words from a chat earlier today (in reference to why the omnibox autocomplete
result icons are not just copies of the go/icons ones):

"""
these ones, especially in 1x need special treatment on a 16pt based grid
because they are small and highly visible
"""

The key is "small and highly visible". For the tab close buttons, and all other
icons, if the 1x and 2x are identical modulo a constant factor, the pattern is
to have no 1x version. But tab close buttons are small and highly visible so
they probably do need special 1x icons. On my local branch I count 56 icons with
no 1x version and 21 with (most icons are not part of the highly visible
"primary" ui).

> 
> So if you're basing something on that, I think you're basing it on a
fictitious
> distinction.

[Peter Kasting, 2015-10-20 00:51:54]

Then let's put this on hold pending the other change, since if that goes
through, we won't need this.  If we end up ditching that, then maybe this makes
more sense, but I'm still really uncomfortable with the concept of "primary",
since it seems hard to define and hard for me to understand.  "Small and highly
visible" seems like a rather fuzzy, arbitrary criteria.  What would be small and
not highly visible?  What would be large and highly visible?  It seems like we
may have icons that are small and highly visible but have no 1x version because
the general version already works fine in 1x, and in theory possibly icons that
aren't both "small and highly visible" but which have 1x versions because they
look better.

[Evan Stade, 2015-10-20 01:02:11]

On 2015/10/20 00:51:54, Peter Kasting wrote:
> Then let's put this on hold pending the other change, since if that goes
> through, we won't need this.

OK. I think this patch gets us most of the way to the other change because there
aren't a ton more places that have the option of not specifying a size. But it
can certainly wait.

> What would be small and not highly visible? 

Something buried deep in an obscure dialog or rarely seen, such as an icon in
the collected cookies dialog or the default icon for extension items in the
renderer context menu.

> What would be large and highly visible? 

The downward thrusting arrow that trumpets the arrival of the download shelf,
the notification center icons (on CrOS), etc. It's true small is much more
common than large, but small is an important detail because pixel alignment is
more noticeable on smaller icons.

[Evan Stade, 2015-12-10 23:42:13]

ping on this. If you don't foresee having time to work on your change in the
near future, may I suggest we reconsider this CL?

[Peter Kasting, 2015-12-10 23:49:11]

On 2015/12/10 23:42:13, Evan Stade wrote:
> ping on this. If you don't foresee having time to work on your change in the
> near future, may I suggest we reconsider this CL?

I can try to make time for the other change.

[Evan Stade, 2016-01-28 05:07:01]

On 2015/12/10 23:49:11, Peter Kasting wrote:
> On 2015/12/10 23:42:13, Evan Stade wrote:
> > ping on this. If you don't foresee having time to work on your change in the
> > near future, may I suggest we reconsider this CL?
> 
> I can try to make time for the other change.

ping?

[Peter Kasting, 2016-01-28 05:19:01]

On 2016/01/28 05:07:01, Evan Stade wrote:
> On 2015/12/10 23:49:11, Peter Kasting wrote:
> > On 2015/12/10 23:42:13, Evan Stade wrote:
> > > ping on this. If you don't foresee having time to work on your change in
the
> > > near future, may I suggest we reconsider this CL?
> > 
> > I can try to make time for the other change.
> 
> ping?

I know, I've been thinking about this pretty much every week and feeling bad
that I haven't done it yet :(

I'm nearly done with what MD work I have that seems to affect the quality of the
shipped product, and this is next on the list to happen after that.  Basically I
just have stacked tabs, unittests, the layout tweaks to the EV bubble, and
trying to verify that the tabstrip's drawing performance hasn't been
meaningfully impacted.

Since changing this will involve updating the vector icon definitions to use
whatever scale they're actually drawn at, I had hoped at the same time to go
through the actual image definitions and modify them as I did with the SVGs
Sebastien sent me for the tabstrip, where I removed duplicate and degenerate
segments and adjusted things to be perfectly symmetrical, use
precisely-representable floating point values, etc.  But doing that would
increase the time necessary to implement this, so that will probably get
backburnered.

[Evan Stade, 2016-04-20 21:32:05]

On 2016/01/28 05:19:01, Peter Kasting wrote:
> On 2016/01/28 05:07:01, Evan Stade wrote:
> > On 2015/12/10 23:49:11, Peter Kasting wrote:
> > > On 2015/12/10 23:42:13, Evan Stade wrote:
> > > > ping on this. If you don't foresee having time to work on your change in
> the
> > > > near future, may I suggest we reconsider this CL?
> > > 
> > > I can try to make time for the other change.
> > 
> > ping?
> 
> I know, I've been thinking about this pretty much every week and feeling bad
> that I haven't done it yet :(
> 
> I'm nearly done with what MD work I have that seems to affect the quality of
the
> shipped product, and this is next on the list to happen after that.  Basically
I
> just have stacked tabs, unittests, the layout tweaks to the EV bubble, and
> trying to verify that the tabstrip's drawing performance hasn't been
> meaningfully impacted.
> 
> Since changing this will involve updating the vector icon definitions to use
> whatever scale they're actually drawn at, I had hoped at the same time to go
> through the actual image definitions and modify them as I did with the SVGs
> Sebastien sent me for the tabstrip, where I removed duplicate and degenerate
> segments and adjusted things to be perfectly symmetrical, use
> precisely-representable floating point values, etc.  But doing that would
> increase the time necessary to implement this, so that will probably get
> backburnered.

How is this looking? I think this CL is an improvement, so landing it would be
nice if you don't have time in the foreseeable future for working on the
alternative. Then I would feel good about writing up a howto (people have been
asking me how to make/use vector icons), which is a task I've been holding off
on as I didn't want to write something that was going to change.

[Peter Kasting, 2016-04-21 23:06:04]

The only thing I'm reluctant on here is the terminology "primary UI icon".  It
seems like the concept of "very often visible in the UI" that the .h comment
mentions isn't something the vector icon code can know, or indeed needs to know.

The important bits to the vector layer are that the size is computed from the
vector icon definition itself, and possibly that it has an explicit 1x
representation.  So I would probably restrict the name/comments to talking about
that stuff.

CreateInstrinsicallySizedVectorIcon()?
CreateVectorIconWith1xRepresentation()?
Just CreateVectorIcon(), and talk about the other stuff in the comment?

The rest of the change is a plus and we should land some form of it.

[Evan Stade, 2016-04-25 22:31:30]

On 2016/04/21 23:06:04, Peter Kasting wrote:
> Just CreateVectorIcon(), and talk about the other stuff in the comment?

I like this option best.

> The rest of the change is a plus and we should land some form of it.

great, I'll work on it soon.

[Evan Stade, 2016-04-26 16:58:08]

Description was changed from

==========
For "primary UI" vector icons, get the size from the vector definition.

These are carefully crafted for display at a certain size, so we can't
reuse them at multiple sizes. Hence, it makes sense not to allow/require
specifying the size at the call site.

BUG=505953
==========

to

==========
For some vector icons, get the size from the vector definition.

Sometimes various icons intended to be shown in the same place in the UI have
different dimensions in their .icon definition, so those call sites can't be
updated. For example, GetTabAlertIndicatorImage.

This patch is probably not comprehensive but it covers many CreateVectorIcon
cases that are possible to update.

BUG=505953
==========

[Evan Stade, 2016-04-26 16:58:29]

ptal, also note updated CL description.

https://codereview.chromium.org/1406613002/diff/20001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_image_util.cc (left):

https://codereview.chromium.org/1406613002/diff/20001/ui/views/controls/menu/...
ui/views/controls/menu/menu_image_util.cc:15: return
gfx::CreateVectorIcon(gfx::VectorIconId::MENU_CHECK, kMenuCheckSize,
In some places we aren't actually getting rid of a constant because it's still
needed elsewhere. In these cases, we still have the constant expressed multiple
times, and the only difference is that if the constants diverge, the error
behavior will be "icon is wrong size" instead of "icon is scaled imperfectly".

[Peter Kasting, 2016-04-26 20:16:39]

LGTM, but it looks like the set of affected callsites changed significantly
between the two patch sets?  Were some of the callsites in PS1 not actually
suitable for removing the constant?

[Evan Stade, 2016-04-26 22:52:04]

On 2016/04/26 20:16:39, Peter Kasting wrote:
> LGTM, but it looks like the set of affected callsites changed significantly
> between the two patch sets?  Were some of the callsites in PS1 not actually
> suitable for removing the constant?

A lot of them got refactored/consolidated (e.g. into BarControlButton)

[Evan Stade, 2016-04-26 22:54:06]

Scott, did you want to take a look at the new patchset? It's not much changed
from the first one but 6 months have elapsed so perhaps your original lg has
expired.

[sky, 2016-04-26 23:41:36]

SLGTM

[Evan Stade, 2016-04-26 23:42:04]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-04-26 23:42:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1406613002/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-26 23:42:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406613002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406613002/20001

[commit-bot: I haz the power, 2016-04-27 00:21:32]

Description was changed from

==========
For some vector icons, get the size from the vector definition.

Sometimes various icons intended to be shown in the same place in the UI have
different dimensions in their .icon definition, so those call sites can't be
updated. For example, GetTabAlertIndicatorImage.

This patch is probably not comprehensive but it covers many CreateVectorIcon
cases that are possible to update.

BUG=505953
==========

to

==========
For some vector icons, get the size from the vector definition.

Sometimes various icons intended to be shown in the same place in the UI have
different dimensions in their .icon definition, so those call sites can't be
updated. For example, GetTabAlertIndicatorImage.

This patch is probably not comprehensive but it covers many CreateVectorIcon
cases that are possible to update.

BUG=505953
==========

[commit-bot: I haz the power, 2016-04-27 00:21:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-27 00:22:43]

Description was changed from

==========
For some vector icons, get the size from the vector definition.

Sometimes various icons intended to be shown in the same place in the UI have
different dimensions in their .icon definition, so those call sites can't be
updated. For example, GetTabAlertIndicatorImage.

This patch is probably not comprehensive but it covers many CreateVectorIcon
cases that are possible to update.

BUG=505953
==========

to

==========
For some vector icons, get the size from the vector definition.

Sometimes various icons intended to be shown in the same place in the UI have
different dimensions in their .icon definition, so those call sites can't be
updated. For example, GetTabAlertIndicatorImage.

This patch is probably not comprehensive but it covers many CreateVectorIcon
cases that are possible to update.

BUG=505953

Committed: https://crrev.com/3b7f55d085ee8bb37b7124d18778a7b24428c1ff
Cr-Commit-Position: refs/heads/master@{#389952}
==========

[commit-bot: I haz the power, 2016-04-27 00:22:45]

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