DevTools security panel: explain subresources with cert errors separately

chrome/browser/ssl/chrome_security_state_model_client.cc

Index: chrome/browser/ssl/chrome_security_state_model_client.cc
diff --git a/chrome/browser/ssl/chrome_security_state_model_client.cc b/chrome/browser/ssl/chrome_security_state_model_client.cc
index 72bf6d6ff15f81942c291172560dd0695b76be45..8b7d731fd7ff786d2edb31d2f10670c09b29d5a7 100644
--- a/chrome/browser/ssl/chrome_security_state_model_client.cc
+++ b/chrome/browser/ssl/chrome_security_state_model_client.cc
@@ -204,18 +204,40 @@ content::SecurityStyle ChromeSecurityStateModelClient::GetSecurityStyle(
             security_info.cert_id));
   }
 
-  security_style_explanations->ran_insecure_content =
+  // Record the presence of mixed content (HTTP subresources on an HTTPS
+  // page).
+  security_style_explanations->ran_mixed_content =
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_RAN ||
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
-  security_style_explanations->displayed_insecure_content =
+  security_style_explanations->displayed_mixed_content =
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_DISPLAYED ||
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
 
-  if (net::IsCertStatusError(security_info.cert_status)) {
+  bool is_cert_status_error = net::IsCertStatusError(security_info.cert_status);
+  bool is_cert_status_minor_error =
+      net::IsCertStatusMinorError(security_info.cert_status);
+
+  // Record the presence of content with certificate errors (HTTPS
+  // subresources that were loaded with certificate errors, on an HTTPS
+  // page that was loaded without major certificate errors).
+  if (!is_cert_status_error || is_cert_status_minor_error) {

[felt, 2016/08/30 16:28:52]

should that be an && instead of an ||?

[estark, 2016/08/30 16:36:57]

Nope, I think || is right. |is_cert_status_error| is true if there is any error
(major or minor). So this condition is supposed to be true if there were no cert
errors at all, or if there was only a minor cert error.

[felt, 2016/08/30 16:41:41]

That's what I'm confused about -- you want this to be populated if there were no
cert errors at all? The comment says that this is for recording cert errors,
which doesn't make sense when there are no cert errors. Could you update the
comment?

[estark, 2016/08/30 17:27:58]

Ahhh, ok, I think I understand the confusion now.
ran/displayed_content_with_cert_errors record the presence of subresources with
cert errors, but we only populate those fields if the main resource was loaded
without cert errors. I updated the comment to try to explain this better, lmk if
it makes sense or not.

+    security_style_explanations->ran_content_with_cert_errors =
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_RAN ||
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
+    security_style_explanations->displayed_content_with_cert_errors =
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_DISPLAYED ||
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
+  }
+
+  if (is_cert_status_error) {
     base::string16 error_string = base::UTF8ToUTF16(net::ErrorToString(
         net::MapCertStatusToNetError(security_info.cert_status)));
 
@@ -225,11 +247,12 @@ content::SecurityStyle ChromeSecurityStateModelClient::GetSecurityStyle(
             IDS_CERTIFICATE_CHAIN_ERROR_DESCRIPTION_FORMAT, error_string),
         security_info.cert_id);
 
-    if (net::IsCertStatusMinorError(security_info.cert_status))
+    if (is_cert_status_minor_error) {
       security_style_explanations->unauthenticated_explanations.push_back(
           explanation);
-    else
+    } else {
       security_style_explanations->broken_explanations.push_back(explanation);
+    }
   } else {
     // If the certificate does not have errors and is not using
     // deprecated SHA1, then add an explanation that the certificate is