New origin security indicator icons.

New origin security indicator icons.

For the Omnibox.

BUG=604520
Committed: https://crrev.com/c354e28254ce755f07053d1711f8b1d47e18fc6d
Cr-Commit-Position: refs/heads/master@{#395495}

[palmer, 2016-05-02 20:53:34]

palmer@chromium.org changed reviewers:
+ estade@chromium.org, felt@chromium.org

[palmer, 2016-05-02 20:53:35]

estade: As an OWNER
felt: FYI

I've posted screenshots of how it looks on the bug.

[Evan Stade, 2016-05-04 21:29:26]

is this still correct? [1]
nit: can you update [2] since the difference between omnibox and location bar is
now a bit more than a "tweak"

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/components/toolbar...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox...

[palmer, 2016-05-04 21:33:01]

> is this still correct? [1]
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/toolbar...

Yes, we are still surfacing Dubious as Neutral. That is the new status quo.

> nit: can you update [2] since the difference between omnibox and location bar
is
> now a bit more than a "tweak"
> 
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox...

Sure. What about this should I tweak? Do you just mean the comment, or should
the code and/or icon be different?

[Evan Stade, 2016-05-04 21:38:51]

On 2016/05/04 21:33:01, palmer wrote:
> > is this still correct? [1]
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/toolbar...
> 
> Yes, we are still surfacing Dubious as Neutral. That is the new status quo.
> 
> > nit: can you update [2] since the difference between omnibox and location
bar
> is
> > now a bit more than a "tweak"
> > 
> > [2]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox...
> 
> Sure. What about this should I tweak? Do you just mean the comment, or should
> the code and/or icon be different?

I think the code is still correct, just the comment needs updating (it's a
totally different icon, not a tweaked one). Also, have you tested this in
incognito (with classic theme)?

[palmer, 2016-05-07 00:02:01]

> > Sure. What about this should I tweak? Do you just mean the comment, or
should
> > the code and/or icon be different?
> 
> I think the code is still correct, just the comment needs updating (it's a
> totally different icon, not a tweaked one).

Comment updated.

> Also, have you tested this in
> incognito (with classic theme)?

AFAICT, Incognito looks the same as non-Incognito, and hence looks like the
screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=604520#c16.
(That's with a ToT build, and after rm -rf ~/.config/chromium.) Should I be
seeing something different?

[palmer, 2016-05-07 00:02:29]

palmer@chromium.org changed reviewers:
+ pkasting@google.com

[palmer, 2016-05-07 00:02:30]

+pkasting generally, and for the omnibox component OWNERS.

[palmer, 2016-05-07 00:10:43]

Looks like I need to update the *invert*.icon files too, for incognito. Coming
up.

[Evan Stade, 2016-05-10 21:56:35]

https://codereview.chromium.org/1935663002/diff/80001/ui/gfx/vector_icons/loc...
File ui/gfx/vector_icons/location_bar_https_invalid.icon (right):

https://codereview.chromium.org/1935663002/diff/80001/ui/gfx/vector_icons/loc...
ui/gfx/vector_icons/location_bar_https_invalid.icon:6: PATH_COLOR_ARGB, 0xFF,
0xC5, 0x39, 0x29,
don't create a separate .icon file just for a different color. Pass the color at
runtime.

[palmer, 2016-05-10 22:22:09]

> ui/gfx/vector_icons/location_bar_https_invalid.icon:6: PATH_COLOR_ARGB, 0xFF,
> 0xC5, 0x39, 0x29,
> don't create a separate .icon file just for a different color. Pass the color
at
> runtime.

Given maxwalker's most recent email, saying that all the icons should be all
white in inverted mode, does that imply I need to refactor gfx::VectorIconId
OmniboxView::GetVectorIcon(bool invert) as well? It has:

 91   if (!IsEditingOrEmpty()) {
 92     gfx::VectorIconId id = controller_->GetToolbarModel()->GetVectorIcon(); 
                                                                          
 93     if (invert) {
 94       if (id == gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID)
 95         return gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID_INVERT;          
                                                                          
 96       if (id == gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID)
 97         return gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID_INVERT;        
                                                                          
 98     }
 99     return id;
100   } 

So we'd change it to not take an |invert| argument, and to not do those
replacements?

[Evan Stade, 2016-05-10 23:13:15]

On 2016/05/10 22:22:09, palmer wrote:
> > ui/gfx/vector_icons/location_bar_https_invalid.icon:6: PATH_COLOR_ARGB,
0xFF,
> > 0xC5, 0x39, 0x29,
> > don't create a separate .icon file just for a different color. Pass the
color
> at
> > runtime.
> 
> Given maxwalker's most recent email, saying that all the icons should be all
> white in inverted mode, does that imply I need to refactor gfx::VectorIconId
> OmniboxView::GetVectorIcon(bool invert) as well? It has:
> 
>  91   if (!IsEditingOrEmpty()) {
>  92     gfx::VectorIconId id =
controller_->GetToolbarModel()->GetVectorIcon(); 
>                                                                           
>  93     if (invert) {
>  94       if (id == gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID)
>  95         return gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID_INVERT;        
 
>                                                                           
>  96       if (id == gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID)
>  97         return gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID_INVERT;      
 
>                                                                           
>  98     }
>  99     return id;
> 100   } 
> 
> So we'd change it to not take an |invert| argument, and to not do those
> replacements?

Yes, and I think the color of the icon (set in location_bar_view.cc) can just be
gfx::DeriveDefaultIconColor(GetSecureTextColor(GetToolbarModel()->GetSecurityLevel(false)))

[palmer, 2016-05-11 00:22:32]

> > So we'd change it to not take an |invert| argument, and to not do those
> > replacements?
> 
> Yes, and I think the color of the icon (set in location_bar_view.cc) can just
be
>
gfx::DeriveDefaultIconColor(GetSecureTextColor(GetToolbarModel()->GetSecurityLevel(false)))

OK, I have done that. Let me know what you think.

One problem: the colors are a bit dim: dim green, dim red, gray instead of white
in Incognito w/Classic theme. But AFAICT I am returning the 'correct' colors. Do
I need to explicitly disable any alpha channel in the .icon files, or something
like that? Thanks for clues.

[Use pkasting(at)chromium.org, 2016-05-11 01:56:35]

Description was changed from

==========
New origin security indicator icons.

For the Omnibox.

BUG=604520
==========

to

==========
New origin security indicator icons.

For the Omnibox.

BUG=604520
==========

[Use pkasting(at)chromium.org, 2016-05-11 01:56:35]

pkasting@google.com changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[Peter Kasting, 2016-05-11 18:53:22]

On 2016/05/11 00:22:32, palmer wrote:
> One problem: the colors are a bit dim: dim green, dim red, gray instead of
white
> in Incognito w/Classic theme. But AFAICT I am returning the 'correct' colors.
Do
> I need to explicitly disable any alpha channel in the .icon files, or
something
> like that? Thanks for clues.

You'd have to debug and figure out the source of the dimness.  Perhaps the icons
are for some reason being drawn partly-transparent; perhaps somewhere the color
used to construct the icon is acquiring an alpha value; or perhaps the
GetReadableColor() call in computing the icon color is doing something you don't
expect/want.

https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:368: case
security_state::SecurityStateModel::NONE:
Nit: Combine with the SECURITY_WARNING case

https://codereview.chromium.org/1935663002/diff/100001/ui/gfx/vector_icons/lo...
File ui/gfx/vector_icons/location_bar_http.icon (right):

https://codereview.chromium.org/1935663002/diff/100001/ui/gfx/vector_icons/lo...
ui/gfx/vector_icons/location_bar_http.icon:11: LINE_TO, 16.5f, 3,
These icon files have a number of extraneous directives in them.

For example, in this file, I believe this line and the next one, line 18, lines
23 and 24, and lines 30 and 31 are all unnecessary.

Consider cleaning all these up.

[Evan Stade, 2016-05-11 19:30:10]

On 2016/05/11 18:53:22, Peter Kasting wrote:
> On 2016/05/11 00:22:32, palmer wrote:
> > One problem: the colors are a bit dim: dim green, dim red, gray instead of
> white
> > in Incognito w/Classic theme. But AFAICT I am returning the 'correct'
colors.
> Do
> > I need to explicitly disable any alpha channel in the .icon files, or
> something
> > like that? Thanks for clues.
> 
> You'd have to debug and figure out the source of the dimness.  Perhaps the
icons
> are for some reason being drawn partly-transparent; perhaps somewhere the
color
> used to construct the icon is acquiring an alpha value; or perhaps the
> GetReadableColor() call in computing the icon color is doing something you
don't
> expect/want.
> 
>
https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:368: case
> security_state::SecurityStateModel::NONE:
> Nit: Combine with the SECURITY_WARNING case
> 
>
https://codereview.chromium.org/1935663002/diff/100001/ui/gfx/vector_icons/lo...
> File ui/gfx/vector_icons/location_bar_http.icon (right):
> 
>
https://codereview.chromium.org/1935663002/diff/100001/ui/gfx/vector_icons/lo...
> ui/gfx/vector_icons/location_bar_http.icon:11: LINE_TO, 16.5f, 3,
> These icon files have a number of extraneous directives in them.
> 
> For example, in this file, I believe this line and the next one, line 18,
lines
> 23 and 24, and lines 30 and 31 are all unnecessary.
> 
> Consider cleaning all these up.

the dimness is coming from DeriveDefaultIconColor --- I forgot about that. You
probably only want to apply DeriveDefaultIconColor to the text color if it's
fully black/if the security state is none. Kind of an ugly exception but can't
think of a better way.

[Evan Stade, 2016-05-12 16:08:11]

https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:365: case
security_state::SecurityStateModel::NONE:
I'd rather you not add this, doesn't seem necessary or make much sense

https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:876: icon_color =
GetSecureTextColor(security_level);
icon_color = security_level == security_state::SecurityStateModel::NONE ? 
gfx::DeriveDefaultIconColor(GetColor(TEXT)) :
GetSecureTextColor(security_level);

https://codereview.chromium.org/1935663002/diff/120001/ui/gfx/vector_icons/lo...
File ui/gfx/vector_icons/location_bar_https_valid.icon (right):

https://codereview.chromium.org/1935663002/diff/120001/ui/gfx/vector_icons/lo...
ui/gfx/vector_icons/location_bar_https_valid.icon:26: LINE_TO, 13, 12.02f,
to tack onto what pkasting said about extraneous directives, it looks like you
didn't use svomg on these svgs, because this output is what i get if I skip that
step and it's not what I get if I include that step.

[palmer, 2016-05-12 21:23:15]

https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:368: case
security_state::SecurityStateModel::NONE:
On 2016/05/11 18:53:21, Peter Kasting wrote:
> Nit: Combine with the SECURITY_WARNING case

Done.

https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:365: case
security_state::SecurityStateModel::NONE:
On 2016/05/12 16:08:11, Evan Stade wrote:
> I'd rather you not add this, doesn't seem necessary or make much sense

Done.

https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:876: icon_color =
GetSecureTextColor(security_level);
On 2016/05/12 16:08:11, Evan Stade wrote:
> icon_color = security_level == security_state::SecurityStateModel::NONE ? 
> gfx::DeriveDefaultIconColor(GetColor(TEXT)) :
> GetSecureTextColor(security_level);

Done.

https://codereview.chromium.org/1935663002/diff/120001/ui/gfx/vector_icons/lo...
File ui/gfx/vector_icons/location_bar_https_valid.icon (right):

https://codereview.chromium.org/1935663002/diff/120001/ui/gfx/vector_icons/lo...
ui/gfx/vector_icons/location_bar_https_valid.icon:26: LINE_TO, 13, 12.02f,
> to tack onto what pkasting said about extraneous directives, it looks like you
> didn't use svomg on these svgs, because this output is what i get if I skip
that
> step and it's not what I get if I include that step.

True, I did not use SVGO, even though you strongly suggest it at
http://evanstade.github.io/skiafy/, because as a security nerd I didn't relish
the thought of installing Node and then an unknown number of modules and SVGO
plugins on my main workstation, all as root. :) It seemed easier to manually
clean these up than to take that risk (which seems to be a loophole in our
software install policy anyway? I don't even want to know). I am sorry to annoy
you. New patchset coming soon.

[Evan Stade, 2016-05-12 21:33:46]

On 2016/05/12 21:23:15, palmer wrote:
>
https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/1935663002/diff/100001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:368: case
> security_state::SecurityStateModel::NONE:
> On 2016/05/11 18:53:21, Peter Kasting wrote:
> > Nit: Combine with the SECURITY_WARNING case
> 
> Done.
> 
>
https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:365: case
> security_state::SecurityStateModel::NONE:
> On 2016/05/12 16:08:11, Evan Stade wrote:
> > I'd rather you not add this, doesn't seem necessary or make much sense
> 
> Done.
> 
>
https://codereview.chromium.org/1935663002/diff/120001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:876: icon_color =
> GetSecureTextColor(security_level);
> On 2016/05/12 16:08:11, Evan Stade wrote:
> > icon_color = security_level == security_state::SecurityStateModel::NONE ? 
> > gfx::DeriveDefaultIconColor(GetColor(TEXT)) :
> > GetSecureTextColor(security_level);
> 
> Done.
> 
>
https://codereview.chromium.org/1935663002/diff/120001/ui/gfx/vector_icons/lo...
> File ui/gfx/vector_icons/location_bar_https_valid.icon (right):
> 
>
https://codereview.chromium.org/1935663002/diff/120001/ui/gfx/vector_icons/lo...
> ui/gfx/vector_icons/location_bar_https_valid.icon:26: LINE_TO, 13, 12.02f,
> > to tack onto what pkasting said about extraneous directives, it looks like
you
> > didn't use svomg on these svgs, because this output is what i get if I skip
> that
> > step and it's not what I get if I include that step.
> 
> True, I did not use SVGO, even though you strongly suggest it at
> http://evanstade.github.io/skiafy/, because as a security nerd I didn't relish
> the thought of installing Node and then an unknown number of modules and SVGO
> plugins on my main workstation, all as root. :) It seemed easier to manually
> clean these up than to take that risk (which seems to be a loophole in our
> software install policy anyway? I don't even want to know). I am sorry to
annoy
> you. New patchset coming soon.

if you use svgomg you don't have to install anything

[palmer, 2016-05-12 21:40:03]

> if you use svgomg you don't have to install anything

I updated the documentation at
https://sites.google.com/a/chromium.org/dev/developers/how-tos/vectorized-ico...
to mention SVGOMG. (I didn't realize you were talking about a different thing
than SVGO.) You might also want to point to it on your page at
http://evanstade.github.io/skiafy/.

[Evan Stade, 2016-05-12 21:42:43]

On 2016/05/12 21:40:03, palmer wrote:
> > if you use svgomg you don't have to install anything
> 
> I updated the documentation at
>
https://sites.google.com/a/chromium.org/dev/developers/how-tos/vectorized-ico...
> to mention SVGOMG. (I didn't realize you were talking about a different thing
> than SVGO.) You might also want to point to it on your page at
> http://evanstade.github.io/skiafy/.

already done :)

[palmer, 2016-05-12 22:14:51]

Patchset 10 is the result of SVGOMG -> Skiafier for:

ui/gfx/vector_icons/location_bar_https_invalid.1x.icon
ui/gfx/vector_icons/location_bar_https_invalid.icon
ui/gfx/vector_icons/location_bar_https_valid.1x.icon
ui/gfx/vector_icons/location_bar_https_valid.icon
ui/gfx/vector_icons/location_bar_https_valid_in_chip.icon

For the others, that chain resulted in invalid Skia output ("???" for some
coordinates, which the compiler rejects). You can see that the files in Patchset
9 are about the same size or slightly smaller, so overall we have
fairly-well-minimized and visually correct icons now.

[Peter Kasting, 2016-05-17 03:12:34]

LGTM

On 2016/05/12 22:14:51, palmer wrote:
> Patchset 10 is the result of SVGOMG -> Skiafier for:
> 
> ui/gfx/vector_icons/location_bar_https_invalid.1x.icon
> ui/gfx/vector_icons/location_bar_https_invalid.icon
> ui/gfx/vector_icons/location_bar_https_valid.1x.icon
> ui/gfx/vector_icons/location_bar_https_valid.icon
> ui/gfx/vector_icons/location_bar_https_valid_in_chip.icon
> 
> For the others, that chain resulted in invalid Skia output ("???" for some
> coordinates, which the compiler rejects).

Was it clear which coordinates got goofed up and why?  It would be nice to (a)
work around this manually and (b) fix Skiafier if the bug is in that.

https://codereview.chromium.org/1935663002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:875: icon_color =
security_level == security_state::SecurityStateModel::NONE
Nit: I would put the comparison in parens (reads more clearly to me and the
precedence of ?: is always a bit weird)

[Evan Stade, 2016-05-17 18:15:02]

On 2016/05/17 03:12:34, Peter Kasting wrote:
> LGTM
> 
> On 2016/05/12 22:14:51, palmer wrote:
> > Patchset 10 is the result of SVGOMG -> Skiafier for:
> > 
> > ui/gfx/vector_icons/location_bar_https_invalid.1x.icon
> > ui/gfx/vector_icons/location_bar_https_invalid.icon
> > ui/gfx/vector_icons/location_bar_https_valid.1x.icon
> > ui/gfx/vector_icons/location_bar_https_valid.icon
> > ui/gfx/vector_icons/location_bar_https_valid_in_chip.icon
> > 
> > For the others, that chain resulted in invalid Skia output ("???" for some
> > coordinates, which the compiler rejects).
> 
> Was it clear which coordinates got goofed up and why?  It would be nice to (a)
> work around this manually and (b) fix Skiafier if the bug is in that.

I talked to palmer about this last week over chat, and I believe he's working on
one or both of these.

[palmer, 2016-05-18 20:34:47]

> > > For the others, that chain resulted in invalid Skia output ("???" for some
> > > coordinates, which the compiler rejects).
> > 
> > Was it clear which coordinates got goofed up and why?  It would be nice to
(a)
> > work around this manually and (b) fix Skiafier if the bug is in that.

Yeah, SVGO(MG) changed how the drawing happens, including using the SVG S
element, which estade's Skiafier can't handle yet.

> I talked to palmer about this last week over chat, and I believe he's working
on
> one or both of these.

Right.

I want to make the 52 cutoff Thursday night, but I am not sure if I can/pretty
sure I can't if we gate on my fixing Skiafier. estade, do you object to landing
the non-minimal .icon files (in this or in a previous patchset), and fizing
Skiafier and landed fully-optimized .icons immediately afterward (i.e. next
week)?

[Evan Stade, 2016-05-18 21:40:43]

I think mac needs updating

https://codereview.chromium.org/1935663002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/omnibox_view.cc (right):

https://codereview.chromium.org/1935663002/diff/180001/components/omnibox/bro...
components/omnibox/browser/omnibox_view.cc:92: gfx::VectorIconId id =
controller_->GetToolbarModel()->GetVectorIcon();
nit: don't need this local var any more

[groby-ooo-7-16, 2016-05-18 22:39:31]

On 2016/05/18 21:40:43, Evan Stade wrote:
> I think mac needs updating

Presumably yes. LocationBarViewMac::UpdateLocationIcon()

[palmer, 2016-05-18 22:54:59]

> > I think mac needs updating
> 
> Presumably yes. LocationBarViewMac::UpdateLocationIcon()

Yes. I have a patchset cooking that handles this and responds to the comments
above. Coming up...

[palmer, 2016-05-19 17:49:23]

https://codereview.chromium.org/1935663002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:875: icon_color =
security_level == security_state::SecurityStateModel::NONE
On 2016/05/17 03:12:34, Peter Kasting wrote:
> Nit: I would put the comparison in parens (reads more clearly to me and the
> precedence of ?: is always a bit weird)

Done.

https://codereview.chromium.org/1935663002/diff/180001/components/omnibox/bro...
File components/omnibox/browser/omnibox_view.cc (right):

https://codereview.chromium.org/1935663002/diff/180001/components/omnibox/bro...
components/omnibox/browser/omnibox_view.cc:92: gfx::VectorIconId id =
controller_->GetToolbarModel()->GetVectorIcon();
On 2016/05/18 21:40:42, Evan Stade wrote:
> nit: don't need this local var any more

Done.

[palmer, 2016-05-19 17:51:05]

palmer@chromium.org changed reviewers:
+ groby@chromium.org

[palmer, 2016-05-19 17:51:06]

+groby for chrome/browser/ui/cocoa/. Thanks!

[Evan Stade, 2016-05-19 17:58:49]

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:37: static SkColor
GetSecureTextColorSkia(
you only call this in one place, it doesn't seem like it's worth it to make this
instead of just inlining the conversion and using GetSecureTextCOlor

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:202: color =
BaseTextColor(in_dark_mode);
I'm often told not to handle NOTREACHED

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:630: NSColor* color =
GetSecureTextColor(security_level, in_dark_mode);
no need for this local var

[palmer, 2016-05-19 18:34:36]

The CQ bit was checked by palmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 18:35:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1935663002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1935663002/200001

[Peter Kasting, 2016-05-19 18:36:19]

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:202: color =
BaseTextColor(in_dark_mode);
On 2016/05/19 17:58:49, Evan Stade wrote:
> I'm often told not to handle NOTREACHED

Yeah.  And in general looking at this makes it clear that we could just
early-return to do this all more simply:

{
  if (security_level == security_state::SecurityStateModel::EV_SECURE ||
      security_level == security_state::SecurityStateModel::SECURE)
    return SecureSchemeColor(in_dark_mode);

  if (security_level == security_state::SecurityStateModel::SECURITY_ERROR)
    return SecurityErrorSchemeColor(in_dark_mode);

  DCHECK_EQ(security_state::SecurityStateModel::SECURITY_WARNING,
            security_level);
  return SecurityWarningSchemeColor(in_dark_mode);
}

[palmer, 2016-05-19 18:40:41]

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:37: static SkColor
GetSecureTextColorSkia(
> you only call this in one place, it doesn't seem like it's worth it to make
this
> instead of just inlining the conversion and using GetSecureTextCOlor

I had it that way at first, but groby suggested in email that I mimic
BaseTextColor*. I'm fine with doing it either way.

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:202: color =
BaseTextColor(in_dark_mode);
> I'm often told not to handle NOTREACHED

This is borrowed from the pre-existing code (compare with the deleted lines in
this file); and mirrors the Views code. Again, I could go either way, but I'd
prefer not to mess with pre-existing code unless there is a compelling reason.
I've never been advised not to handle NOTREACHED.

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:630: NSColor* color =
GetSecureTextColor(security_level, in_dark_mode);
On 2016/05/19 17:58:49, Evan Stade wrote:
> no need for this local var

Done.

[Peter Kasting, 2016-05-19 18:43:22]

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:37: static SkColor
GetSecureTextColorSkia(
On 2016/05/19 18:40:40, palmer wrote:
> > you only call this in one place, it doesn't seem like it's worth it to make
> this
> > instead of just inlining the conversion and using GetSecureTextCOlor
> 
> I had it that way at first, but groby suggested in email that I mimic
> BaseTextColor*. I'm fine with doing it either way.

Simpler is good.

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:202: color =
BaseTextColor(in_dark_mode);
On 2016/05/19 18:40:40, palmer wrote:
> > I'm often told not to handle NOTREACHED
> 
> This is borrowed from the pre-existing code (compare with the deleted lines in
> this file); and mirrors the Views code.

Please do fix, here and/or there, as possible.

> Again, I could go either way, but I'd
> prefer not to mess with pre-existing code unless there is a compelling reason.
> I've never been advised not to handle NOTREACHED.

Not handling NOTREACHED has been part of Chrome style for as long as I can
remember.  See
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
for more.

The existing code was wrong.

[palmer, 2016-05-19 19:55:44]

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:37: static SkColor
GetSecureTextColorSkia(
On 2016/05/19 18:43:21, Peter Kasting wrote:
> On 2016/05/19 18:40:40, palmer wrote:
> > > you only call this in one place, it doesn't seem like it's worth it to
make
> > this
> > > instead of just inlining the conversion and using GetSecureTextCOlor
> > 
> > I had it that way at first, but groby suggested in email that I mimic
> > BaseTextColor*. I'm fine with doing it either way.
> 
> Simpler is good.

Done.

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:202: color =
BaseTextColor(in_dark_mode);
> > This is borrowed from the pre-existing code (compare with the deleted lines
in
> > this file); and mirrors the Views code.
> 
> Please do fix, here and/or there, as possible.

Fixed here in Cocoa.

The Views code by estade/estark/jcivelli (GetSecureTextColor in
chrome/browser/ui/views/location_bar/location_bar_view.cc) was structured
differently(switch/case, with return at the bottom, sometimes) than the
equivalent Cocoa function; I rewrote it to follow your early-return + DCHECK_EQ
style. See if you like what I did there in the latest patchset.

[palmer, 2016-05-19 19:56:55]

> The Views code by estade/estark/jcivelli (GetSecureTextColor in
> chrome/browser/ui/views/location_bar/location_bar_view.cc) was structured
> differently(switch/case, with return at the bottom, sometimes) than the
> equivalent Cocoa function; I rewrote it to follow your early-return +
DCHECK_EQ
> style. See if you like what I did there in the latest patchset.

I should add that I did visually check (Classic theme, MD + non-MD, Normal +
Incognito) that everything still looks right after that rewrite.

[Peter Kasting, 2016-05-19 20:36:25]

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:369: }
Here's an even shorter way to write this that eliminates the repeated
GetReadableColor() calls that moving away from using a switch introduced:

  if (security_level ==
      security_state::SecurityStateModel::SECURITY_POLICY_WARNING)
    return GetColor(DEEMPHASIZED_TEXT);

  const bool md = ui::MaterialDesignController::IsModeMaterial();
  SkColor text_color = GetColor(TEXT);
  if (md && color_utils::IsDark(GetColor(BACKGROUND)) &&
      ((security_level == security_state::SecurityStateModel::EV_SECURE) ||
       (security_level == security_state::SecurityStateModel::SECURE) ||
       (security_level == security_state::SecurityStateModel::SECURITY_ERROR)))
    return text_color;

  if ((security_level == security_state::SecurityStateModel::EV_SECURE) ||
      (security_level == security_state::SecurityStateModel::SECURE))
    text_color = GetColor(EV_BUBBLE_TEXT_AND_BORDER);
  else if (security_level == security_state::SecurityStateModel::SECURITY_ERROR)
    text_color = md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0);

  return color_utils::GetReadableColor(text_color, GetColor(BACKGROUND));

[Peter Kasting, 2016-05-19 20:42:53]

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:369: }
Even shorter:

  if (security_level ==
      security_state::SecurityStateModel::SECURITY_POLICY_WARNING)
    return GetColor(DEEMPHASIZED_TEXT);

  SkColor text_color = GetColor(TEXT);
  if ((security_level == security_state::SecurityStateModel::EV_SECURE) ||
      (security_level == security_state::SecurityStateModel::SECURE) ||
      (security_level == security_state::SecurityStateModel::SECURITY_ERROR)) {
    const bool md = ui::MaterialDesignController::IsModeMaterial();
    if (md && color_utils::IsDark(GetColor(BACKGROUND)))
      return text_color;
    if (security_level == security_state::SecurityStateModel::SECURITY_ERROR)
      text_color = md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0);
    else
      text_color = GetColor(EV_BUBBLE_TEXT_AND_BORDER);
  }
  return color_utils::GetReadableColor(text_color, GetColor(BACKGROUND));

[palmer, 2016-05-19 20:59:07]

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:369: }
On 2016/05/19 20:42:53, Peter Kasting wrote:
> Even shorter:
> 
>   if (security_level ==
>       security_state::SecurityStateModel::SECURITY_POLICY_WARNING)
>     return GetColor(DEEMPHASIZED_TEXT);
> 
>   SkColor text_color = GetColor(TEXT);
>   if ((security_level == security_state::SecurityStateModel::EV_SECURE) ||
>       (security_level == security_state::SecurityStateModel::SECURE) ||
>       (security_level == security_state::SecurityStateModel::SECURITY_ERROR))
{
>     const bool md = ui::MaterialDesignController::IsModeMaterial();
>     if (md && color_utils::IsDark(GetColor(BACKGROUND)))
>       return text_color;
>     if (security_level == security_state::SecurityStateModel::SECURITY_ERROR)
>       text_color = md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0);
>     else
>       text_color = GetColor(EV_BUBBLE_TEXT_AND_BORDER);
>   }
>   return color_utils::GetReadableColor(text_color, GetColor(BACKGROUND));

