Remove unneeded DCHECK for connection status.

chrome/browser/ui/website_settings/website_settings.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/website_settings/website_settings.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 419 matching lines @@
   }
 
   // Site Connection
   // We consider anything less than 80 bits encryption to be weak encryption.
   // TODO(wtc): Bug 1198735: report mixed/unsafe content for unencrypted and
   // weakly encrypted connections.
   site_connection_status_ = SITE_CONNECTION_STATUS_UNKNOWN;
 
   if (!ssl.cert_id) {
     // Not HTTPS.
-    DCHECK_EQ(ssl.security_style, content::SECURITY_STYLE_UNAUTHENTICATED);

[markusheintz_, 2014/02/24 14:20:36]

This DCHECK can still verify a precondition that we want to enforce.

The else below can just ensures some behavior in none debug builds that do not
contain the DCHECK.

I guess wtc@ should have a look at this since he knows the ssl code better. 

[wtc, 2014/02/24 22:56:05]

I haven't read this code for a long time, so I have to refresh my memory. Here
are my findings.

ssl.security_style is set in two places:

1. content/browser/ssl/ssl_policy.cc: If the URL scheme is not https, it sets
ssl.security_style to SECURITY_STYLE_UNAUTHENTICATED. If the URL scheme is https
but ssl.cert_id is 0, it sets ssl.security_style to
SECURITY_STYLE_UNAUTHENTICATED.

2. SSLBlockingPage::OverrideEntry (in chrome/browser/ssl/ssl_blocking_page.cc),
which sets ssl.security_style to SECURITY_STYLE_AUTHENTICATION_BROKEN. This is
only for SSL error interstitial pages.

Assuming that we only use an SSL error interstitial page for certificate errors,
it seems that !ssl.cert_id (which means no certificate) implies that
ssl.security_style is SECURITY_STYLE_UNAUTHENTICATED. However, the "Not HTTPS"
comment is wrong. It should say "HTTPS without a certificate, or not HTTPS".

The "HTTPS without a certificate" case can happen if we visit an HTTPS URL
through a proxy but we get an HTTP error response from the proxy server rather
than the destination server.

So, I'd rewrite this code as follows:

  if (!ssl.cert_id) {
    // HTTPS without a certificate, or not HTTPS.
    DCHECK_EQ(ssl.security_style, content::SECURITY_STYLE_UNAUTHENTICATED);
    site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;

    site_connection_details_.assign(l10n_util::GetStringFUTF16(
        IDS_PAGE_INFO_SECURITY_TAB_NOT_ENCRYPTED_CONNECTION_TEXT,
        subject_name));

or:

  if (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {
    // HTTPS without a certificate, or not HTTPS.
    DCHECK(!ssl.cert_id);
    site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;

    site_connection_details_.assign(l10n_util::GetStringFUTF16(
        IDS_PAGE_INFO_SECURITY_TAB_NOT_ENCRYPTED_CONNECTION_TEXT,
        subject_name));

I think the latter is more clear.

     if (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED)
       site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;
     else
       site_connection_status_ = SITE_CONNECTION_STATUS_ENCRYPTED_ERROR;
 
     site_connection_details_.assign(l10n_util::GetStringFUTF16(
         IDS_PAGE_INFO_SECURITY_TAB_NOT_ENCRYPTED_CONNECTION_TEXT,
         subject_name));
   } else if (ssl.security_bits < 0) {
     // Security strength is unknown.  Say nothing.
@@ skipping 229 matching lines @@
   if (visited_before_today) {
     first_visit_text = l10n_util::GetStringFUTF16(
         IDS_PAGE_INFO_SECURITY_TAB_VISITED_BEFORE_TODAY,
         base::TimeFormatShortDate(first_visit));
   } else {
     first_visit_text = l10n_util::GetStringUTF16(
         IDS_PAGE_INFO_SECURITY_TAB_FIRST_VISITED_TODAY);
   }
   ui_->SetFirstVisit(first_visit_text);
 }