[Material][Mac] Update Omnibox bubble decorations

Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs

BUG=611113
Committed: https://crrev.com/a76a3173e38bb3fab808ce5cdbf651d169f9194f
Cr-Commit-Position: refs/heads/master@{#395108}

[spqchan, 2016-05-18 17:07:14]

Description was changed from

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

to

==========
Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs


BUG=611113
==========

[spqchan, 2016-05-18 17:07:14]

spqchan@chromium.org changed reviewers:
+ shrike@chromium.org

[spqchan, 2016-05-18 17:13:31]

PTAL

[spqchan, 2016-05-18 17:13:41]

Description was changed from

==========
Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs


BUG=611113
==========

to

==========
Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs

BUG=611113
==========

[shrike, 2016-05-18 21:24:08]

Hi spqchan@,

This is looking good!

The lock icon is too small - I'm not sure what size you are passing into the
CreateVectorIcon() function but in the end the lock icon needs to be 8pt wide.
It also needs to sit a bit lower, so that its lower edge aligns with the text
baseline.

I realize now that there's no Incognito spec, but based on other specs please
make the lock 80% opaque white (so #FFFFFF, 0xCC alpha) and the divider 15%
opaque white( #FFFFFF, 0x26 alpha).

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:257: // the
text color to #5A5A5A.
#5A5A5A is kChromeIconGrey(sp) from ui/gfx/color_palette.h. Can you replace this
with something like

          owner_->IsLocationBarDark() ? SK_ColorWHITE : gfx::kChromeIconGrey;

and then convert the Skia color to an NSColor using
skia::SkColorToCalibratedNSColor()?

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm:17: const
CGFloat kMaterialDividerAlpha = 15.0;
This should actually be 38 (15% of 255).

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm:18: const
CGFloat kMaterialDividerGreyScale = 0.0;
Grey should be Gray

[spqchan, 2016-05-18 23:58:54]

PTAL

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:257: // the
text color to #5A5A5A.
On 2016/05/18 21:24:08, shrike wrote:
> #5A5A5A is kChromeIconGrey(sp) from ui/gfx/color_palette.h. Can you replace
this
> with something like
> 
>           owner_->IsLocationBarDark() ? SK_ColorWHITE : gfx::kChromeIconGrey;
> 
> and then convert the Skia color to an NSColor using
> skia::SkColorToCalibratedNSColor()?

Done.

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm:17: const
CGFloat kMaterialDividerAlpha = 15.0;
On 2016/05/18 21:24:08, shrike wrote:
> This should actually be 38 (15% of 255).

Done.

https://codereview.chromium.org/1990703002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.mm:18: const
CGFloat kMaterialDividerGreyScale = 0.0;
On 2016/05/18 21:24:08, shrike wrote:
> Grey should be Gray

Done.

[spqchan, 2016-05-19 00:02:21]

Hold the CL, just realized I missed the dark mode for search

[spqchan, 2016-05-19 00:32:26]

On 2016/05/19 00:02:21, spqchan wrote:
> Hold the CL, just realized I missed the dark mode for search

Okay done, just fixed the colors for keyword in incognito!

[shrike, 2016-05-19 17:57:12]

Hi spqchan,

Thank you for making the changes.

Re: the lock icon, it's needs to be a little bit larger (on my Retina it's
coming in at 7pt wide - it needs to be 8). It's also not sitting on the text
baseline (off by 1px). I thinking that bumping the size a bit more will fix the
baseline problem.

This icon is not the right icon for the new EV Cert. The new icon is a lock
without a keyhole while this icon has a keyhole. I've asked on one of the
omnibox security tracking bugs if the vector art is available (there's not much
we can do about it until we have the art).

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:113: [line
setLineWidth:0];
Change the line width to 1 - we actually want a 1pt line (not a 1px line). That
will fix the problem of the line being drawn to lightly on Retina.

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:396: [line
setLineWidth:0];
Change the line width to 1 - we actually want a 1pt line (not a 1px line). That
will fix the problem of the line being drawn to lightly on Retina.

[spqchan, 2016-05-19 18:18:39]

On 2016/05/19 17:57:12, shrike wrote:
> Hi spqchan,
> 
> Thank you for making the changes.
> 
> Re: the lock icon, it's needs to be a little bit larger (on my Retina it's
> coming in at 7pt wide - it needs to be 8). It's also not sitting on the text
> baseline (off by 1px). I thinking that bumping the size a bit more will fix
the
> baseline problem.
> 
> This icon is not the right icon for the new EV Cert. The new icon is a lock
> without a keyhole while this icon has a keyhole. I've asked on one of the
> omnibox security tracking bugs if the vector art is available (there's not
much
> we can do about it until we have the art).
> 
>
https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right):
> 
>
https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:113: [line
> setLineWidth:0];
> Change the line width to 1 - we actually want a 1pt line (not a 1px line).
That
> will fix the problem of the line being drawn to lightly on Retina.
> 
>
https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm
(right):
> 
>
https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:396: [line
> setLineWidth:0];
> Change the line width to 1 - we actually want a 1pt line (not a 1px line).
That
> will fix the problem of the line being drawn to lightly on Retina.

