MD Settings: Certificate manager, add "UNTRUSTED" labels.

MD Settings: Certificate manager, add "UNTRUSTED" labels.

BUG=598127
Committed: https://crrev.com/8eafae1cc7010ffabd318d3b96969b49cfef9d1a
Cr-Commit-Position: refs/heads/master@{#383650}

[dpapad, 2016-03-29 00:20:39]

dpapad@chromium.org changed reviewers:
+ dschuyler@chromium.org

[dpapad, 2016-03-29 00:20:40]

Hi Dave. Mostly followed your suggestions, screenshot at
http://imgur.com/UB4GZtN.
Specifically:
1) Using list-frame and list-item.unberbar
2) I did not add extra margin to reach 56px, instead I am relying on the 48px of
   list-frame, and need to follow up with whether
 - 48px is OK for this UI or
 - list-frame should be updated to 56px or
 - only this UI should use 56px and list-frame should not be modified.
3) Ended up not using the ".list-item > .middle" style because it brings an
   unwanted margin. I could use it, but I would have to override the margin.
   Seemed better to just not use that style.

[dschuyler, 2016-03-29 00:39:18]

LGTM with some suggestions/requests.

https://codereview.chromium.org/1842473004/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1842473004/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:249: <message
name="IDS_SETTINGS_CERTIFICATE_MANAGER_UNTRUSTED" desc="This text is displayed
next to untrusted certificates in a red box. Untrusted certificates are
sometimes added in order to explicitly distrust them. Thus, they will be listed
but it's important that the user note that they serve a different purpose from
the rest.">
This is an excellent comment, but it may be in
the wrong place (or I may be misunderstanding).
I *think* the desc="" is to help the person
doing localization of the text. This feels more
like a note to another programmer rather than a
translator (WDYT?)

https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html
(right):

https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html:25:
color: var(--paper-red-700);
Please consider adding a --settings variable
at the top of settings_shared_css.html for an
error color and then referring to it here.  I
believe we will need an error color in more
places.  (I think Default Browser may have an
old 'color: red;' icon that needs to be changed,
for example).

maybe --settings-error-color?

This can be done separately, but it would be
cooler to do it here :)

https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html:32:
background-color: var(--google-grey-300);
Same-ish comment about using a color variable
defined in settings_shared_css.html (or making
a new one).

maybe --settings-hover-color?

[dpapad, 2016-03-29 00:57:54]

https://codereview.chromium.org/1842473004/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1842473004/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:249: <message
name="IDS_SETTINGS_CERTIFICATE_MANAGER_UNTRUSTED" desc="This text is displayed
next to untrusted certificates in a red box. Untrusted certificates are
sometimes added in order to explicitly distrust them. Thus, they will be listed
but it's important that the user note that they serve a different purpose from
the rest.">
On 2016/03/29 at 00:39:17, dschuyler wrote:
> This is an excellent comment, but it may be in
> the wrong place (or I may be misunderstanding).
> I *think* the desc="" is to help the person
> doing localization of the text. This feels more
> like a note to another programmer rather than a
> translator (WDYT?)

I agree with your assessment. Initially I did not think about it too much, just
copied it over from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat....
Removed the part of the comment that is not helpful for translation. Not sure if
it is worth moving the removed part elsewhere in the code. Let me know if you
have a suggestion.

https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html
(right):

https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html:25:
color: var(--paper-red-700);
On 2016/03/29 at 00:39:17, dschuyler wrote:
> Please consider adding a --settings variable
> at the top of settings_shared_css.html for an
> error color and then referring to it here.  I
> believe we will need an error color in more
> places.  (I think Default Browser may have an
> old 'color: red;' icon that needs to be changed,
> for example).
> 
> maybe --settings-error-color?
> 
> This can be done separately, but it would be
> cooler to do it here :)

Done.

https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html:32:
background-color: var(--google-grey-300);
On 2016/03/29 at 00:39:17, dschuyler wrote:
> Same-ish comment about using a color variable
> defined in settings_shared_css.html (or making
> a new one).
> 
> maybe --settings-hover-color?

Done.

[dschuyler, 2016-03-29 01:04:48]

On 2016/03/29 00:57:54, dpapad wrote:
>
https://codereview.chromium.org/1842473004/diff/20001/chrome/app/settings_str...
> File chrome/app/settings_strings.grdp (right):
> 
>
https://codereview.chromium.org/1842473004/diff/20001/chrome/app/settings_str...
> chrome/app/settings_strings.grdp:249: <message
> name="IDS_SETTINGS_CERTIFICATE_MANAGER_UNTRUSTED" desc="This text is displayed
> next to untrusted certificates in a red box. Untrusted certificates are
> sometimes added in order to explicitly distrust them. Thus, they will be
listed
> but it's important that the user note that they serve a different purpose from
> the rest.">
> On 2016/03/29 at 00:39:17, dschuyler wrote:
> > This is an excellent comment, but it may be in
> > the wrong place (or I may be misunderstanding).
> > I *think* the desc="" is to help the person
> > doing localization of the text. This feels more
> > like a note to another programmer rather than a
> > translator (WDYT?)
> 
> I agree with your assessment. Initially I did not think about it too much,
just
> copied it over from
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat....
> Removed the part of the comment that is not helpful for translation. Not sure
if
> it is worth moving the removed part elsewhere in the code. Let me know if you
> have a suggestion.
> 
>
https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
> File
>
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html
> (right):
> 
>
https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html:25:
> color: var(--paper-red-700);
> On 2016/03/29 at 00:39:17, dschuyler wrote:
> > Please consider adding a --settings variable
> > at the top of settings_shared_css.html for an
> > error color and then referring to it here.  I
> > believe we will need an error color in more
> > places.  (I think Default Browser may have an
> > old 'color: red;' icon that needs to be changed,
> > for example).
> > 
> > maybe --settings-error-color?
> > 
> > This can be done separately, but it would be
> > cooler to do it here :)
> 
> Done.
> 
>
https://codereview.chromium.org/1842473004/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html:32:
> background-color: var(--google-grey-300);
> On 2016/03/29 at 00:39:17, dschuyler wrote:
> > Same-ish comment about using a color variable
> > defined in settings_shared_css.html (or making
> > a new one).
> > 
> > maybe --settings-hover-color?
> 
> Done.

Thanks! still LGTM.  I think there's no need to change
the old version of that string desc.  Those versions of
the strings are planning to be removed when the MD goes
in.  And they've already been translated afaik, so a
change might not be seen by a translator (for the old
files).

[dpapad, 2016-03-29 01:06:05]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-29 01:06:13]

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

[commit-bot: I haz the power, 2016-03-29 01:48:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-29 01:49:37]

Description was changed from

==========
MD Settings: Certificate manager, add "UNTRUSTED" labels.

BUG=598127
==========

to

==========
MD Settings: Certificate manager, add "UNTRUSTED" labels.

BUG=598127

Committed: https://crrev.com/8eafae1cc7010ffabd318d3b96969b49cfef9d1a
Cr-Commit-Position: refs/heads/master@{#383650}
==========

[commit-bot: I haz the power, 2016-03-29 01:49:38]

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