Refactor certificate stats collection, added SSLCertificateStats.

Refactor certificate stats collection, added SSLCertificateStats.

The code that extracts certificate stats from an SSLCertificate and its
certificate chain is moved into SSLCertificate::GetStats. The stats
collector code loops through the resulting SSLCertificateStats and
creates the StatsReports for those stats.

This will allow the new stats collector to reuse GetStats in a future
CL.

BUG=chromium:627816, chromium:629436
Committed: https://crrev.com/e29352bb34de60bd0a56d4ce46c2ce35ac2b27b4
Cr-Commit-Position: refs/heads/master@{#13917}

[hbos, 2016-08-19 16:53:52]

Patchset #1 (id:1) has been deleted

[hbos, 2016-08-19 17:10:20]

Patchset #1 (id:20001) has been deleted

[hbos, 2016-08-19 17:20:35]

hbos@webrtc.org changed reviewers:
+ perkj@webrtc.org

[hbos, 2016-08-19 17:20:35]

Please take a look, perkj.

[perkj_webrtc, 2016-08-20 09:49:53]

lgtm with a nit. Should you have another reviewer that knows more about this
like hta?

https://codereview.webrtc.org/2259283002/diff/40001/webrtc/base/sslidentity.cc
File webrtc/base/sslidentity.cc (right):

https://codereview.webrtc.org/2259283002/diff/40001/webrtc/base/sslidentity.c...
webrtc/base/sslidentity.cc:70: // TODO(bemasc): Move this computation to a
helper class that caches these
change this todo? bemasc no longer works for webrtc. Is the todo still correct?

[hbos, 2016-08-22 10:36:57]

hbos@webrtc.org changed reviewers:
+ hta@webrtc.org

[hbos, 2016-08-22 10:36:58]

Please take a look, hta.

https://codereview.webrtc.org/2259283002/diff/40001/webrtc/base/sslidentity.cc
File webrtc/base/sslidentity.cc (right):

https://codereview.webrtc.org/2259283002/diff/40001/webrtc/base/sslidentity.c...
webrtc/base/sslidentity.cc:70: // TODO(bemasc): Move this computation to a
helper class that caches these
On 2016/08/20 09:49:53, perkj_webrtc wrote:
> change this todo? bemasc no longer works for webrtc. Is the todo still
correct?

I guess it should be reassigned but I'm not sure to whom...

[hta-webrtc, 2016-08-24 14:07:14]

So the chief refactoring is that you move the internal->external representation
of the certs' properties into webrtc/base/sslidentity, right?

Looks reasonable. But there are no tests for the new API.

[hbos, 2016-08-24 16:55:33]

Patchset #2 (id:60001) has been deleted

[hbos, 2016-08-25 08:13:01]

PTAL hta.

On 2016/08/24 14:07:14, hta-webrtc wrote:
> So the chief refactoring is that you move the internal->external
representation
> of the certs' properties into webrtc/base/sslidentity, right?
> 
> Looks reasonable. But there are no tests for the new API.

That is correct. Added unittests.

[hta-webrtc, 2016-08-25 09:32:14]

lgtm

https://codereview.webrtc.org/2259283002/diff/100001/webrtc/base/sslidentity_...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/2259283002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:487: for (size_t i = 0; i <
info.ders.size(); ++i) {
Do you want to add EXPECT_GE(1, info.ders.size() (or whatever the expected size
actually is), to guard against the bug where no certs are returned?

[hbos, 2016-08-25 10:25:42]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-08-25 10:25:43]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2259283002/#ps100001 (title:
"Another unittest for the case of certificate chains using fake certificates")

[commit-bot: I haz the power, 2016-08-25 10:25:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2016-08-25 10:46:33]

https://codereview.webrtc.org/2259283002/diff/100001/webrtc/base/sslidentity_...
File webrtc/base/sslidentity_unittest.cc (right):

https://codereview.webrtc.org/2259283002/diff/100001/webrtc/base/sslidentity_...
webrtc/base/sslidentity_unittest.cc:487: for (size_t i = 0; i <
info.ders.size(); ++i) {
On 2016/08/25 09:32:14, hta-webrtc wrote:
> Do you want to add EXPECT_GE(1, info.ders.size() (or whatever the expected
size
> actually is), to guard against the bug where no certs are returned?

Not necessary, I have already EXPECT checked that there are as many ders, pems
and fingerprints as there are |ders|.

[commit-bot: I haz the power, 2016-08-25 10:52:43]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-08-25 10:52:49]

Description was changed from

==========
Refactor certificate stats collection, added SSLCertificateStats.

The code that extracts certificate stats from an SSLCertificate and its
certificate chain is moved into SSLCertificate::GetStats. The stats
collector code loops through the resulting SSLCertificateStats and
creates the StatsReports for those stats.

This will allow the new stats collector to reuse GetStats in a future
CL.

BUG=chromium:627816, chromium:629436
==========

to

==========
Refactor certificate stats collection, added SSLCertificateStats.

The code that extracts certificate stats from an SSLCertificate and its
certificate chain is moved into SSLCertificate::GetStats. The stats
collector code loops through the resulting SSLCertificateStats and
creates the StatsReports for those stats.

This will allow the new stats collector to reuse GetStats in a future
CL.

BUG=chromium:627816, chromium:629436

Committed: https://crrev.com/e29352bb34de60bd0a56d4ce46c2ce35ac2b27b4
Cr-Commit-Position: refs/heads/master@{#13917}
==========

[commit-bot: I haz the power, 2016-08-25 10:52:50]

Patchset 3 (id:??) landed as
https://crrev.com/e29352bb34de60bd0a56d4ce46c2ce35ac2b27b4
Cr-Commit-Position: refs/heads/master@{#13917}