Material Page Info (all desktop): Introduce new strings and styling info for the security section.

Material Page Info (all desktop): Introduce new strings and styling info for the security section.

This is the common code shared between platforms. Mac and Views will have separate CLs based on this.

BUG=512442
Committed: https://crrev.com/bd3c00418b2d5fb2a42b5612c089225200b6967f
Cr-Commit-Position: refs/heads/master@{#417816}

[lgarron, 2016-08-23 15:29:56]

lgarron@chromium.org changed reviewers:
+ felt@chromium.org

[lgarron, 2016-08-23 15:29:56]

felt@, could you review?

[felt, 2016-08-23 15:51:07]

lgtm

https://codereview.chromium.org/2262223002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2262223002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:383: +      <message
name="IDS_WEBSITE_SETTINGS_NUM_COOKIES" desc="The label of the counts for
allowed cookies that are in use on the page..">
nit: double .. at the end

https://codereview.chromium.org/2262223002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://codereview.chromium.org/2262223002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:122: // Deprecated
method go get just the summary from GetSecurityDescription().
nit: grammar

https://codereview.chromium.org/2262223002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.h:123: base::string16
GetSecuritySummary() const;
will GetSecuritySummary() be removed in a follow up CL?

[msw, 2016-08-25 03:57:01]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2016-08-25 03:57:02]

https://codereview.chromium.org/2262223002/diff/60001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2262223002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:22: const SkColor
COLOR_UNSTYLED = SkColorSetRGB(0x00, 0x00, 0x00);
The way these colors are set seems wrong; see my comments on
https://codereview.chromium.org/2278513003/

[lgarron, 2016-08-25 04:18:24]

https://codereview.chromium.org/2262223002/diff/60001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/2262223002/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:22: const SkColor
COLOR_UNSTYLED = SkColorSetRGB(0x00, 0x00, 0x00);
On 2016/08/25 at 03:57:01, msw wrote:
> The way these colors are set seems wrong; see my comments on
https://codereview.chromium.org/2278513003/

Also see my comment there. :-)

Anyhow, I have just now discovered that color_palette.h [1] exists. I've
switched to that, to share color definitions.

[1]
https://cs.chromium.org/chromium/src/ui/gfx/color_palette.h?l=24&cl=GROK&gsn=...

[lgarron, 2016-08-25 04:22:46]

Patchset #6 (id:100001) has been deleted

[lgarron, 2016-08-27 02:00:51]

Description was changed from

==========
Material Page Info: introduce new strings and styling info.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442
==========

to

==========
Material Page Info: Introduce new strings and styling info.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442
==========

[lgarron, 2016-08-27 02:01:16]

Description was changed from

==========
Material Page Info: Introduce new strings and styling info.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442
==========

to

==========
Material Page Info: Introduce new strings and styling info for the security
section.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442
==========

[lgarron, 2016-08-27 02:18:32]

Description was changed from

==========
Material Page Info: Introduce new strings and styling info for the security
section.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442
==========

to

==========
Material Page Info (all desktop): Introduce new strings and styling info for the
security section.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442
==========

[lgarron, 2016-09-01 20:28:36]

https://codereview.chromium.org/2262223002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2262223002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8966: +      <message
name="IDS_PAGEINFO_RESET_INVALID_CERTIFICATE_DECISIONS_BUTTON" desc="Text of
button in the page info that resets allow/deny decisions of invalid
certificates, which will start showing security warnings for the page again.">
This will change the Android button from "STOP USING AN INVALID CERTIFICATE" TO
RE-ENABLE WARNINGS".
The sentence "You have chosen to disable security warnings for this site." is
not present for context.

felt@, jww@, do you have a preference between 1) keeping the string intact on
Android or 2) consistency with desktop?

[felt, 2016-09-06 18:40:33]

https://codereview.chromium.org/2262223002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2262223002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8966: +      <message
name="IDS_PAGEINFO_RESET_INVALID_CERTIFICATE_DECISIONS_BUTTON" desc="Text of
button in the page info that resets allow/deny decisions of invalid
certificates, which will start showing security warnings for the page again.">
On 2016/09/01 20:28:36, lgarron wrote:
> This will change the Android button from "STOP USING AN INVALID CERTIFICATE"
TO
> RE-ENABLE WARNINGS".
> The sentence "You have chosen to disable security warnings for this site." is
> not present for context.
> 
> felt@, jww@, do you have a preference between 1) keeping the string intact on
> Android or 2) consistency with desktop?

I answered this in person, but putting here for posterity: my preference is to
have it make sense on Android, I am OK with it being very slightly different
across platforms if that's what makes the most sense in each context.

[jww, 2016-09-06 22:30:57]

jww@chromium.org changed reviewers:
+ jww@chromium.org

[jww, 2016-09-06 22:30:57]

https://codereview.chromium.org/2262223002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2262223002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8966: +      <message
name="IDS_PAGEINFO_RESET_INVALID_CERTIFICATE_DECISIONS_BUTTON" desc="Text of
button in the page info that resets allow/deny decisions of invalid
certificates, which will start showing security warnings for the page again.">
On 2016/09/06 18:40:32, felt wrote:
> On 2016/09/01 20:28:36, lgarron wrote:
> > This will change the Android button from "STOP USING AN INVALID CERTIFICATE"
> TO
> > RE-ENABLE WARNINGS".
> > The sentence "You have chosen to disable security warnings for this site."
is
> > not present for context.
> > 
> > felt@, jww@, do you have a preference between 1) keeping the string intact
on
> > Android or 2) consistency with desktop?
> 
> I answered this in person, but putting here for posterity: my preference is to
> have it make sense on Android, I am OK with it being very slightly different
> across platforms if that's what makes the most sense in each context.

I agree with felt@. Consistency only makes sense if... it makes sense, IMO ;-)

[lgarron, 2016-09-10 01:27:31]

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

[commit-bot: I haz the power, 2016-09-10 01:27:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 02:13:05]

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

[commit-bot: I haz the power, 2016-09-10 02:13:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lgarron, 2016-09-10 02:47:14]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2016-09-10 02:47:14]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org
Link to the patchset: https://codereview.chromium.org/2262223002/#ps180001
(title: "Move tests from followup CL into this one.")

[commit-bot: I haz the power, 2016-09-10 02:47:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 03:35:09]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-09-10 03:38:21]

Description was changed from

==========
Material Page Info (all desktop): Introduce new strings and styling info for the
security section.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442
==========

to

==========
Material Page Info (all desktop): Introduce new strings and styling info for the
security section.

This is the common code shared between platforms. Mac and Views will have
separate CLs based on this.

BUG=512442

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

[commit-bot: I haz the power, 2016-09-10 03:38:25]

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