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

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 isn't possible by simply naming the enum as technically the enum doesn't define all of the possible combinations of bits.) 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.

I also added CERT_STATUS_NO_ERROR in place of "0" as a magic number.

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

[Peter Kasting, 2011-08-31 21:44:07]

jam: content/
wtc: rest

[Peter Kasting, 2011-09-01 00:19:54]

NOTE (mostly to self): Also need to change "0" to "CERT_STATUS_NO_ERROR" in two
places in net/base/cert_database_nss_unittest.cc.  (Currently that file is part
of a different change, so I'll need to land that first.)

[jam, 2011-09-01 01:50:42]

content lgtm

[wtc, 2011-09-02 22:40:35]

Adding the CertStatus typedef is a good idea.

But the potential "deal breaker" for this CL is my first
comment below.

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h#ne...
net/base/cert_status_flags.h:34: static const CertStatus CERT_STATUS_IS_DNSSEC  
               = 1 << 18;
Can these remain enumerators?  Otherwise people may ask why
they don't follow the naming convention of constants.

Do not rename these constants.  They are well-known and
have appeared in bug reports.

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h#ne...
net/base/cert_status_flags.h:37: // be invalid.
Since CertStatus is an unsigned integer, we can use the
most significant bit.

Originiarry cert_status has the 'int' type, so we want to
avoid a negative value for ease of debugging and logging.

[Peter Kasting, 2011-09-12 17:28:40]

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h#ne...
net/base/cert_status_flags.h:34: static const CertStatus CERT_STATUS_IS_DNSSEC  
               = 1 << 18;
On 2011/09/02 22:40:35, wtc wrote:
> Can these remain enumerators?  Otherwise people may ask why
> they don't follow the naming convention of constants.
> 
> Do not rename these constants.  They are well-known and
> have appeared in bug reports.

In that case I think it's fine to simply add:

// These are named LIKE_MACROS instead of kLikeConstants for historical reasons.
// Don't change these names -- they are well-known and have appeared in bug
reports.

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h#ne...
net/base/cert_status_flags.h:37: // be invalid.
On 2011/09/02 22:40:35, wtc wrote:
> Since CertStatus is an unsigned integer, we can use the
> most significant bit.
> 
> Originiarry cert_status has the 'int' type, so we want to
> avoid a negative value for ease of debugging and logging.

OK, I can remove that comment.  There's one unit test
(HttpNetworkTransactionTest.ResetStateForRestart) that is using "-15" as an
"obviously bogus" value for the cert status; I'll just change it to -1, which
should still be fine as all the test really checks is that the status gets reset
to CERT_STATUS_NO_ERROR.

[Peter Kasting, 2011-09-12 17:33:41]

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h#ne...
net/base/cert_status_flags.h:34: static const CertStatus CERT_STATUS_IS_DNSSEC  
               = 1 << 18;
On 2011/09/12 17:28:40, Peter Kasting wrote:
> On 2011/09/02 22:40:35, wtc wrote:
> > Can these remain enumerators?  Otherwise people may ask why
> > they don't follow the naming convention of constants.
> 
> In that case I think it's fine to simply add:
> 
> // These are named LIKE_MACROS instead of kLikeConstants for historical
reasons.
> // Don't change these names -- they are well-known and have appeared in bug
> reports.

I pinged Brett about this and his response was that he didn't care either way.

[Peter Kasting, 2011-09-15 20:28:06]

On 2011/09/12 17:28:40, Peter Kasting wrote:
> http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h
> File net/base/cert_status_flags.h (right):
> 
> In that case I think it's fine to simply add:
> 
> // These are named LIKE_MACROS instead of kLikeConstants for historical
reasons.
> // Don't change these names -- they are well-known and have appeared in bug
> reports.
> 
>
http://codereview.chromium.org/7819009/diff/1/net/base/cert_status_flags.h#ne...
> net/base/cert_status_flags.h:37: // be invalid.
> 
> OK, I can remove that comment.  There's one unit test
> (HttpNetworkTransactionTest.ResetStateForRestart) that is using "-15" as an
> "obviously bogus" value for the cert status; I'll just change it to -1, which
> should still be fine as all the test really checks is that the status gets
reset
> to CERT_STATUS_NO_ERROR.

Made these changes.  Does this look good to you?

[wtc, 2011-09-20 18:21:17]

Sorry about the late reply.  I was out of the office in the past
few days for a surgery.  I will review this CL tomorrow.

[wtc, 2011-09-21 23:54:24]

LGTM on Patch Set 4.  Please remove the CERT_STATUS_NO_ERROR
constant before you check this in.  All the other suggested
changes are optional.

Some of the code cleanup changes seem to be a matter of
personal preference.  I would not make those changes.

http://codereview.chromium.org/7819009/diff/25011/chrome/browser/ssl/ssl_erro...
File chrome/browser/ssl/ssl_error_info.h (right):

http://codereview.chromium.org/7819009/diff/25011/chrome/browser/ssl/ssl_erro...
chrome/browser/ssl/ssl_error_info.h:49: // Callers only interested in the error
count can pass NULL for |errors|.
This comment is stale.  It should be updated to describe
the cert_id and request_url arguments.  You can just add
a TODO comment to note this.

http://codereview.chromium.org/7819009/diff/25011/chrome/browser/ssl/ssl_erro...
chrome/browser/ssl/ssl_error_info.h:53: std::vector<SSLErrorInfo>* errors);
The CL's commit message should call out it is this function
that had the wrong-argument-order bug.