That is good; but: it no longer has the DCHECK_EQ, if that matters. It's also
less clearly-parallel to the Cocoa version, if that matters. You all are the
OWNERS of this code, so I'm happy to go with whatever your preference is. Just
thought I should raise those points first.

I do like short code...

[Peter Kasting, 2016-05-19 21:02:28]

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:369: }
On 2016/05/19 20:59:06, palmer wrote:
> On 2016/05/19 20:42:53, Peter Kasting wrote:
> > Even shorter:
> > 
> >   if (security_level ==
> >       security_state::SecurityStateModel::SECURITY_POLICY_WARNING)
> >     return GetColor(DEEMPHASIZED_TEXT);
> > 
> >   SkColor text_color = GetColor(TEXT);
> >   if ((security_level == security_state::SecurityStateModel::EV_SECURE) ||
> >       (security_level == security_state::SecurityStateModel::SECURE) ||
> >       (security_level ==
security_state::SecurityStateModel::SECURITY_ERROR))
> {
> >     const bool md = ui::MaterialDesignController::IsModeMaterial();
> >     if (md && color_utils::IsDark(GetColor(BACKGROUND)))
> >       return text_color;
> >     if (security_level ==
security_state::SecurityStateModel::SECURITY_ERROR)
> >       text_color = md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0);
> >     else
> >       text_color = GetColor(EV_BUBBLE_TEXT_AND_BORDER);
> >   }
> >   return color_utils::GetReadableColor(text_color, GetColor(BACKGROUND));
> 
> That is good; but: it no longer has the DCHECK_EQ, if that matters.

