Auto select the connection tab of the Website Settings UI in case of an https error or mixed content

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 393 matching lines @@
       site_connection_details_ += ASCIIToUTF16("\n\n");
       site_connection_details_ += l10n_util::GetStringUTF16(
           IDS_PAGE_INFO_SECURITY_TAB_FALLBACK_MESSAGE);
     }
     if (no_renegotiation) {
       site_connection_details_ += ASCIIToUTF16("\n\n");
       site_connection_details_ += l10n_util::GetStringUTF16(
           IDS_PAGE_INFO_SECURITY_TAB_RENEGOTIATION_MESSAGE);
     }
   }
+
+  // By default select the permissions tab that displays all the site
+  // permissions. In case of a connection error or an issue with the
+  // certificate presented by the website select the connection tab to draw to

[wtc, 2012/08/21 21:45:32]

Nit: add a comma (,) between "the website" and "select".

I believe "draw to users attention" should be changed to
"draw the user's attention".

[markusheintz_, 2012/08/21 22:29:44]

Done.

+  // users attention to the issue.  If the site does not provide a certificate
+  // because it was loaded over an unencrypted connection, don't select the
+  // connection tab.
+  WebsiteSettingsUI::TabId tab_id = WebsiteSettingsUI::TAB_ID_PERMISSIONS;
+  if (site_connection_status_ == SITE_CONNECTION_STATUS_ENCRYPTED_ERROR ||
+      site_connection_status_ == SITE_CONNECTION_STATUS_MIXED_CONTENT ||
+      site_identity_status_ == SITE_IDENTITY_STATUS_ERROR ||
+      site_identity_status_ == SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN)

[markusheintz_, 2012/08/21 22:29:44]

@wtc: I'm not sure if we should check for the revocation status here. I remember
a post from Scott Violet (http://www.imperialviolet.org/2012/02/05/crlsets.html)
where he discussed cert revocation checks. So I wonder whether I'm not better
off removing this check.

[wtc, 2012/08/22 19:02:27]

Not checking for SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN
here is fine by me, because the two errors are considered
minor errors (net::IsCertStatusMinorError() is true).

The question is whether the UI for SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN
may make the user curious and want to click the lock icon
to find out more. I forgot what the UI looks like, so I
can't make a good judgment on this.

[markusheintz_, 2012/08/23 00:12:32]

Good point. I'll keep the check for now.

+    tab_id = WebsiteSettingsUI::TAB_ID_CONNECTION;

[wtc, 2012/08/21 21:45:32]

Nit: add curly braces {} because the conditional expression
in this "if" statement is multi-line.

[markusheintz_, 2012/08/21 22:29:44]

I was recently told that multi line conditions don't require {}. I'll see if I
can find the CL. For now I would stick to the no {} solution to be consistent
with my other UI code.

[markusheintz_, 2012/08/21 22:29:44]

I was recently told that multi line conditions don't require { }. Personally my
heart is not tight to either way. Since I omitted the {} in other places (I need
to look up the CL) I would prefer to omit the braces to be consistent.

+  ui_->SetSelectedTab(tab_id);
 }
 
 void WebsiteSettings::PresentSitePermissions() {
   PermissionInfoList permission_info_list;
 
   WebsiteSettingsUI::PermissionInfo permission_info;
   for (size_t i = 0; i < arraysize(kPermissionType); ++i) {
     permission_info.type = kPermissionType[i];
 
     content_settings::SettingInfo info;
@@ skipping 84 matching lines @@
     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);
 }