I verified that only this declaration had the arguments in the
wrong order.  Fortunately the function definition and its
callers have the arguments in the right order.

http://codereview.chromium.org/7819009/diff/25011/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/7819009/diff/25011/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:556: //  - CertStatus: the status
of the server's ssl cert (0 means no errors or no
Add net:: to CertStatus.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:17: typedef uint32 CertStatus;
The CertStatus type should be defined and documented separately,
before the CERT_STATUS_xxx flags.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:18: static const CertStatus CERT_STATUS_NO_ERROR   
               = 0;
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.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:37: static const CertStatus CERT_STATUS_IS_DNSSEC  
               = 1 << 18;
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.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_verify_result.cc
File net/base/cert_verify_result.cc (right):

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_verify_result...
net/base/cert_verify_result.cc:20: cert_status = CERT_STATUS_NO_ERROR;
Here the value 0 means not only "no error" but also "no other
status bits, such as IS_EV, are set", so 0 is more appropriate.

http://codereview.chromium.org/7819009/diff/25011/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/7819009/diff/25011/net/http/http_response_info...
net/http/http_response_info.cc:155: if (!pickle.ReadUInt32(&iter, &cert_status))
I verified that this change does not affect how Pickle
serializes cert_status.

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

http://codereview.chromium.org/7819009/diff/25011/chrome/browser/ssl/ssl_erro...
File chrome/browser/ssl/ssl_error_info.h (right):

http://codereview.chromium.org/7819009/diff/25011/chrome/browser/ssl/ssl_erro...
chrome/browser/ssl/ssl_error_info.h:49: // Callers only interested in the error
count can pass NULL for |errors|.
On 2011/09/21 23:54:25, wtc wrote:
> This comment is stale.  It should be updated to describe
> the cert_id and request_url arguments.  You can just add
> a TODO comment to note this.

Added TODO.

http://codereview.chromium.org/7819009/diff/25011/chrome/browser/ssl/ssl_erro...
chrome/browser/ssl/ssl_error_info.h:53: std::vector<SSLErrorInfo>* errors);
On 2011/09/21 23:54:25, wtc wrote:
> The CL's commit message should call out it is this function
> that had the wrong-argument-order bug.

Done.

http://codereview.chromium.org/7819009/diff/25011/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/7819009/diff/25011/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:556: //  - CertStatus: the status
of the server's ssl cert (0 means no errors or no
On 2011/09/21 23:54:25, wtc wrote:
> Add net:: to CertStatus.

Done.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:17: typedef uint32 CertStatus;
On 2011/09/21 23:54:25, wtc wrote:
> The CertStatus type should be defined and documented separately,
> before the CERT_STATUS_xxx flags.

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

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:18: static const CertStatus CERT_STATUS_NO_ERROR   
               = 0;
On 2011/09/21 23:54:25, wtc wrote:
> 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.

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.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:37: static const CertStatus CERT_STATUS_IS_DNSSEC  
               = 1 << 18;
On 2011/09/21 23:54:25, wtc wrote:
> 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.

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.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_verify_result.cc
File net/base/cert_verify_result.cc (right):

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_verify_result...
net/base/cert_verify_result.cc:20: cert_status = CERT_STATUS_NO_ERROR;
On 2011/09/21 23:54:25, wtc wrote:
> Here the value 0 means not only "no error" but also "no other
> status bits, such as IS_EV, are set", so 0 is more appropriate.

See comments in cert_status_flags.h regarding the name.

[wtc, 2011-09-22 17:55:06]

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:17: typedef uint32 CertStatus;
On 2011/09/22 00:36:17, Peter Kasting wrote:
>
> I don't understand what you're requesting that isn't happening.  Can you give
me
> a sample?

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;
...

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:18: static const CertStatus CERT_STATUS_NO_ERROR   
               = 0;
On 2011/09/22 00:36:17, Peter Kasting wrote:
>
> 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?

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]

Will check in.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:17: typedef uint32 CertStatus;
On 2011/09/22 17:55:07, wtc wrote:
> Yes.  We need a comment to document that CertStatus is a
> bitwise-OR of the CERT_STATUS_xxx flags.

Ah, thanks for the example.  Added something.

http://codereview.chromium.org/7819009/diff/25011/net/base/cert_status_flags....
net/base/cert_status_flags.h:18: static const CertStatus CERT_STATUS_NO_ERROR   
               = 0;
On 2011/09/22 17:55:07, wtc wrote:
> 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.

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.