Chromium Code Reviews
Help | Chromium Project | Sign in
(151)

Issue 2812064: Revert 52799 - Add support for parsing certificate formats other than raw, DE... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 7 months ago by Ryan Sleevi
Modified:
3 years, 10 months ago
CC:
chromium-reviews, pam+watch_chromium.org, John Grabowski, cbentzel+watch_chromium.org, darin-cc_chromium.org, PaweĊ‚ Hajdan Jr.
Visibility:
Public.

Description

Revert 52799 - Add support for parsing certificate formats other than raw, DER-encoded certificates - specifically formats that represent collections of certificates. The certificate format can now be specified as an explicit format, or as a bit-mask of formats that are acceptable/expected, with the first parsable format winning. This is one half of a commit to address BUG #37142, with the second half involving connecting this through the X509UserCertHandler and the actual UI. R=wtc BUG=37142 TEST=X509CertificateParseTest* and PEMTokenizerTest.* Review URL: http://codereview.chromium.org/2819018 TBR=rsleevi@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52801

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -1033 lines) Patch
D net/base/pem_tokenizer.h View 1 chunk +0 lines, -79 lines 0 comments Download
D net/base/pem_tokenizer.cc View 1 chunk +0 lines, -95 lines 0 comments Download
D net/base/pem_tokenizer_unittest.cc View 1 chunk +0 lines, -169 lines 0 comments Download
M net/base/x509_certificate.h View 5 chunks +1 line, -37 lines 0 comments Download
M net/base/x509_certificate.cc View 3 chunks +0 lines, -89 lines 0 comments Download
M net/base/x509_certificate_mac.cc View 3 chunks +2 lines, -78 lines 0 comments Download
M net/base/x509_certificate_nss.cc View 3 chunks +8 lines, -63 lines 0 comments Download
M net/base/x509_certificate_unittest.cc View 5 chunks +8 lines, -154 lines 1 comment Download
M net/base/x509_certificate_win.cc View 2 chunks +0 lines, -69 lines 0 comments Download
D net/data/ssl/certificates/google.binary.p7b View 1 chunk +0 lines, -31 lines 0 comments Download
D net/data/ssl/certificates/google.chain.pem View 1 chunk +0 lines, -38 lines 0 comments Download
D net/data/ssl/certificates/google.pem_cert.p7b View 1 chunk +0 lines, -37 lines 0 comments Download
D net/data/ssl/certificates/google.pem_pkcs7.p7b View 1 chunk +0 lines, -37 lines 0 comments Download
D net/data/ssl/certificates/google.single.der View 1 chunk +0 lines, -16 lines 0 comments Download
D net/data/ssl/certificates/google.single.pem View 1 chunk +0 lines, -19 lines 0 comments Download
D net/data/ssl/certificates/thawte.single.pem View 1 chunk +0 lines, -19 lines 0 comments Download
M net/net.gyp View 2 chunks +0 lines, -3 lines 0 comments Download
Trybot results:
Commit:

Messages

Total messages: 3 (0 generated)
Ryan Sleevi
4 years, 7 months ago (2010-07-17 03:17:07 UTC) #1
wtc
When you revert a CL, please add a comment to note why you had to ...
4 years, 7 months ago (2010-07-17 13:28:58 UTC) #2
rsleevi-old
4 years, 7 months ago (2010-07-17 18:22:58 UTC) #3
On 2010/07/17 13:28:58, wtc wrote:
> When you revert a CL, please add a comment to note why
> you had to revert it.
> 
> The CL you reverted broke the Mac builds.  I found some
> compiler warnings in x509_certificate_unittest.cc that
> were treated as errors.  I don't know what those warnings
> means.  I hope you can figure out how to fix them.
> 
> http://codereview.chromium.org/2812064/diff/1/9
> File net/base/x509_certificate_unittest.cc (left):
> 
> http://codereview.chromium.org/2812064/diff/1/9#oldcode669
> net/base/x509_certificate_unittest.cc:669: for (size_t j = 0; j < 20; ++j)
> The constant 20 should be replaced by
> sizeof(actual_fingerprint.data).

Yeah, this was my first time using drover, and it didn't like my cygwin
environment that I was running from (Specifically, it wasn't able to find fi,
which would have allowed me to edit the message). I was given the option of futz
with it, or just revert.

I've fixed the issue in the original CL. for whatever reason, the OS X 10.5 gcc
4.2.1, unlike all the other boxes/compilers, didn't like some of the data being
in an anonymous namespace. The simple fix was to remove the anonymous namespace
qualifier in the x509_certificate_unittests. This doesn't have any functional
effect, even to linking, since all of the implementation is contained within
that file and only used by the unit tests.

This non-use of anon was consistant with the other places in code using
instantiated tests, so it seems correct. I was just waiting to reland until I
had a chance to babysit the tree again.

Trybots compile it fine (though they fail the tests, because the trybots don't
like binary files), so it's all good.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld dd99357-tainted