For the SSL cert status, convert anonymous enum that gives bit values into a typedefed uint32. Th...

net/base/cert_status_flags.h

Index: net/base/cert_status_flags.h
===================================================================
--- net/base/cert_status_flags.h	(revision 101367)
+++ net/base/cert_status_flags.h	(working copy)
@@ -6,45 +6,48 @@
 #define NET_BASE_CERT_STATUS_FLAGS_H_
 #pragma once
 
+#include "base/basictypes.h"
+
 namespace net {
 
 // Status flags, such as errors and extended validation.
-enum {
-  // Bits 0 to 15 are for errors.
-  CERT_STATUS_ALL_ERRORS                 = 0xFFFF,
-  CERT_STATUS_COMMON_NAME_INVALID        = 1 << 0,
-  CERT_STATUS_DATE_INVALID               = 1 << 1,
-  CERT_STATUS_AUTHORITY_INVALID          = 1 << 2,
-  // 1 << 3 is reserved for ERR_CERT_CONTAINS_ERRORS (not useful with WinHTTP).
-  CERT_STATUS_NO_REVOCATION_MECHANISM    = 1 << 4,
-  CERT_STATUS_UNABLE_TO_CHECK_REVOCATION = 1 << 5,
-  CERT_STATUS_REVOKED                    = 1 << 6,
-  CERT_STATUS_INVALID                    = 1 << 7,
-  CERT_STATUS_WEAK_SIGNATURE_ALGORITHM   = 1 << 8,
-  CERT_STATUS_NOT_IN_DNS                 = 1 << 9,
-  CERT_STATUS_NON_UNIQUE_NAME            = 1 << 10,
+// NOTE: Because these names have appeared in bug reports, we preserve them as
+// MACRO_STYLE for continuity, instead of renaming them to kConstantStyle as
+// befits most static consts.
+typedef uint32 CertStatus;

[wtc, 2011/09/21 23:54:25]

The CertStatus type should be defined and documented separately,
before the CERT_STATUS_xxx flags.

[Peter Kasting, 2011/09/22 00:36:17]

I don't understand what you're requesting that isn't happening.  Can you give me
a sample?

[wtc, 2011/09/22 17:55:07]

Yes.  We need a comment to document that CertStatus is a
bitwise-OR of the CERT_STATUS_xxx flags.

Here is an example (I reused the comment for the |cert_status|
field in net/base/ssl_info.h):

// Bitmask of status flags of a certificate, representing, for example,
// known errors and extended validation (EV) status.
typedef uint32 CertStatus;

// Status flags, such as errors and extended validation.
// NOTE: Because these names have appeared in bug reports, ...
...
// Bits 0 to 15 are for errors.
static const CertStatus CERT_STATUS_ALL_ERRORS = 0xFFFF;
static const CertStatus CERT_STATUS_COMMON_NAME_INVALID = 1 << 0;
static const CertStatus CERT_STATUS_DATE_INVALID = 1 << 1;
...

[Peter Kasting, 2011/09/22 18:25:50]

Ah, thanks for the example.  Added something.

+static const CertStatus CERT_STATUS_NO_ERROR                   = 0;

[wtc, 2011/09/21 23:54:25]

Please remove the NO_ERROR constant.  It is obvious that 0
means no error bits are set, which means no error.  If someone
needs the NO_ERROR constant, he will also need the SET_ERROR,
CLEAR_ERROR, and ERROR_IS_SET macros/functions.

[Peter Kasting, 2011/09/22 00:36:17]

I looked up ERROR_IS_SET in code search and didn't find anything.  Can you tell
me where these macros are and what they do?

I want to find a way to include CERT_STATUS_NO_ERROR.  We're pretty careful in
most other places to give an explicit name to 0 if it's actually a legal value
in a case like this, rather than letting all values be named except for 0 as a
magic number.  This makes it clear 0 is explicitly legal and makes callers who
use it more readable ("cert_status = 0" isn't terribly meaningful on its own).

I'm not wedded to the name, though.  If using NO_ERROR is too specific to just
referring to the bottom 16 bits of the value, we could use CERT_STATUS_NORMAL or
something else, if you like that better.

[wtc, 2011/09/22 17:55:07]

Sorry I wasn't clear.  What I meant is that if a programmer
needs a NO_ERROR constant to understand what a bitmask of 0
means, that shows he is unfamiliar with bit manipulation,
and therefore he is likely to also need functions/macros
for setting a bit flag, clearing a bit flag, or testing if
a bit flag is set.

But our code performs those bit operations directly, for example,
  cert_status |= CERT_STATUS_IS_EV;

  if (cert_status & CERT_STATUS_IS_EV) {
    ...
  }

So we don't need a constant to help Chromium developers
understand a bitmask of 0 means no bit flag is set, or
setting a bitmask to 0 means clearing all bits.

[Peter Kasting, 2011/09/22 18:25:50]

Ah.  I think where I'm coming from is that it's not always obvious at all usage
points that CertStatus is actually a bitfield, what the bit values are, and what
"0" means, without referring back to the definition here.  I was trying to help
that.  But you own this code, so if you really don't think it's a good idea,
fair enough.  I've removed it.

 
-  // Bits 16 to 30 are for non-error statuses.
-  CERT_STATUS_IS_EV                      = 1 << 16,
-  CERT_STATUS_REV_CHECKING_ENABLED       = 1 << 17,
-  CERT_STATUS_IS_DNSSEC                  = 1 << 18,
+// Bits 0 to 15 are for errors.
+static const CertStatus CERT_STATUS_ALL_ERRORS                 = 0xFFFF;
+static const CertStatus CERT_STATUS_COMMON_NAME_INVALID        = 1 << 0;
+static const CertStatus CERT_STATUS_DATE_INVALID               = 1 << 1;
+static const CertStatus CERT_STATUS_AUTHORITY_INVALID          = 1 << 2;
+// 1 << 3 is reserved for ERR_CERT_CONTAINS_ERRORS (not useful with WinHTTP).
+static const CertStatus CERT_STATUS_NO_REVOCATION_MECHANISM    = 1 << 4;
+static const CertStatus CERT_STATUS_UNABLE_TO_CHECK_REVOCATION = 1 << 5;
+static const CertStatus CERT_STATUS_REVOKED                    = 1 << 6;
+static const CertStatus CERT_STATUS_INVALID                    = 1 << 7;
+static const CertStatus CERT_STATUS_WEAK_SIGNATURE_ALGORITHM   = 1 << 8;
+static const CertStatus CERT_STATUS_NOT_IN_DNS                 = 1 << 9;
+static const CertStatus CERT_STATUS_NON_UNIQUE_NAME            = 1 << 10;
 
-  // 1 << 31 (the sign bit) is reserved so that the cert status will never be
-  // negative.
-};
+// Bits 16 to 31 are for non-error statuses.
+static const CertStatus CERT_STATUS_IS_EV                      = 1 << 16;
+static const CertStatus CERT_STATUS_REV_CHECKING_ENABLED       = 1 << 17;
+static const CertStatus CERT_STATUS_IS_DNSSEC                  = 1 << 18;

[wtc, 2011/09/21 23:54:25]

Changing how these flags are defined has the downside that
it'll take more work to track down in SVN history when a particular
flag was added.  I would not make this change.

[Peter Kasting, 2011/09/22 00:36:17]

This is true of any cleanup to anything in the codebase.  Looking past one
obvious cleanup commit is not a huge burden on coders or we'd never do any
cleanup.

 
 // Returns true if the specified cert status has an error set.
-static inline bool IsCertStatusError(int status) {
+static inline bool IsCertStatusError(CertStatus status) {
   return (CERT_STATUS_ALL_ERRORS & status) != 0;
 }
 
 // Maps a network error code to the equivalent certificate status flag.  If
 // the error code is not a certificate error, it is mapped to 0.
-int MapNetErrorToCertStatus(int error);
+CertStatus MapNetErrorToCertStatus(int error);
 
 // Maps the most serious certificate error in the certificate status flags
 // to the equivalent network error code.
-int MapCertStatusToNetError(int cert_status);
+int MapCertStatusToNetError(CertStatus cert_status);
 
 }  // namespace net