Address alv's comments on codereview.chromium.org/119026....

[Replaced by http://codereview.chromium.org/164521 ]

Address alv's comments on codereview.chromium.org/119026.

BUG=none
TEST=none

[alv, 2009-08-12 18:40:42]

This patch looks ok, but see the inline comment.
r=alexei

http://codereview.chromium.org/164394/diff/1/2
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/164394/diff/1/2#newcode433
Line 433: ScopedCERTCertificatePolicies scoped_policies(policies);
It will work, but it is too fancy and increases size of the code. Since there is
no need to use "goto" I would use a "tag_found" boolean and return it value
after destroying the policies object. You don't even need to have "if" since you
have already made a check at line 429.

[wtc, 2009-08-13 00:35:33]

LGTM.  Thanks!

Alexei, I have a request for you below.

We don't support a default OCSP responder right now, so
we'll use CERT_REV_M_IGNORE_IMPLICIT_DEFAULT_SOURCE as
you suggested.  Do you think Firefox/PSM, which supports
a default OCSP responder, should also use that flag when
doing EV verification?

http://codereview.chromium.org/164394/diff/1/2
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/164394/diff/1/2#newcode103
Line 103: class ScopedCERTCertificatePolicies {
Nit: move this up, under ScopedCERTCertList, because these
two are more related (in NSS).

http://codereview.chromium.org/164394/diff/1/2#newcode356
Line 356: if (policy_oids && num_policy_oids > 0) {
Alexei, I had to add this if statement so that we're
using different flags for normal and EV verifications.
Please verify that these two sets of flags look sane.

http://codereview.chromium.org/164394/diff/1/2#newcode373
Line 373: // TODO(ukai): need to find out if we need to call OCSP-related NSS
functions,
Please remove this TODO comment.  Alexei told me that since
we use CERT_PKIXVerifyCert (rather than the old
CERT_VerifyCert function and its variants), we don't need
to call these functions.

[wtc, 2009-08-13 00:38:22]

http://codereview.chromium.org/164394/diff/1/2
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/164394/diff/1/2#newcode433
Line 433: ScopedCERTCertificatePolicies scoped_policies(policies);
On 2009/08/12 18:40:42, alv wrote:
> It will work, but it is too fancy and increases size of the code.

I, a C programmer, agree with the "too fancy" opinion, but
this is a C++ project, so we need to use the techniques
familiar to C++ programmers.

[wtc, 2009-08-13 01:20:58]

http://codereview.chromium.org/164394/diff/1/2
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/164394/diff/1/2#newcode353
Line 353: CERT_REV_M_IGNORE_MISSING_FRESH_INFO;
Nit: please list CERT_REV_M_IGNORE_MISSING_FRESH_INFO
before CERT_REV_M_STOP_TESTING_ON_FRESH_INFO, to match
the order in which these macros are defined in the NSS
header certt.h.  This makes it easier to verify that we
didn't miss any macros.

[alv, 2009-08-13 18:37:45]

On 2009/08/13 00:35:33, wtc wrote:
> We don't support a default OCSP responder right now, so
> we'll use CERT_REV_M_IGNORE_IMPLICIT_DEFAULT_SOURCE as
> you suggested.  Do you think Firefox/PSM, which supports
> a default OCSP responder, should also use that flag when
> doing EV verification?
Users that are located behind a firewall would use the feature to get ocsp
response. Mozilla products correctly use
CERT_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE flag.

> http://codereview.chromium.org/164394/diff/1/2#newcode356
> Line 356: if (policy_oids && num_policy_oids > 0) {
> Alexei, I had to add this if statement so that we're
> using different flags for normal and EV verifications.
> Please verify that these two sets of flags look sane.
These flags are correct.

[wtc, 2009-08-13 22:08:16]

Since Fumitoshi is on vacation now, I will check this in
for him.

[ukai, 2009-08-18 08:54:43]

On 2009/08/13 22:08:16, wtc wrote:
> Since Fumitoshi is on vacation now, I will check this in
> for him.

I'm back from vacation.
Thanks!