I considered that when I wrote it and decided that it actually doesn't need the
DCHECK, because even if a caller were to pass NONE, it does something very
reasonable as written.

> It's also
> less clearly-parallel to the Cocoa version, if that matters.

I think that's in large part because the two have noticeable functional
differences.  I don't know if that's correct or not, but given that they don't
return the same values, it seems reasonable that they're not structured the same
either.

[palmer, 2016-05-19 21:52:01]

> I think that's in large part because the two have noticeable functional
> differences.  I don't know if that's correct or not, but given that they don't
> return the same values, it seems reasonable that they're not structured the
same
> either.

SGTM. Shortened, and tested, in the latest patchset.

[Evan Stade, 2016-05-19 22:00:45]

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:183: security_level ==
security_state::SecurityStateModel::SECURE) {
wait, I lost the part where it was explained why we moved away from a switch.
switch has the nice property of complaining when you add a new enum value that
isn't handled (provided you avoid default)

[Peter Kasting, 2016-05-19 22:02:59]

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:183: security_level ==
security_state::SecurityStateModel::SECURE) {
On 2016/05/19 22:00:45, Evan Stade wrote:
> wait, I lost the part where it was explained why we moved away from a switch.
> switch has the nice property of complaining when you add a new enum value that
> isn't handled (provided you avoid default)

We don't want to handle every value in the enum in this function, so we'd need a
default case, which eliminates that benefit.

[Evan Stade, 2016-05-19 22:09:07]

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:183: security_level ==
security_state::SecurityStateModel::SECURE) {
On 2016/05/19 22:02:59, Peter Kasting wrote:
> On 2016/05/19 22:00:45, Evan Stade wrote:
> > wait, I lost the part where it was explained why we moved away from a
switch.
> > switch has the nice property of complaining when you add a new enum value
that
> > isn't handled (provided you avoid default)
> 
> We don't want to handle every value in the enum in this function, so we'd need
a
> default case, which eliminates that benefit.

well, I'd handle that with

[...]
  case NONE:
  case SECURITY_POLICY_WARNING:
    NOTREACHED();
}

