For the SSL cert status, convert anonymous enum that gives bit values…

… into a typedefed uint32. This allows code all over Chromium to use an explicit type instead of "int". This also means the individual named bit constants themselves have the same explicit type. I find the resulting code to be noticeably clearer. This also exposed a bug in SSLErrorInfo::GetErrorsForCertStatus() where not having an explicit type allowed a function argument ordering bug to creep in, so I claim this is safer too. Normally this makes things like DCHECK_EQ() unhappy, but when I'd originally tested this I didn't seem to need to make any changes due to that. Will be watching the trybots... The original motiviation for this change was to find a way to eliminate some cases of passing anonymous-typed values as template arguments (which happens when you use a value from the enum in e.g. EXPECT_EQ()), which is technically illegal in C++03, though we don't warn about it. Simply naming the enum would have done this, but this would have encouraged readers to actually use the enum name as a type, which for a bitfield is inappropriate for the reason given in the first paragraph. BUG=92247 TEST=Compiles Review URL: http://codereview.chromium.org/7969023 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@102415 0039d316-1c4b-4281-b951-d872f2087c98
marcosholgado · Sep 23, 2011 · 70d6650 · 70d6650
1 parent 0a075d6
commit 70d6650
Show file tree

Hide file tree

Showing 42 changed files with 156 additions and 125 deletions.
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc
@@ -1480,11 +1480,12 @@ void TestingAutomationProvider::WaitForTabToBeRestored(
   }
 }
 
