[android] New origin security indicator icons.

[android] New origin security indicator icons.

For the Omnibox.

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

[palmer, 2016-06-14 01:03:56]

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

[commit-bot: I haz the power, 2016-06-14 01:04:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063823005/1

[commit-bot: I haz the power, 2016-06-14 01:07:02]

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

[commit-bot: I haz the power, 2016-06-14 01:07:03]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[palmer, 2016-06-14 01:09:13]

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

[commit-bot: I haz the power, 2016-06-14 01:09:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063823005/20001

[commit-bot: I haz the power, 2016-06-14 01:46:20]

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

[commit-bot: I haz the power, 2016-06-14 01:46:21]

Dry run: This issue passed the CQ dry run.

[palmer, 2016-06-14 01:48:15]

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

[palmer, 2016-06-14 01:48:16]

tedchoc: I think you are an the OWNER for all these files.
felt: FYI.

[Ted C, 2016-06-14 17:10:24]

On 2016/06/14 01:48:16, palmer wrote:
> tedchoc: I think you are an the OWNER for all these files.
> felt: FYI.

The larger assets look less than ideal (i.e. pretty terrible)
in terms of jagged edges and sharpness.  Maybe the tool makes
them look worse, but it would be good to have some screenshots
to confirm/deny this.

Also, this is a change in behavior where we previously showed
no asset for http content on phones.  I don't see that discussed
in either the design doc or the mocks, but given that this has
been proposed in the past and shot down it would be good to know
why we have changed.

This spec doesn't seem to account for theme colors either.  If you
look at:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
we used to serve out different assets depending on theme color and
that doesn't seem to be the case anymore.

What happens if the site has a green theme color?  You won't be able
to see the lock.

Or what about colors that are hard to contrast with green (like red/green
for color blindness).

For broken https, we disable the theme color so that is less of an issue.

I'm also surprised we are using the same shape for lock icons, which
isn't exactly accessibility friendly.

Then there is the CustomTabToolbar that also has their own logic for
showing lock icons.  We should pull out some more shared logic from
LocationBarLayout to share with that.  But even if we show an icon for
http in regular chrome, should we be doing that in CCT?  That is another
open UI question.

CCT security icon logic:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

I think there a few open UI questions I raised above that I'd like to
get answers for before we land something like this.

[Ted C, 2016-06-14 18:24:34]

https://codereview.chromium.org/2063823005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
(right):

https://codereview.chromium.org/2063823005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1202:
return R.drawable.omnibox_https_valid_in_chip;
how does https_valid_in_chip differ from omnibox_https_valid?  The assets look
pretty much the same for me.

Also, I realized in this new spec that we never use the same shape to mean the
same thing, so my comment on accessibility doesn't seem valid.

There needs to be some work on the theme stuff, but one thing i'll mark off as
an invalid assumption on my part.

[palmer, 2016-06-14 18:32:01]

> how does https_valid_in_chip differ from omnibox_https_valid?  The assets look
> pretty much the same for me.

I just realized that we don't need it for mobile; no EV on mobile. On Desktop,
where there is EV, it matters (the image is slightly smaller, to fit within the
EV chip).

> Also, I realized in this new spec that we never use the same shape to mean the
> same thing, so my comment on accessibility doesn't seem valid.
> 
> There needs to be some work on the theme stuff, but one thing i'll mark off as
> an invalid assumption on my part.

Right, having different icon shapes is a key a11y goal for the new design.

Also yeah I'll fix the crunchy images at large sizes. Got a few other fires
burning today, but I'll post a new patchset and address as many of your other
concerns as possible ASAP.

Thanks for the review!

[palmer, 2016-06-14 20:54:00]

palmer@chromium.org changed reviewers:
+ ainslie@chromium.org, maxwalker@chromium.org

[palmer, 2016-06-14 20:54:01]

+ainslie, maxwalker for design thoughts in response to tedchoc's questions.

[palmer, 2016-06-14 20:55:34]

> The larger assets look less than ideal (i.e. pretty terrible)
> in terms of jagged edges and sharpness.  Maybe the tool makes
> them look worse, but it would be good to have some screenshots
> to confirm/deny this.

Hoping the new patchset fixes this; I re-generated the PNGs from the original
SVG scaled to 96x96, instead of from 32x32 as before (accidentally).

> Also, this is a change in behavior where we previously showed
> no asset for http content on phones.  I don't see that discussed
> in either the design doc or the mocks, but given that this has
> been proposed in the past and shot down it would be good to know
> why we have changed.

Changed back to returning 0 for that case in the new patchset.

> This spec doesn't seem to account for theme colors either.  If you
> look at:
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> we used to serve out different assets depending on theme color and
> that doesn't seem to be the case anymore.

Good point. It might be that we have to tint them differently based on the
current theme color. That should be relatively doable. Does anyone happen to
know of good examples for sites with theme colors, or should I make one?

> Or what about colors that are hard to contrast with green (like red/green
> for color blindness).

Any recommendations, design peeps?

> Then there is the CustomTabToolbar that also has their own logic for
> showing lock icons.  We should pull out some more shared logic from
> LocationBarLayout to share with that.  But even if we show an icon for
> http in regular chrome, should we be doing that in CCT?  That is another
> open UI question.

I was going to leave CCT for a subsequent CL. I like small CLs, where possible.
Do you think we should handle CCT in this one, though?

[felt, 2016-06-14 22:05:30]

On 2016/06/14 20:55:34, palmer wrote:
> > The larger assets look less than ideal (i.e. pretty terrible)
> > in terms of jagged edges and sharpness.  Maybe the tool makes
> > them look worse, but it would be good to have some screenshots
> > to confirm/deny this.
> 
> Hoping the new patchset fixes this; I re-generated the PNGs from the original
> SVG scaled to 96x96, instead of from 32x32 as before (accidentally).
> 
> > Also, this is a change in behavior where we previously showed
> > no asset for http content on phones.  I don't see that discussed
> > in either the design doc or the mocks, but given that this has
> > been proposed in the past and shot down it would be good to know
> > why we have changed.
> 
> Changed back to returning 0 for that case in the new patchset.
> 
> > This spec doesn't seem to account for theme colors either.  If you
> > look at:
> >
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > we used to serve out different assets depending on theme color and
> > that doesn't seem to be the case anymore.
> 
> Good point. It might be that we have to tint them differently based on the
> current theme color. That should be relatively doable. Does anyone happen to
> know of good examples for sites with theme colors, or should I make one?

tripadvisor.com and facebook.com are nice examples of contrast-y websites.

> 
> > Or what about colors that are hard to contrast with green (like red/green
> > for color blindness).
> 
> Any recommendations, design peeps?

In my experience the designers tend not to look at CL mail, you might want to
actually ping them. 

A proposal: I think we currently use a threshold to decide when to switch the
icon set for contrast reasons, no? We could use that same threshold and then
switch to the Incognito design.

> 
> > Then there is the CustomTabToolbar that also has their own logic for
> > showing lock icons.  We should pull out some more shared logic from
> > LocationBarLayout to share with that.  But even if we show an icon for
> > http in regular chrome, should we be doing that in CCT?  That is another
> > open UI question.
> 
> I was going to leave CCT for a subsequent CL. I like small CLs, where
possible.
> Do you think we should handle CCT in this one, though?

[chromium-reviews, 2016-06-20 01:11:09]

> In my experience the designers tend not to look at CL mail, you might want
> to
> actually ping them.
>

Yah. At this point, I think I've trained priority inbox to ignore them :)


> A proposal: I think we currently use a threshold to decide when to switch
> the
> icon set for contrast reasons, no? We could use that same threshold and
> then
> switch to the Incognito design.
>

I'll defer to Max here. I'm not sure if he was imagining something else.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2016-06-21 01:39:20]

See screenshots on the bug of Patchset 6.

Is everyone on board with handling CCT in a separate CL?

[felt, 2016-06-21 02:00:51]

On 2016/06/21 01:39:20, palmer wrote:
> See screenshots on the bug of Patchset 6.
> 
> Is everyone on board with handling CCT in a separate CL?

My interpretation of the thread w/ the UI team is that tablet should use the (i)
for HTTP, like desktop. The only difference is that on a phone-size device where
nothing is showing, that should remain the case.

[palmer, 2016-06-21 23:26:53]

> My interpretation of the thread w/ the UI team is that tablet should use the
(i)
> for HTTP, like desktop. The only difference is that on a phone-size device
where
> nothing is showing, that should remain the case.

OK, latest patchset does that.

[palmer, 2016-06-22 00:49:45]

> > for HTTP, like desktop. The only difference is that on a phone-size device
> where
> > nothing is showing, that should remain the case.
> 
> OK, latest patchset does that.

Should I do it for CCT, too? (I am thinking I'll just handle CCT in this CL,
too.)

[felt, 2016-06-22 04:13:41]

On 2016/06/22 00:49:45, palmer wrote:
> > > for HTTP, like desktop. The only difference is that on a phone-size device
> > where
> > > nothing is showing, that should remain the case.
> > 
> > OK, latest patchset does that.
> 
> Should I do it for CCT, too? (I am thinking I'll just handle CCT in this CL,
> too.)

i don't think CCT shows an icon for http right now, does it? i think the
intention is to keep the logic the same but swap out the icons

[palmer, 2016-06-22 18:57:01]

> i don't think CCT shows an icon for http right now, does it? i think the
> intention is to keep the logic the same but swap out the icons

OK. New patchset coming up that includes CCT.

[palmer, 2016-06-22 19:34:46]

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

[commit-bot: I haz the power, 2016-06-22 19:35:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063823005/140001

[commit-bot: I haz the power, 2016-06-22 20:59:17]

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

[commit-bot: I haz the power, 2016-06-22 20:59:18]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-06-22 23:47:26]

lgtm

[Ted C, 2016-06-22 23:49:02]

On 2016/06/22 23:47:26, Ted C wrote:
> lgtm

Oh before you submit this, did you run the new assets through our
png optimization?

Command:
tools/resources/optimize-png-files.sh -o2 <dir>

I would move all the new assets to a different dir and run them just
over them otherwise it will take...for...ev...er...

[palmer, 2016-06-23 01:10:34]

The CQ bit was checked by palmer@chromium.org

[palmer, 2016-06-23 01:10:34]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2063823005/#ps160001
(title: "Optimized PNGs.")

[commit-bot: I haz the power, 2016-06-23 01:11:10]

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

[commit-bot: I haz the power, 2016-06-23 01:51:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-23 01:53:13]

Description was changed from

==========
[android] New origin security indicator icons.

For the Omnibox.

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

to

==========
[android] New origin security indicator icons.

For the Omnibox.

BUG=604520

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

[commit-bot: I haz the power, 2016-06-23 01:53:14]

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