Sure thing, I'll make the changes to get the size right. I noticed that the icon
is incorrect too, but from my understanding the new icon is in
https://codereview.chromium.org/1935663002/ and once it's commited, the changes
will let us display the new icon. Is that correct?

[shrike, 2016-05-19 18:28:12]

On 2016/05/19 18:18:39, spqchan wrote:
> Sure thing, I'll make the changes to get the size right. I noticed that the
icon
> is incorrect too, but from my understanding the new icon is in
> https://codereview.chromium.org/1935663002/ and once it's commited, the
changes
> will let us display the new icon. Is that correct?

Thank you - I think you're right.

Just FYI (in case you are not aware), today is branch point. I'm trying to get
as many remaining changes in before 5pm today but whatever does not make it will
get cherry-picked back.

[spqchan, 2016-05-19 23:05:35]

PTAL

I made the fixes and slightly increased the size of the icon.
I'm not sure if I need to push it slightly up to match the baseline, I can't
really tell.

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:113: [line
setLineWidth:0];
On 2016/05/19 17:57:11, shrike wrote:
> Change the line width to 1 - we actually want a 1pt line (not a 1px line).
That
> will fix the problem of the line being drawn to lightly on Retina.

Done.

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

https://codereview.chromium.org/1990703002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:396: [line
setLineWidth:0];
On 2016/05/19 17:57:11, shrike wrote:
> Change the line width to 1 - we actually want a 1pt line (not a 1px line).
That
> will fix the problem of the line being drawn to lightly on Retina.

Done.

[shrike, 2016-05-19 23:58:16]

On 2016/05/19 23:05:35, spqchan wrote:
> PTAL
> 
> I made the fixes and slightly increased the size of the icon.
> I'm not sure if I need to push it slightly up to match the baseline, I can't
> really tell.

Looks great!

The only thing that needs to change is the lock icon color in Incognito. It's a
bit darker than the EV Cert text that follows it. I'm guessing it's set to be
80% opaque white - it probably should be the color of the EV Cert text.

[spqchan, 2016-05-20 01:08:03]

PTAL

[spqchan, 2016-05-20 01:08:05]

PTAL

[shrike, 2016-05-20 16:31:08]

lgtm

[spqchan, 2016-05-20 16:57:42]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2016-05-20 16:57:42]

+rsesek for OWNERS

[Robert Sesek, 2016-05-20 17:06:43]

lgtm

[spqchan, 2016-05-20 17:08:36]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-05-20 17:08:57]

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

[commit-bot: I haz the power, 2016-05-20 17:48:02]

Description was changed from

==========
Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs

BUG=611113
==========

to

==========
Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs

BUG=611113
==========

[commit-bot: I haz the power, 2016-05-20 17:48:03]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-20 17:49:10]

Description was changed from

==========
Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs

BUG=611113
==========

to

==========
Updated the EV cert update, Keyword search, and Popup blocking
decorations with the new specs

BUG=611113

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

[commit-bot: I haz the power, 2016-05-20 17:49:11]

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