Address post-review feedback for r81702

Address post-review feedback for r81702.

On Mac, if certificate revocation checking is disabled in the preferences, absolutely no revocation checking will occur, which now also includes bypassing/ignoring the local CRL and OCSP caches.

R=wtc
BUG=78523
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82617

[Ryan Sleevi, 2011-04-20 22:46:58]

PTAL

[wtc, 2011-04-20 23:28:11]

LGTM.

http://codereview.chromium.org/6879095/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6879095/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:292: // validate a certificate for an SSL peer.
|hostname| contains the name of
Also change the "peer" on this line.

http://codereview.chromium.org/6879095/diff/1/third_party/apple_apsl/README.c...
File third_party/apple_apsl/README.chromium (right):

http://codereview.chromium.org/6879095/diff/1/third_party/apple_apsl/README.c...
third_party/apple_apsl/README.chromium:42: guarantee for forwards compatibility.
However, the use of this header is the
Nit: forwards compatibility => backwards compatibility ?

Or just say "API stability"?

[Ryan Sleevi, 2011-04-21 02:31:12]

wtc: PTAL - x509_certificate_mac.cc has been updated following your comments on
revocation checking behaviour. With the change, the cache is completely bypassed
if disabled and no errors are presented to users.

[wtc, 2011-04-21 22:04:38]

LGTM.

Would be nice to note the disable-revocation-checking
change in the commit message.

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:303: ScopedCFTypeRef<CFMutableArrayRef>
scoped_local_policies(local_policies);
Please rewrite this as:
  ScopedCFTypeRef<CFMutableArrayRef> local_policies(
      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
  if (!local_policies)
    return memFullErr;

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:324: // not desirable here.
It would be nice to stress that we need to specify a revocation
policy (an OCSP policy that suppresses access to both
network and cache) to disable revocation checking.

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:338: // report an error error that OCSP was
unavailable, but this will be
Typo: error error => error

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:354: return noErr;
Nit: you can put lines 357-370 inside a
  if (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED) {
  }
block, then we only need one instance of
  policies->reset(scoped_local_policies.release());
  return noErr;
at the end of this function.

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:372:
policies->reset(scoped_local_policies.release());
We can use 'swap' here.  Would that be better?

[Ryan Sleevi, 2011-04-21 23:59:17]

I'll land this CL this evening.

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:324: // not desirable here.
On 2011/04/21 22:04:38, wtc wrote:
> It would be nice to stress that we need to specify a revocation
> policy (an OCSP policy that suppresses access to both
> network and cache) to disable revocation checking.

Wording has been updated.

http://codereview.chromium.org/6879095/diff/7/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:372:
policies->reset(scoped_local_policies.release());
On 2011/04/21 22:04:38, wtc wrote:
> We can use 'swap' here.  Would that be better?

We can't. ScopedCFType<CFArrayRef> is a different type than
ScopedCFType<CFMutableArrayRef>, even though the two CF types are exchangeable.