Remove obsolete SHA-1 UX elements

Remove obsolete SHA-1 UX elements

In the past, Chrome provided a "minor" warning state for certificate
chains containing SHA-1 certificates that expired before 2017. It's now
2017, so all such certificates have expired, and this UX is no longer
needed.

BUG=676680
Review-Url: https://codereview.chromium.org/2616553002
Cr-Commit-Position: refs/heads/master@{#442597}
Committed: https://chromium.googlesource.com/chromium/src/+/be87bd63a20be57d92589cd382263623e978af9f

[elawrence, 2017-01-03 23:09:58]

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

[commit-bot: I haz the power, 2017-01-03 23:10:40]

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, 2017-01-04 00:07:29]

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

[commit-bot: I haz the power, 2017-01-04 00:07:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[elawrence, 2017-01-04 02:04:15]

Patchset #1 (id:1) has been deleted

[elawrence, 2017-01-04 02:04:25]

Patchset #1 (id:20001) has been deleted

[elawrence, 2017-01-04 02:04:57]

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

[commit-bot: I haz the power, 2017-01-04 02:05:14]

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, 2017-01-04 02:56:13]

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

[commit-bot: I haz the power, 2017-01-04 02:56:14]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-01-04 16:35:02]

elawrence@chromium.org changed reviewers:
+ estark@chromium.org

[elawrence, 2017-01-04 16:35:03]

PTAL?

[elawrence, 2017-01-04 17:41:27]

On 2017/01/04 16:35:03, elawrence wrote:
> PTAL?

One issue is that IDS_PAGE_INFO_SECURITY_TAB_DEPRECATED_SIGNATURE_ALGORITHM
appears not to actually be used anywhere; WebsiteSettings::Init uses it when
computing a string for a tab (the "Connection" tab of the pageinfo flyout) that
no longer exists. I'm not sure whether I should try to clean all of that up in
this CL or let Lucas do it as a part of crbug.com/571533

[estark, 2017-01-05 16:05:16]

On 2017/01/04 17:41:27, elawrence wrote:
> On 2017/01/04 16:35:03, elawrence wrote:
> > PTAL?
> 
> One issue is that IDS_PAGE_INFO_SECURITY_TAB_DEPRECATED_SIGNATURE_ALGORITHM
> appears not to actually be used anywhere; WebsiteSettings::Init uses it when
> computing a string for a tab (the "Connection" tab of the pageinfo flyout)
that
> no longer exists. I'm not sure whether I should try to clean all of that up in
> this CL or let Lucas do it as a part of crbug.com/571533

It looks like it is still used on Android; it gets put in
|identity_status_description| [1] which gets displayed in the android connection
info popup [2].

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/website_settings/websi...
[2]
https://cs.chromium.org/chromium/src/chrome/browser/ui/android/page_info/conn...

[estark, 2017-01-05 16:18:42]