return NsDummyColor;

[Peter Kasting, 2016-05-19 22:50:11]

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:183: security_level ==
security_state::SecurityStateModel::SECURE) {
On 2016/05/19 22:09:07, Evan Stade wrote:
> On 2016/05/19 22:02:59, Peter Kasting wrote:
> > On 2016/05/19 22:00:45, Evan Stade wrote:
> > > wait, I lost the part where it was explained why we moved away from a
> switch.
> > > switch has the nice property of complaining when you add a new enum value
> that
> > > isn't handled (provided you avoid default)
> > 
> > We don't want to handle every value in the enum in this function, so we'd
need
> a
> > default case, which eliminates that benefit.
> 
> well, I'd handle that with
> 
> [...]
>   case NONE:
>   case SECURITY_POLICY_WARNING:
>     NOTREACHED();
> }
> 
> return NsDummyColor;

We could.  It's not a huge deal.  But to me the potential win of being warned on
an added case in the future that we don't add here is less than the loss from
having to explicitly write out each case we don't think will get here anyway,
especially when this is technically more in keeping with the "don't handle
DCHECK failure" policy (arguably).

I say this partly because we're in the midst of a project to try and actively
_reduce_ the number of security states, so it seems especially unlikely we'd add
more.

[Peter Kasting, 2016-05-20 00:14:12]

