Vectorize LocationIconView icons

Vectorize LocationIconView icons

BUG=542443
Committed: https://crrev.com/3c7c6b17fdbecfee9b524c441a218eb741fddc0a
Cr-Commit-Position: refs/heads/master@{#358727}

[Evan Stade, 2015-10-20 21:20:32]

Description was changed from

==========
wip - vectorize locationiconview

BUG=
==========

to

==========
Vectorize LocationIconView icons

BUG=542443
==========

[Evan Stade, 2015-10-20 21:20:32]

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

[Evan Stade, 2015-10-20 21:21:28]

There should be an OWNERS file in components/toolbar/. Who belongs there? One of
you two want to volunteer?

https://codereview.chromium.org/1410293003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1410293003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:131: return
IDR_OMNIBOX_SEARCH_SECURED;
is it actually possible to hit this? How would one do it? It seems related to
instant but I can't see it on the ntp or on google.com.

[Peter Kasting, 2015-10-20 23:15:28]

It seems like this contains the changes in
https://codereview.chromium.org/1418543002 (or at least some of them)?

On 2015/10/20 21:21:28, Evan Stade wrote:
> There should be an OWNERS file in components/toolbar/. Who belongs there? One
of
> you two want to volunteer?

I can be there.

https://codereview.chromium.org/1410293003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1410293003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:131: return
IDR_OMNIBOX_SEARCH_SECURED;
On 2015/10/20 21:21:28, Evan Stade wrote:
> is it actually possible to hit this? How would one do it? It seems related to
> instant but I can't see it on the ntp or on http://google.com.

Search term replacement is dead and there's a bug on removing it entirely.

[Evan Stade, 2015-10-21 17:00:14]

added pkasting to components/toolbar/OWNERS
sky review only necessary for ui/gfx/BUILD.gn

https://codereview.chromium.org/1410293003/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1410293003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:131: return
IDR_OMNIBOX_SEARCH_SECURED;
On 2015/10/20 23:15:28, Peter Kasting wrote:
> On 2015/10/20 21:21:28, Evan Stade wrote:
> > is it actually possible to hit this? How would one do it? It seems related
to
> > instant but I can't see it on the ntp or on http://google.com.
> 
> Search term replacement is dead and there's a bug on removing it entirely.

guess i can delete this resource then

[Peter Kasting, 2015-10-21 19:45:48]

LGTM

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:130: return
GetIconForSecurityLevel(GetSecurityLevel(false));
I think GetIconForSecurityLevel() is now only called here, so why don't we just
combine these two functions into one.  Same with the new function you're adding.

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:171: return
gfx::VectorIconId::BUSINESS;
It's not at all obvious what this is from the name, and we're not using it for
anything relating to "business", so let's name the icon and enum
LOCATION_BAR_HTTPS_POLICY_WARNING to match the non-vector naming scheme and for
clarity.

(I assume you got this name from the internal icon repository, but I'd rather
optimize for readability here, and if it's necessary for maintainers to know
where the icon comes from, put a comment in the icon file saying "Note: this is
sourced from the internal "business" icon" or some similar explanation.

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1382:
gfx::kChromeIconGrey)
This has a similar problem to the one I mentioned in your previous icon review
where the grey is hardcoded and we should probably
BlendTowardOppositeLuminance() or the like depending on the omnibox background
color.

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:579: :
AutocompleteMatch::TypeToVectorIcon(match_.type);
I'd rather not touch this file just to change the formatting from one valid
method to another valid method (especially when the latter is less common for
omnibox code)

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.cc:89: model_.get() ?
model_->CurrentTextType()
Nit: .get() unneeded

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.cc:93: : id;
Honestly I'm not actually clear on what this last conditional is doing, maybe
add a comment?

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.h (right):

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.h:78: // FIXME
?

[sky, 2015-10-21 22:21:22]

BUILD.gn LGTM

[Evan Stade, 2015-10-21 23:20:02]

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:130: return
GetIconForSecurityLevel(GetSecurityLevel(false));
On 2015/10/21 19:45:47, Peter Kasting wrote:
> I think GetIconForSecurityLevel() is now only called here, so why don't we
just
> combine these two functions into one.  Same with the new function you're
adding.

Done.

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:171: return
gfx::VectorIconId::BUSINESS;
On 2015/10/21 19:45:47, Peter Kasting wrote:
> It's not at all obvious what this is from the name, and we're not using it for
> anything relating to "business", so let's name the icon and enum
> LOCATION_BAR_HTTPS_POLICY_WARNING to match the non-vector naming scheme and
for
> clarity.
> 
> (I assume you got this name from the internal icon repository, but I'd rather
> optimize for readability here, and if it's necessary for maintainers to know
> where the icon comes from, put a comment in the icon file saying "Note: this
is
> sourced from the internal "business" icon" or some similar explanation.

Yes, "business" is the name of this icon, which is straight from the MD icon
depot. I've been consistent in using the MD name for icons of this type. The
other icons here are actually specialized for this one UI surface, but this one
is not, the reason being that this icon should be very rarely visible.

Note that the IDR_OMNIBOX_ name is only an alias, it actually points at
controlled_setting_mandatory.png which at one point was used in many parts of
Chrome (seemingly this is the last usage, though). I anticipate BUSINESS will be
used in more than one place as well (for example, the origin info bubble, in
place of pageinfo_enterprise_managed.png). I guess you could view it as a
drawback that vector icons can't have these kinds of aliases, but I never really
considered them very useful anyway.

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1382:
gfx::kChromeIconGrey)
On 2015/10/21 19:45:47, Peter Kasting wrote:
> This has a similar problem to the one I mentioned in your previous icon review
> where the grey is hardcoded and we should probably
> BlendTowardOppositeLuminance() or the like depending on the omnibox background
> color.

maybe I should try to figure this one out before proceeding with the CL. The
tricky thing is that Sebastien wants to swap the green and white in the https
icon for dark themes (not something we could easily do automatically).

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:579: :
AutocompleteMatch::TypeToVectorIcon(match_.type);
On 2015/10/21 19:45:47, Peter Kasting wrote:
> I'd rather not touch this file just to change the formatting from one valid
> method to another valid method (especially when the latter is less common for
> omnibox code)

indeed, git cl format quirk. reverted

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.cc:89: model_.get() ?
model_->CurrentTextType()
On 2015/10/21 19:45:47, Peter Kasting wrote:
> Nit: .get() unneeded

Done.

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.cc:93: : id;
On 2015/10/21 19:45:47, Peter Kasting wrote:
> Honestly I'm not actually clear on what this last conditional is doing, maybe
> add a comment?

done

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.h (right):

https://codereview.chromium.org/1410293003/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.h:78: // FIXME
On 2015/10/21 19:45:47, Peter Kasting wrote:
> ?

fixed

[Peter Kasting, 2015-10-22 00:05:24]

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:171: return
gfx::VectorIconId::BUSINESS;
On 2015/10/21 23:20:01, Evan Stade wrote:
> On 2015/10/21 19:45:47, Peter Kasting wrote:
> > It's not at all obvious what this is from the name, and we're not using it
for
> > anything relating to "business", so let's name the icon and enum
> > LOCATION_BAR_HTTPS_POLICY_WARNING to match the non-vector naming scheme and
> for
> > clarity.
> > 
> > (I assume you got this name from the internal icon repository, but I'd
rather
> > optimize for readability here, and if it's necessary for maintainers to know
> > where the icon comes from, put a comment in the icon file saying "Note: this
> is
> > sourced from the internal "business" icon" or some similar explanation.
> 
> Yes, "business" is the name of this icon, which is straight from the MD icon
> depot. I've been consistent in using the MD name for icons of this type. The
> other icons here are actually specialized for this one UI surface, but this
one
> is not, the reason being that this icon should be very rarely visible.
> 
> Note that the IDR_OMNIBOX_ name is only an alias, it actually points at
> controlled_setting_mandatory.png which at one point was used in many parts of
> Chrome (seemingly this is the last usage, though). I anticipate BUSINESS will
be
> used in more than one place as well (for example, the origin info bubble, in
> place of pageinfo_enterprise_managed.png). I guess you could view it as a
> drawback that vector icons can't have these kinds of aliases, but I never
really
> considered them very useful anyway.

FWIW, I'd like to rename all the icons to match their uses in Chrome, not just
this one.  Most maintainers here will be much more familiar with Chrome code and
practices than with the MD icon depot, which basically no one will have ever
looked at.  I like being able to tie icons back to the MD repo -- which is why
I'd be glad to see us put a comment in the icon file indicating its origin --
but the only people who should need those are people who want to update the icon
itself, for which the comment seems like the right combination of "gives the
info to the relevant reader" without "also gives it to everyone else who doesn't
care".

(Actually, putting such comments in the icon files makes sense anyway, because
in most cases today it's not really clear from the icon file whether it's an
unchanged copy of the MD icon, an updated copy tweaked by the designer, wholly
our own invention, etc.  One or two of the icons say this but most don't.)

For icons used in more than one place, like this one might eventually be, it's
OK if we don't have aliases and thus can't name this "omnibox".  But we should
at least pick a name that has some more meaning than "business".  As a reader
this is almost like if you named the bookmark star icon "cabinet".  Not only
does it appear to have nothing to do with the code, it's also a generic word
that seems to say nothing about the icon itself.

[Evan Stade, 2015-11-06 00:15:21]

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1410293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1382:
gfx::kChromeIconGrey)
On 2015/10/21 23:20:01, Evan Stade wrote:
> On 2015/10/21 19:45:47, Peter Kasting wrote:
> > This has a similar problem to the one I mentioned in your previous icon
review
> > where the grey is hardcoded and we should probably
> > BlendTowardOppositeLuminance() or the like depending on the omnibox
background
> > color.
> 
> maybe I should try to figure this one out before proceeding with the CL. The
> tricky thing is that Sebastien wants to swap the green and white in the https
> icon for dark themes (not something we could easily do automatically).

After looking at this a little more, I believe we'll want a strategy where we
pick from one of two color schemes rather than picking one color/set of colors
and somehow modifying based on the background. Sebastien provided the color to
use for a white bg and for a black bg, so I think the code should do something
like "pick whichever of these two colors has greater contrast to the background"
(perhaps after that we can modify to make sure it's readable, if we care about
supporting random backgrounds like green).

This encompasses location bar chips and other text in the location bar, so I
think it can be punted for now until we address all of that simultaneously (I
gather we'll have to do that soon to support dark bg for incognito).

tldr I assume the LGs are still good and I'll just rebase and land this as is.

[Evan Stade, 2015-11-06 00:30:09]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-06 00:30:09]

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

[commit-bot: I haz the power, 2015-11-06 00:32:25]

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

[commit-bot: I haz the power, 2015-11-06 00:43:43]

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

[commit-bot: I haz the power, 2015-11-06 00:43:44]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2015-11-07 01:23:23]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-07 01:23:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1410293003/#ps140001
(title: "fix bad merge")

[commit-bot: I haz the power, 2015-11-07 01:24:34]

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

[commit-bot: I haz the power, 2015-11-07 01:35:28]

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

[commit-bot: I haz the power, 2015-11-07 01:35:29]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2015-11-09 23:53:39]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-09 23:53:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1410293003/#ps180001
(title: "fix android")

[commit-bot: I haz the power, 2015-11-09 23:54:18]

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

[commit-bot: I haz the power, 2015-11-10 01:25:47]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-10 01:26:55]

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