https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
components/security_state/content/content_utils.cc:235:
security_style_explanations->broken_explanations.push_back(
I might be getting myself confused, but I think this should be in
|unauthenticated_explanations| (that field is badly named... it should be
|neutral_explanations| or something) and should only be included if there is no
cert error.

Usually SHA1 will be treated as a cert error and I don't know if we need a
special explanation for it; it will be covered by the generic certificate error
explanation on line 283. (Or maybe we want a special SHA1 explanation on line
283 if SHA1 is the only certificate error.)

This path here should explain the case where the security level is neutral when
the policy is set, which is why I think it should be in
|unauthenticated_explanations| and only when SHA1 isn't being treated as a cert
error.

https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
components/security_state/core/security_state.h:114: SHA1DeprecationStatus
sha1_deprecation_status;
Maybe this should be a bool rather than an enum now

https://codereview.chromium.org/2616553002/diff/60001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/page_info_model.cc (left):

https://codereview.chromium.org/2616553002/diff/60001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/page_info_model.cc:122: // The date after which no
new SHA-1 certificates may be issued.
Ah, sad, I didn't know we had this code...

[elawrence, 2017-01-06 21:54:26]

> It looks like it is still used on Android; it gets put in
> |identity_status_description| [1] which gets displayed in the android
connection
> info popup [2].

Ah. I couldn't entice that string to show; I did see
IDS_CERT_ERROR_WEAK_SIGNATURE_ALGORITHM_DESCRIPTION when clicking the red /!\
and then "Details." But presumably the
IDS_PAGE_INFO_SECURITY_TAB_DEPRECATED_SIGNATURE_ALGORITHM string would appear if
the EnableSha1ForLocalAnchors policy was enabled on my Android device.

> security_style_explanations->broken_explanations.push_back(
> I might be getting myself confused, but I think this should be in
> |unauthenticated_explanations| (that field is badly named... it should be
> |neutral_explanations| or something) and should only be included if there is
no
> cert error.

Will Fix. The impact here for a non-public PKI cert is small (the red triangle
adjacent to the "SHA-1 Certificate" entry in the DevTools Security Tab becomes a
red circle. For a public PKI cert, this change swaps the triangle for a circle
and also moves the "SHA-1 Certificate" entry below the [Certificate Error
net::ERR_CERT_WEAK_SIGNATURE_ALGORITHM] entry. So this seems like a righteous
update. 

> Usually SHA1 will be treated as a cert error and I don't know if we need a
> special explanation for it; it will be covered by the generic certificate
error
> explanation on line 283. (Or maybe we want a special SHA1 explanation on line
> 283 if SHA1 is the only certificate error.)
> This path here should explain the case where the security level is neutral
when
> the policy is set, which is why I think it should be in
> |unauthenticated_explanations| and only when SHA1 isn't being treated as a
cert
> error.

As of M57, there are two scenarios in which SHA1 will not be treated as a
certificate error:

1> If the SHA-1 certificate chains to a non-public PKI and the
EnableSha1ForLocalAnchors policy is set,
2> If the user is on Windows 7 and the SHA-1 is present in an intermediate
certificate but not the leaf cert (a temporary hack via
AreSHA1IntermediatesAllowed)

I think it's a good thing to call out specifically that the problem is the use
of SHA-1, even if we're also showing the ERR_CERT_WEAK_SIGNATURE_ALGORITHM
explanation just above. 

It would be trivial to suppress it if you feel strongly that we should show only
the less specific error? 

>
https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
> File components/security_state/core/security_state.h (right):
> 
>
https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
> components/security_state/core/security_state.h:114: SHA1DeprecationStatus
> sha1_deprecation_status;
> Maybe this should be a bool rather than an enum now

Yes, I think so. I overlooked the fact that security_state::UNKNOWN_SHA1 isn't
really a valid value and triggers a DCHECK. I'll move this to a bool that
defaults to false.

>
https://codereview.chromium.org/2616553002/diff/60001/ios/chrome/browser/ui/o...
> Ah, sad, I didn't know we had this code...

It's not entirely clear to me that the iOS code is reachable any more, based on
the remarks here:
https://bugs.chromium.org/p/chromium/issues/detail?id=657225#c8. But I figured
I'd preserve the pattern for this CL just-in-case and let any future cleanup in
the iOS code happen in a future CL.

[estark, 2017-01-08 16:39:45]

Thanks, this is looking good, I just have some nits and a request for a couple
additional tests (see below). Sorry for missing some of these on my previous
pass.

On 2017/01/06 21:54:26, elawrence wrote:
> > It looks like it is still used on Android; it gets put in
> > |identity_status_description| [1] which gets displayed in the android
> connection
> > info popup [2].
> 
> Ah. I couldn't entice that string to show; I did see
> IDS_CERT_ERROR_WEAK_SIGNATURE_ALGORITHM_DESCRIPTION when clicking the red /!\
> and then "Details." But presumably the
> IDS_PAGE_INFO_SECURITY_TAB_DEPRECATED_SIGNATURE_ALGORITHM string would appear
if
> the EnableSha1ForLocalAnchors policy was enabled on my Android device.
> 
> > security_style_explanations->broken_explanations.push_back(
> > I might be getting myself confused, but I think this should be in
> > |unauthenticated_explanations| (that field is badly named... it should be
> > |neutral_explanations| or something) and should only be included if there is
> no
> > cert error.
> 
> Will Fix. The impact here for a non-public PKI cert is small (the red triangle
> adjacent to the "SHA-1 Certificate" entry in the DevTools Security Tab becomes
a
> red circle. For a public PKI cert, this change swaps the triangle for a circle
> and also moves the "SHA-1 Certificate" entry below the [Certificate Error
> net::ERR_CERT_WEAK_SIGNATURE_ALGORITHM] entry. So this seems like a righteous
> update. 
> 
> > Usually SHA1 will be treated as a cert error and I don't know if we need a
> > special explanation for it; it will be covered by the generic certificate
> error
> > explanation on line 283. (Or maybe we want a special SHA1 explanation on
line
> > 283 if SHA1 is the only certificate error.)
> > This path here should explain the case where the security level is neutral
> when
> > the policy is set, which is why I think it should be in
> > |unauthenticated_explanations| and only when SHA1 isn't being treated as a
> cert
> > error.
> 
> As of M57, there are two scenarios in which SHA1 will not be treated as a
> certificate error:
> 
> 1> If the SHA-1 certificate chains to a non-public PKI and the
> EnableSha1ForLocalAnchors policy is set,
> 2> If the user is on Windows 7 and the SHA-1 is present in an intermediate
> certificate but not the leaf cert (a temporary hack via
> AreSHA1IntermediatesAllowed)
> 
> I think it's a good thing to call out specifically that the problem is the use
> of SHA-1, even if we're also showing the ERR_CERT_WEAK_SIGNATURE_ALGORITHM
> explanation just above. 
> 
> It would be trivial to suppress it if you feel strongly that we should show
only
> the less specific error?

That seems reasonable to me, I agree that it's useful to call out the specific
problem for SHA1. Just one request that you add test cases for these though (the
error and non-error cases) -- I left comments about that inline.
 
> 
> >
>
https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
> > File components/security_state/core/security_state.h (right):
> > 
> >
>
https://codereview.chromium.org/2616553002/diff/60001/components/security_sta...
> > components/security_state/core/security_state.h:114: SHA1DeprecationStatus
> > sha1_deprecation_status;
> > Maybe this should be a bool rather than an enum now
> 
> Yes, I think so. I overlooked the fact that security_state::UNKNOWN_SHA1 isn't
> really a valid value and triggers a DCHECK. I'll move this to a bool that
> defaults to false.
> 
> >
>
https://codereview.chromium.org/2616553002/diff/60001/ios/chrome/browser/ui/o...
> > Ah, sad, I didn't know we had this code...
> 
> It's not entirely clear to me that the iOS code is reachable any more, based
on
> the remarks here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=657225#c8. But I figured
> I'd preserve the pattern for this CL just-in-case and let any future cleanup
in
> the iOS code happen in a future CL.

SG, I agree it's better to do the cleanup in a separate CL.

[estark, 2017-01-08 16:39:59]

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right):

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:240:
EXPECT_EQ(false, security_info.sha1_in_chain);
nit: EXPECT_FALSE

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:371:
EXPECT_EQ(false, security_info.sha1_in_chain);
nit: EXPECT_FALSE

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:403:
security_state::NONE, true, security_state::CONTENT_STATUS_NONE, false,
Could you add:
- a SecurityStyleTestObserver to this test and check that its
latest_explanations() are as expected at this point? i.e. containing an
|unauthenticated_explanation| for SHA1  and no |broken_explanations|
- a test like this one but with WEAK_SIGNATURE_ALGORITHM and
SHA1_SIGNATURE_PRESENT to test the latest_explanations() for the error case?
i.e. a |broken_explanation| for a cert error with an explanation for SHA1 as
well?

(lmk if it's not obvious how to use SecurityStyleTestObserver to do this; there
are some examples elsewhere in this file)

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:581: // Same as
the test above but with a SHA1 cert.
nit: while you are here, could you change "test above" to
"SecurityStateTabHelperTest.ActiveAndPassiveContentWithCertErrors"?

https://codereview.chromium.org/2616553002/diff/100001/components/pageinfo_st...
File components/pageinfo_strings.grdp (right):

https://codereview.chromium.org/2616553002/diff/100001/components/pageinfo_st...
components/pageinfo_strings.grdp:6: <message
name="IDS_PAGE_INFO_SECURITY_TAB_DEPRECATED_SIGNATURE_ALGORITHM" desc="The
security summary phrase in the page information panel for a security problem
where the site's certificate chain contains a SHA1 signature. Such certificates
are not supported except when a policy override is present.">
optional nit: "not supported" => "treated as errors"
(not sure if that's actually better or not)

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
File components/security_state/core/security_state.cc (right):

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state.cc:93: return true;
nit: could simplify as

return visible_security_state.certificate && (visible_security_state.cert_status
& net::CERT_STATUS_SHA1_SIGNATURE_PRESENT);

Also I'd probably just inline this in the one callsite below and not split it
out into a separate function.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state.h:104: // True if a SHA1 signature
was observed anywhere in the certificate chain
nit: period at the end

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
File components/security_state/core/security_state_unittest.cc (right):

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:117: 
Could you add a test for the cert-error SHA1 case, testing that if cert_status
includes WEAK_SIGNATURE_ALGORITHM (the error status) *and*
SHA1_SIGNATURE_PRESENT, the security level is DANGEROUS?

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:118: // Tests that
SHA1-signed certificates expiring in 2016 downgrade the
the "expiring in 2016" bit should be removed, yeah?

Also maybe this should clarify that this is the non-error SHA1 case, something
like:
"Tests that SHA1-signed certificates, when not treated as a certificate error
such as when allowed by policy, downgrade the security state of the page to
NONE."

And ditto on the tests below.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:124: EXPECT_EQ(true,
security_info.sha1_in_chain);
nit: EXPECT_TRUE

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:135: EXPECT_EQ(true,
security_info1.sha1_in_chain);
nit: EXPECT_TRUE

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:143: EXPECT_EQ(true,
security_info2.sha1_in_chain);
nit: EXPECT_TRUE

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:155: EXPECT_EQ(true,
security_info.sha1_in_chain);
nit: EXPECT_TRUE

[elawrence, 2017-01-09 18:12:17]

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

[commit-bot: I haz the power, 2017-01-09 18:12:43]

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

[elawrence, 2017-01-09 18:13:12]

PTAL?

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right):

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:240:
EXPECT_EQ(false, security_info.sha1_in_chain);
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> nit: EXPECT_FALSE

Done.

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:371:
EXPECT_EQ(false, security_info.sha1_in_chain);
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> nit: EXPECT_FALSE

Done.

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:403:
security_state::NONE, true, security_state::CONTENT_STATUS_NONE, false,
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> Could you add:
> - a SecurityStyleTestObserver to this test and check that its
> latest_explanations() are as expected at this point? i.e. containing an
> |unauthenticated_explanation| for SHA1  and no |broken_explanations|
> - a test like this one but with WEAK_SIGNATURE_ALGORITHM and
> SHA1_SIGNATURE_PRESENT to test the latest_explanations() for the error case?
> i.e. a |broken_explanation| for a cert error with an explanation for SHA1 as
> well?
> 
> (lmk if it's not obvious how to use SecurityStyleTestObserver to do this;
there
> are some examples elsewhere in this file)

Done.

https://codereview.chromium.org/2616553002/diff/100001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:581: // Same as
the test above but with a SHA1 cert.
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> nit: while you are here, could you change "test above" to
> "SecurityStateTabHelperTest.ActiveAndPassiveContentWithCertErrors"?

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/pageinfo_st...
File components/pageinfo_strings.grdp (right):

https://codereview.chromium.org/2616553002/diff/100001/components/pageinfo_st...
components/pageinfo_strings.grdp:6: <message
name="IDS_PAGE_INFO_SECURITY_TAB_DEPRECATED_SIGNATURE_ALGORITHM" desc="The
security summary phrase in the page information panel for a security problem
where the site's certificate chain contains a SHA1 signature. Such certificates
are not supported except when a policy override is present.">
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> optional nit: "not supported" => "treated as errors"
> (not sure if that's actually better or not)

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
File components/security_state/core/security_state.cc (right):

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state.cc:93: return true;
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> nit: could simplify as
> 
> return visible_security_state.certificate &&
(visible_security_state.cert_status
> & net::CERT_STATUS_SHA1_SIGNATURE_PRESENT);
> 
> Also I'd probably just inline this in the one callsite below and not split it
> out into a separate function.

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state.h:104: // True if a SHA1 signature
was observed anywhere in the certificate chain
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> nit: period at the end

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
File components/security_state/core/security_state_unittest.cc (right):

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:117: 
On 2017/01/08 16:39:59, estark (slow thru Jan 6) wrote:
> Could you add a test for the cert-error SHA1 case, testing that if cert_status
> includes WEAK_SIGNATURE_ALGORITHM (the error status) *and*
> SHA1_SIGNATURE_PRESENT, the security level is DANGEROUS?

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:118: // Tests that
SHA1-signed certificates expiring in 2016 downgrade the
On 2017/01/08 16:39:58, estark (slow thru Jan 6) wrote:
> the "expiring in 2016" bit should be removed, yeah?
> 
> Also maybe this should clarify that this is the non-error SHA1 case, something
> like:
> "Tests that SHA1-signed certificates, when not treated as a certificate error
> such as when allowed by policy, downgrade the security state of the page to
> NONE."
> 
> And ditto on the tests below.

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:124: EXPECT_EQ(true,
security_info.sha1_in_chain);
On 2017/01/08 16:39:59, estark (slow thru Jan 6) wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:135: EXPECT_EQ(true,
security_info1.sha1_in_chain);
On 2017/01/08 16:39:59, estark (slow thru Jan 6) wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:143: EXPECT_EQ(true,
security_info2.sha1_in_chain);
On 2017/01/08 16:39:59, estark (slow thru Jan 6) wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/2616553002/diff/100001/components/security_st...
components/security_state/core/security_state_unittest.cc:155: EXPECT_EQ(true,
security_info.sha1_in_chain);
On 2017/01/08 16:39:59, estark (slow thru Jan 6) wrote:
> nit: EXPECT_TRUE

Done.

[commit-bot: I haz the power, 2017-01-09 19:13:08]

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

[commit-bot: I haz the power, 2017-01-09 19:13:09]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-01-09 19:16:11]

elawrence@chromium.org changed reviewers:
+ jochen@chromium.org, rohitrao@chromium.org

[elawrence, 2017-01-09 19:16:12]

jochen@chromium.org: Please review change in pageinfo_strings.grdp

rohitrao@chromium.org: Please review changes in page_info_model.cc

Thanks!

[rohitrao (ping after 24h), 2017-01-09 20:07:54]

ios LGTM

[estark, 2017-01-09 23:00:21]

lgtm w/ a couple small nits

https://codereview.chromium.org/2616553002/diff/120001/chrome/browser/ssl/sec...
File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right):

https://codereview.chromium.org/2616553002/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:394: // Test a
SHA-1 certificate
Unfinished comment

https://codereview.chromium.org/2616553002/diff/120001/chrome/browser/ssl/sec...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:428: ASSERT_EQ(1u,
page_explanation.unauthenticated_explanations.size());
Optional nit: other tests in this file check the actual string in the
explanations, a la line 149 in CheckSecureExplanations().

I'm kind of torn on whether that's actually a good testing strategy -- it feels
a little brittle and/or like it's testing an internal implementation detail --
which is why I'm mentioning it as an optional nit only.

[elawrence, 2017-01-09 23:13:35]

Patchset #6 (id:140001) has been deleted

[jochen (gone - plz use gerrit), 2017-01-10 09:50:59]

lgtm

[elawrence, 2017-01-10 14:31:48]

The CQ bit was checked by elawrence@chromium.org

[elawrence, 2017-01-10 14:31:49]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2616553002/#ps160001
(title: "Final nits")

[commit-bot: I haz the power, 2017-01-10 14:32:14]

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, 2017-01-10 16:09:02]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484058708840580,
"parent_rev": "d4e43bb5146c7a59123ff5f3b6e853d8e8c29130", "commit_rev":
"be87bd63a20be57d92589cd382263623e978af9f"}

[commit-bot: I haz the power, 2017-01-10 16:09:33]

Description was changed from

==========
Remove obsolete SHA-1 UX elements

In the past, Chrome provided a "minor" warning state for certificate
chains containing SHA-1 certificates that expired before 2017. It's now
2017, so all such certificates have expired, and this UX is no longer
needed.

BUG=676680
==========

to

==========
Remove obsolete SHA-1 UX elements

In the past, Chrome provided a "minor" warning state for certificate
chains containing SHA-1 certificates that expired before 2017. It's now
2017, so all such certificates have expired, and this UX is no longer
needed.

BUG=676680

Review-Url: https://codereview.chromium.org/2616553002
Cr-Commit-Position: refs/heads/master@{#442597}
Committed:
https://chromium.googlesource.com/chromium/src/+/be87bd63a20be57d92589cd38226...
==========

[commit-bot: I haz the power, 2017-01-10 16:09:35]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/be87bd63a20be57d92589cd38226...