net: show UI warnings for SSLv3.

net: show UI warnings for SSLv3.

This change causes sites using SSLv3 to show a "mixed content" level warning on
the lock icon and for the section in the connection status to go red.

There's no message added to the connection status because, if we plan on
removing SSLv3 by default in Chrome 40, then we want to merge this to Chrome 39
and thus there's no time to get a translation.

BUG=419870
Committed: https://crrev.com/078d21029b8c7628d40dd9c98ca839f017ae1647
Cr-Commit-Position: refs/heads/master@{#300346}

[agl, 2014-10-17 22:06:37]

agl@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2014-10-17 22:27:45]

lgtm

https://codereview.chromium.org/662883002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/662883002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.cc:585: // there's no time
to get a translation.
Comment nit: There's a weird comma splice here, but also the pronouns.

I would delete the comment, since this will only be read on the branch (which no
one does) once we drop ssl3

[agl, 2014-10-17 23:25:36]

https://codereview.chromium.org/662883002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/662883002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.cc:585: // there's no time
to get a translation.
On 2014/10/17 22:27:45, Ryan Sleevi wrote:
> Comment nit: There's a weird comma splice here, but also the pronouns.
> 
> I would delete the comment, since this will only be read on the branch (which
no
> one does) once we drop ssl3

Done.

[agl, 2014-10-17 23:26:12]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-10-17 23:26:30]

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

[commit-bot: I haz the power, 2014-10-18 00:19:57]

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

[commit-bot: I haz the power, 2014-10-18 00:19:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[agl, 2014-10-20 17:37:55]

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

[msw, 2014-10-20 18:27:39]

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (left):

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:561: bool did_fallback =
(ssl.connection_status &
q: do you actually intend to remove SSL_CONNECTION_VERSION_FALLBACK as a trigger
for SITE_CONNECTION_STATUS_MIXED_CONTENT? (I'm not a knowledgeable reviewer for
this, but it caught my eye)

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:588:
IDS_PAGE_INFO_SECURITY_TAB_FALLBACK_MESSAGE);
The IDS and string asset should be removed if they're no longer used.

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:554: const bool is_sslv3
= ssl_version == net::SSL_CONNECTION_VERSION_SSL3;
nit: move this down to where it's used, or inline it.

[agl, 2014-10-20 18:55:20]

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (left):

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:562:
net::SSL_CONNECTION_VERSION_FALLBACK) != 0;
Yes, fallback to SSLv3 is getting removed in Chrome 39 (hopefully) thus we don't
need it any longer. We are now warning about SSLv3 in general to prepare for
removal in Chrome 40.

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:588:
IDS_PAGE_INFO_SECURITY_TAB_FALLBACK_MESSAGE);
On 2014/10/20 18:27:39, msw wrote:
> The IDS and string asset should be removed if they're no longer used.

I did plan on removing this message but, having slept on it, I've decided that
it's still useful, so I've moved |did_fallback| down here and added it back in.

[msw, 2014-10-20 19:14:06]

lgtm with an optional nit.

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (left):

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:562:
net::SSL_CONNECTION_VERSION_FALLBACK) != 0;
On 2014/10/20 18:55:19, agl wrote:
> Yes, fallback to SSLv3 is getting removed in Chrome 39 (hopefully) thus we
don't
> need it any longer. We are now warning about SSLv3 in general to prepare for
> removal in Chrome 40.

Acknowledged.

https://codereview.chromium.org/662883002/diff/20001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:588:
IDS_PAGE_INFO_SECURITY_TAB_FALLBACK_MESSAGE);
On 2014/10/20 18:55:19, agl wrote:
> On 2014/10/20 18:27:39, msw wrote:
> > The IDS and string asset should be removed if they're no longer used.
> 
> I did plan on removing this message but, having slept on it, I've decided that
> it's still useful, so I've moved |did_fallback| down here and added it back
in.

Acknowledged.

https://codereview.chromium.org/662883002/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/662883002/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:586:
(ssl.connection_status & net::SSL_CONNECTION_VERSION_FALLBACK) != 0;
optional nit: inline into the if statement.

[agl, 2014-10-20 21:30:35]

https://codereview.chromium.org/662883002/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/662883002/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:586:
(ssl.connection_status & net::SSL_CONNECTION_VERSION_FALLBACK) != 0;
On 2014/10/20 19:14:06, msw wrote:
> optional nit: inline into the if statement.

Ack.

I think the naming the value helps a little.

[agl, 2014-10-20 21:30:38]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-10-20 21:32:59]

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

[commit-bot: I haz the power, 2014-10-20 22:03:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-20 22:05:18]

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