-void TestingAutomationProvider::GetSecurityState(int handle,
-                                                 bool* success,
-                                                 SecurityStyle* security_style,
-                                                 int* ssl_cert_status,
-                                                 int* insecure_content_status) {
+void TestingAutomationProvider::GetSecurityState(
+    int handle,
+    bool* success,
+    SecurityStyle* security_style,
+    net::CertStatus* ssl_cert_status,
+    int* insecure_content_status) {
   if (tab_tracker_->ContainsHandle(handle)) {
     NavigationController* tab = tab_tracker_->GetResource(handle);
     NavigationEntry* entry = tab->GetActiveEntry();

diff --git a/chrome/browser/automation/testing_automation_provider.h b/chrome/browser/automation/testing_automation_provider.h
@@ -19,6 +19,7 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "content/common/notification_registrar.h"
 #include "content/common/page_type.h"
+#include "net/base/cert_status_flags.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebInputEvent.h"
 
 class ImporterList;
@@ -196,8 +197,10 @@ class TestingAutomationProvider : public AutomationProvider,
   void WaitForTabToBeRestored(int tab_handle, IPC::Message* reply_message);
 
   // Gets the security state for the tab associated to the specified |handle|.
-  void GetSecurityState(int handle, bool* success,
-                        SecurityStyle* security_style, int* ssl_cert_status,
+  void GetSecurityState(int handle,
+                        bool* success,
+                        SecurityStyle* security_style,
+                        net::CertStatus* ssl_cert_status,
                         int* insecure_content_status);
 
   // Gets the page type for the tab associated to the specified |handle|.

diff --git a/chrome/browser/page_info_model.cc b/chrome/browser/page_info_model.cc
@@ -68,7 +68,7 @@ PageInfoModel::PageInfoModel(Profile* profile,
       CertStore::GetInstance()->RetrieveCert(ssl.cert_id(), &cert) &&
       !net::IsCertStatusError(status_with_warnings_removed)) {
     // No error found so far, check cert_status warnings.
-    int cert_status = ssl.cert_status();
+    net::CertStatus cert_status = ssl.cert_status();
     if (cert_status & cert_warnings) {
       string16 issuer_name(UTF8ToUTF16(cert->issuer().GetDisplayName()));
       if (issuer_name.empty()) {
@@ -89,7 +89,7 @@ PageInfoModel::PageInfoModel(Profile* profile,
         NOTREACHED() << "Need to specify string for this warning";
       }
       icon_id = ICON_STATE_WARNING_MINOR;
-    } else if ((ssl.cert_status() & net::CERT_STATUS_IS_EV) != 0) {
+    } else if (ssl.cert_status() & net::CERT_STATUS_IS_EV) {
       // EV HTTPS page.
       DCHECK(!cert->subject().organization_names.empty());
       headline =
@@ -119,7 +119,7 @@ PageInfoModel::PageInfoModel(Profile* profile,
           UTF8ToUTF16(cert->subject().organization_names[0]),
           locality,
           UTF8ToUTF16(cert->issuer().GetDisplayName())));
-    } else if ((ssl.cert_status() & net::CERT_STATUS_IS_DNSSEC) != 0) {
+    } else if (ssl.cert_status() & net::CERT_STATUS_IS_DNSSEC) {
       // DNSSEC authenticated page.
       if (empty_subject_name)
         headline.clear();  // Don't display any title.

diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -49,7 +49,7 @@ class SSLUITest : public InProcessBrowserTest {
     ASSERT_TRUE(entry);
     EXPECT_EQ(NORMAL_PAGE, entry->page_type());
     EXPECT_EQ(SECURITY_STYLE_AUTHENTICATED, entry->ssl().security_style());
-    EXPECT_EQ(0, entry->ssl().cert_status() & net::CERT_STATUS_ALL_ERRORS);
+    EXPECT_EQ(0U, entry->ssl().cert_status() & net::CERT_STATUS_ALL_ERRORS);
     EXPECT_EQ(displayed_insecure_content,
               entry->ssl().displayed_insecure_content());
     EXPECT_FALSE(entry->ssl().ran_insecure_content());
@@ -60,13 +60,13 @@ class SSLUITest : public InProcessBrowserTest {
     ASSERT_TRUE(entry);
     EXPECT_EQ(NORMAL_PAGE, entry->page_type());
     EXPECT_EQ(SECURITY_STYLE_UNAUTHENTICATED, entry->ssl().security_style());
-    EXPECT_EQ(0, entry->ssl().cert_status() & net::CERT_STATUS_ALL_ERRORS);
+    EXPECT_EQ(0U, entry->ssl().cert_status() & net::CERT_STATUS_ALL_ERRORS);
     EXPECT_FALSE(entry->ssl().displayed_insecure_content());
     EXPECT_FALSE(entry->ssl().ran_insecure_content());
   }
 
   void CheckAuthenticationBrokenState(TabContents* tab,
-                                      int error,
+                                      net::CertStatus error,
                                       bool ran_insecure_content,
                                       bool interstitial) {
     NavigationEntry* entry = tab->controller().GetActiveEntry();

diff --git a/chrome/browser/ssl/ssl_error_info.cc b/chrome/browser/ssl/ssl_error_info.cc
@@ -231,10 +231,10 @@ SSLErrorInfo::ErrorType SSLErrorInfo::NetErrorToErrorType(int net_error) {
 
 // static
 int SSLErrorInfo::GetErrorsForCertStatus(int cert_id,
-                                         int cert_status,
+                                         net::CertStatus cert_status,
                                          const GURL& url,
                                          std::vector<SSLErrorInfo>* errors) {
-  const int kErrorFlags[] = {
+  const net::CertStatus kErrorFlags[] = {
     net::CERT_STATUS_COMMON_NAME_INVALID,
     net::CERT_STATUS_DATE_INVALID,
     net::CERT_STATUS_AUTHORITY_INVALID,

diff --git a/chrome/browser/ssl/ssl_error_info.h b/chrome/browser/ssl/ssl_error_info.h
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/string16.h"
+#include "net/base/cert_status_flags.h"
 #include "net/base/x509_certificate.h"
 
 class GURL;
@@ -46,9 +47,10 @@ class SSLErrorInfo {
   // Populates the specified |errors| vector with the errors contained in
   // |cert_status|.  Returns the number of errors found.
   // Callers only interested in the error count can pass NULL for |errors|.
-  static int GetErrorsForCertStatus(int cert_status,
-                                    int cert_id,
-                                    const GURL& request_url,
+  // TODO(wtc): Document |cert_id| and |url| arguments.
+  static int GetErrorsForCertStatus(int cert_id,
+                                    net::CertStatus cert_status,
+                                    const GURL& url,
                                     std::vector<SSLErrorInfo>* errors);
 
   // A title describing the error, usually to be used with the details below.

diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -1664,7 +1664,10 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) {
       // Deserialize the SSL info.
       NavigationEntry::SSLStatus ssl;
       if (!params_.security_info.empty()) {
-        int cert_id, cert_status, security_bits, connection_status;
+        int cert_id;
+        net::CertStatus cert_status;
+        int security_bits;
+        int connection_status;
         SSLManager::DeserializeSecurityInfo(params_.security_info,
                                             &cert_id,
                                             &cert_status,

diff --git a/chrome/browser/ui/toolbar/toolbar_model.cc b/chrome/browser/ui/toolbar/toolbar_model.cc
@@ -86,8 +86,8 @@ ToolbarModel::SecurityLevel ToolbarModel::GetSecurityLevel() const {
       if (ssl.displayed_insecure_content())
         return SECURITY_WARNING;
       if (net::IsCertStatusError(ssl.cert_status())) {
-        DCHECK_EQ(ssl.cert_status() & net::CERT_STATUS_ALL_ERRORS,
-                  net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION);
+        DCHECK_EQ(net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION,
+                  ssl.cert_status() & net::CERT_STATUS_ALL_ERRORS);
         return SECURITY_WARNING;
       }
       if ((ssl.cert_status() & net::CERT_STATUS_IS_EV) &&

diff --git a/chrome/common/automation_messages_internal.h b/chrome/common/automation_messages_internal.h
@@ -12,6 +12,7 @@
 #include "content/common/navigation_types.h"
 #include "googleurl/src/gurl.h"
 #include "ipc/ipc_message_macros.h"
+#include "net/base/cert_status_flags.h"
 #include "net/url_request/url_request_status.h"
 #include "ui/gfx/rect.h"
 #include "webkit/glue/window_open_disposition.h"
@@ -552,15 +553,15 @@ IPC_MESSAGE_ROUTED1(AutomationMsg_DidNavigate,
 // Response:
 //  - bool: whether the operation was successful.
 //  - SecurityStyle: the security style of the tab.
-//  - int: the status of the server's ssl cert (0 means no errors or no ssl
-//         was used).
+//  - net::CertStatus: the status of the server's ssl cert (0 means no errors or
+//                     no ssl was used).
 //  - int: the insecure content state, 0 means no insecure contents.
 
 IPC_SYNC_MESSAGE_CONTROL1_4(AutomationMsg_GetSecurityState,
                             int,
                             bool,
                             SecurityStyle,
-                            int,
+                            net::CertStatus,
                             int)
 
 // This message requests the page type of the page displayed in the specified

diff --git a/chrome/test/automation/tab_proxy.cc b/chrome/test/automation/tab_proxy.cc
@@ -571,7 +571,7 @@ bool TabProxy::WaitForTabToBeRestored(uint32 timeout_ms) {
 }
 
 bool TabProxy::GetSecurityState(SecurityStyle* security_style,
-                                int* ssl_cert_status,
+                                net::CertStatus* ssl_cert_status,
                                 int* insecure_content_status) {
   DCHECK(security_style && ssl_cert_status && insecure_content_status);
 

diff --git a/chrome/test/automation/tab_proxy.h b/chrome/test/automation/tab_proxy.h
@@ -282,7 +282,7 @@ class TabProxy : public AutomationResourceProxy,
 
   // Retrieves the different security states for the current tab.
   bool GetSecurityState(SecurityStyle* security_style,
-                        int* ssl_cert_status,
+                        net::CertStatus* ssl_cert_status,
                         int* insecure_content_status) WARN_UNUSED_RESULT;
 
   // Returns the type of the page currently showing (normal, interstitial,

diff --git a/content/browser/load_from_memory_cache_details.cc b/content/browser/load_from_memory_cache_details.cc
@@ -8,7 +8,7 @@ LoadFromMemoryCacheDetails::LoadFromMemoryCacheDetails(
     const GURL& url,
     int pid,
     int cert_id,
-    int cert_status)
+    net::CertStatus cert_status)
     : url_(url),
       pid_(pid),
       cert_id_(cert_id),

diff --git a/content/browser/load_from_memory_cache_details.h b/content/browser/load_from_memory_cache_details.h
@@ -8,26 +8,27 @@
 
 #include "base/basictypes.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/cert_status_flags.h"
 
 class LoadFromMemoryCacheDetails {
  public:
    LoadFromMemoryCacheDetails(
        const GURL& url,
        int pid,
        int cert_id,
-       int cert_status);
+       net::CertStatus cert_status);
   ~LoadFromMemoryCacheDetails();
 
   const GURL& url() const { return url_; }
   int pid() const { return pid_; }
   int ssl_cert_id() const { return cert_id_; }
-  int ssl_cert_status() const { return cert_status_; }
+  net::CertStatus ssl_cert_status() const { return cert_status_; }
 
  private:
   GURL url_;
   int pid_;
   int cert_id_;
-  int cert_status_;
+  net::CertStatus cert_status_;
 
   DISALLOW_COPY_AND_ASSIGN(LoadFromMemoryCacheDetails);
 };

diff --git a/content/browser/renderer_host/resource_request_details.h b/content/browser/renderer_host/resource_request_details.h
@@ -13,6 +13,7 @@
 #include <string>
 
 #include "googleurl/src/gurl.h"
+#include "net/base/cert_status_flags.h"
 #include "net/base/host_port_pair.h"
 #include "net/url_request/url_request_status.h"
 #include "webkit/glue/resource_type.h"
@@ -37,7 +38,7 @@ class ResourceRequestDetails {
   int origin_child_id() const { return origin_child_id_; }
   const net::URLRequestStatus& status() const { return status_; }
   int ssl_cert_id() const { return ssl_cert_id_; }
-  int ssl_cert_status() const { return ssl_cert_status_; }
+  net::CertStatus ssl_cert_status() const { return ssl_cert_status_; }
   ResourceType::Type resource_type() const { return resource_type_; }
   net::HostPortPair socket_address() const { return socket_address_; }
 
@@ -51,7 +52,7 @@ class ResourceRequestDetails {
   int origin_child_id_;
   net::URLRequestStatus status_;
   int ssl_cert_id_;
-  int ssl_cert_status_;
+  net::CertStatus ssl_cert_status_;
   ResourceType::Type resource_type_;
   net::HostPortPair socket_address_;
 };

diff --git a/content/browser/ssl/ssl_manager.cc b/content/browser/ssl/ssl_manager.cc
@@ -55,12 +55,12 @@ void SSLManager::NotifySSLInternalStateChanged(
 
 // static
 std::string SSLManager::SerializeSecurityInfo(int cert_id,
-                                              int cert_status,
+                                              net::CertStatus cert_status,
                                               int security_bits,
                                               int ssl_connection_status) {
   Pickle pickle;
   pickle.WriteInt(cert_id);
-  pickle.WriteInt(cert_status);
+  pickle.WriteUInt32(cert_status);
   pickle.WriteInt(security_bits);
   pickle.WriteInt(ssl_connection_status);
   return std::string(static_cast<const char*>(pickle.data()), pickle.size());
@@ -69,7 +69,7 @@ std::string SSLManager::SerializeSecurityInfo(int cert_id,
 // static
 bool SSLManager::DeserializeSecurityInfo(const std::string& state,
                                          int* cert_id,
-                                         int* cert_status,
+                                         net::CertStatus* cert_status,
                                          int* security_bits,
                                          int* ssl_connection_status) {
   DCHECK(cert_id && cert_status && security_bits && ssl_connection_status);
@@ -86,7 +86,7 @@ bool SSLManager::DeserializeSecurityInfo(const std::string& state,
   Pickle pickle(state.data(), static_cast<int>(state.size()));
   void * iter = NULL;
   return pickle.ReadInt(&iter, cert_id) &&
-         pickle.ReadInt(&iter, cert_status) &&
+         pickle.ReadUInt32(&iter, cert_status) &&
          pickle.ReadInt(&iter, security_bits) &&
          pickle.ReadInt(&iter, ssl_connection_status);
 }
@@ -124,8 +124,10 @@ void SSLManager::DidCommitProvisionalLoad(
   if (details->is_main_frame) {
     if (entry) {
       // Decode the security details.
-      int ssl_cert_id, ssl_cert_status, ssl_security_bits,
-          ssl_connection_status;
+      int ssl_cert_id;
+      net::CertStatus ssl_cert_status;
+      int ssl_security_bits;
+      int ssl_connection_status;
       DeserializeSecurityInfo(details->serialized_security_info,
                               &ssl_cert_id,
                               &ssl_cert_status,

diff --git a/content/browser/ssl/ssl_manager.h b/content/browser/ssl/ssl_manager.h
@@ -15,6 +15,7 @@
 #include "content/common/notification_observer.h"
 #include "content/common/notification_registrar.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/cert_status_flags.h"
 #include "net/base/net_errors.h"
 
 class LoadFromMemoryCacheDetails;
@@ -57,14 +58,15 @@ class SSLManager : public NotificationObserver {
 
   // Convenience methods for serializing/deserializing the security info.
   static std::string SerializeSecurityInfo(int cert_id,
-                                           int cert_status,
+                                           net::CertStatus cert_status,
                                            int security_bits,
                                            int connection_status);
-  CONTENT_EXPORT static bool DeserializeSecurityInfo(const std::string& state,
-                                                     int* cert_id,
-                                                     int* cert_status,
-                                                     int* security_bits,
-                                                     int* connection_status);
+  CONTENT_EXPORT static bool DeserializeSecurityInfo(
+      const std::string& state,
+      int* cert_id,
+      net::CertStatus* cert_status,
+      int* security_bits,
+      int* connection_status);
 
   // Construct an SSLManager for the specified tab.
   // If |delegate| is NULL, SSLPolicy::GetDefaultPolicy() is used.

diff --git a/content/browser/ssl/ssl_policy.cc b/content/browser/ssl/ssl_policy.cc
@@ -131,7 +131,8 @@ void SSLPolicy::UpdateEntry(NavigationEntry* entry, TabContents* tab_contents) {
 
   // If CERT_STATUS_UNABLE_TO_CHECK_REVOCATION is the only certificate error,
   // don't lower the security style to SECURITY_STYLE_AUTHENTICATION_BROKEN.
-  int cert_errors = entry->ssl().cert_status() & net::CERT_STATUS_ALL_ERRORS;
+  net::CertStatus cert_errors =
+      entry->ssl().cert_status() & net::CERT_STATUS_ALL_ERRORS;
   if (cert_errors) {
     if (cert_errors != net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION)
       entry->ssl().set_security_style(SECURITY_STYLE_AUTHENTICATION_BROKEN);

diff --git a/content/browser/ssl/ssl_request_info.cc b/content/browser/ssl/ssl_request_info.cc
@@ -8,7 +8,7 @@ SSLRequestInfo::SSLRequestInfo(const GURL& url,
                                ResourceType::Type resource_type,
                                int child_id,
                                int ssl_cert_id,
-                               int ssl_cert_status)
+                               net::CertStatus ssl_cert_status)
     : url_(url),
       resource_type_(resource_type),
       child_id_(child_id),

diff --git a/content/browser/ssl/ssl_request_info.h b/content/browser/ssl/ssl_request_info.h
@@ -10,6 +10,7 @@
 
 #include "base/memory/ref_counted.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/cert_status_flags.h"
 #include "webkit/glue/resource_type.h"
 
 // SSLRequestInfo wraps up the information SSLPolicy needs about a request in
@@ -21,13 +22,13 @@ class SSLRequestInfo : public base::RefCounted<SSLRequestInfo> {
                  ResourceType::Type resource_type,
                  int child_id,
                  int ssl_cert_id,
-                 int ssl_cert_status);
+                 net::CertStatus ssl_cert_status);
 
   const GURL& url() const { return url_; }
   ResourceType::Type resource_type() const { return resource_type_; }
   int child_id() const { return child_id_; }
   int ssl_cert_id() const { return ssl_cert_id_; }
-  int ssl_cert_status() const { return ssl_cert_status_; }
+  net::CertStatus ssl_cert_status() const { return ssl_cert_status_; }
 
  private:
   friend class base::RefCounted<SSLRequestInfo>;
@@ -38,7 +39,7 @@ class SSLRequestInfo : public base::RefCounted<SSLRequestInfo> {
   ResourceType::Type resource_type_;
   int child_id_;
   int ssl_cert_id_;
-  int ssl_cert_status_;
+  net::CertStatus ssl_cert_status_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLRequestInfo);
 };