Still LGTM

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:343:
security_state::SecurityStateModel::SECURITY_POLICY_WARNING) {
Nit: No {} (use it when the body is multiple lines, not the condition; see e.g.
line 704)

[Evan Stade, 2016-05-20 00:17:32]

lgtm

[groby-ooo-7-16, 2016-05-23 18:54:46]

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:583: :
skia::NSDeviceColorToSkColor(OmniboxViewMac::GetSecureTextColor(
Question: If we pass in the security level anyways, why not just move the entire
logic into GetSecureTextColor?

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:37: static NSColor*
GetSecureTextColor(
No Skia equivalent?

[palmer, 2016-05-23 21:11:48]

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:583: :
skia::NSDeviceColorToSkColor(OmniboxViewMac::GetSecureTextColor(
> Question: If we pass in the security level anyways, why not just move the
entire
> logic into GetSecureTextColor?

You mean have GetSecureTextColor call GetToolbarModel()->GetSecurityLevel(false)
? Sure, I suppose we could, although it makes GetSecureTextColor assume the
false argument to GetToolbarModel()->GetSecurityLevel(false), which makes it
potentially less useful for some other callers (if any).

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:37: static NSColor*
GetSecureTextColor(
> No Skia equivalent?

An earlier patchset included it, as you advised, but pkasting and estade didn't
like it, so I took it out again. *shrug*

[groby-ooo-7-16, 2016-05-23 23:34:01]

lgtm

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:583: :
skia::NSDeviceColorToSkColor(OmniboxViewMac::GetSecureTextColor(
On 2016/05/23 21:11:48, palmer wrote:
> > Question: If we pass in the security level anyways, why not just move the
> entire
> > logic into GetSecureTextColor?
> 
> You mean have GetSecureTextColor call
GetToolbarModel()->GetSecurityLevel(false)
> ? Sure, I suppose we could, although it makes GetSecureTextColor assume the
> false argument to GetToolbarModel()->GetSecurityLevel(false), which makes it
> potentially less useful for some other callers (if any).

No, I meant handling the ::NONE case in GetSecureTextColor, too. But happy to
punt that to a different CL/debate, since Views doesn't do that either.

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right):

https://codereview.chromium.org/1935663002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:37: static NSColor*
GetSecureTextColor(
On 2016/05/23 21:11:48, palmer wrote:
> > No Skia equivalent?
> 
> An earlier patchset included it, as you advised, but pkasting and estade
didn't
> like it, so I took it out again. *shrug*

Acknowledged.

[palmer, 2016-05-23 23:39:27]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2016-05-23 23:40:12]

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

[commit-bot: I haz the power, 2016-05-24 00:56:07]

Description was changed from

==========
New origin security indicator icons.

For the Omnibox.

BUG=604520
==========

to

==========
New origin security indicator icons.

For the Omnibox.

BUG=604520
==========

[commit-bot: I haz the power, 2016-05-24 00:56:08]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-05-24 00:58:51]

Description was changed from

==========
New origin security indicator icons.

For the Omnibox.

BUG=604520
==========

to

==========
New origin security indicator icons.

For the Omnibox.

BUG=604520

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

[commit-bot: I haz the power, 2016-05-24 00:58:53]

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