Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(29)

Issue 1212973002: Add net::CertificateReportSender for handling cert report sending (Closed)

Created:
5 years, 6 months ago by estark
Modified:
5 years, 4 months ago
Reviewers:
davidben, Ryan Sleevi, mattm
CC:
chromium-reviews, grt+watch_chromium.org, cbentzel+watch_chromium.org, Ryan Sleevi
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add net::CertificateReportSender for handling cert report sending net::CertificateReportSender contains code factored out of CertificateErrorReporter in //chrome. net::CertificateReportSender will be used for both HPKP violation reports and the Safe Browsing Extended Reporting cert reports that CertificateErrorReporter handles. CL #1: crrev.com/1211363005 (parse report-uri) This is CL #2. CL #3: crrev.com/1212613004 (build and send HPKP reports) BUG=445793 Committed: https://crrev.com/19be6db116a497bc311a7faf80279a1233eb7ace Cr-Commit-Position: refs/heads/master@{#340641}

Patch Set 1 #

Total comments: 26

Patch Set 2 : rebase #

Patch Set 3 : rebase #

Patch Set 4 : rsleevi comments #

Patch Set 5 : fix for CookiesPreference relocation #

Patch Set 6 : rebase #

Patch Set 7 : style fixes #

Total comments: 37

Patch Set 8 : davidben comments #

Patch Set 9 : pare down reporter interface to just Send() #

Total comments: 7

Patch Set 10 : rebase #

Patch Set 11 : rebase #

Patch Set 12 : tests in anon namespace / git cl format #

Patch Set 13 : oops, unit test fixes #

Patch Set 14 : more unit test fixes #

Total comments: 2

Patch Set 15 : fix test namespace #

Total comments: 6

Patch Set 16 : mattm comments #

Patch Set 17 : add missing NET_EXPORT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+599 lines, -435 lines) Patch
M chrome/browser/net/certificate_error_reporter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +17 lines, -37 lines 0 comments Download
M chrome/browser/net/certificate_error_reporter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 6 chunks +15 lines, -75 lines 0 comments Download
M chrome/browser/net/certificate_error_reporter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +81 lines, -299 lines 0 comments Download
M chrome/browser/safe_browsing/ping_manager.cc View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ssl/certificate_reporting_test_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +13 lines, -10 lines 0 comments Download
M chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ssl/chrome_fraudulent_certificate_reporter_unittest.cc View 1 2 3 4 5 6 7 8 4 chunks +28 lines, -9 lines 0 comments Download
M net/http/transport_security_state.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +10 lines, -0 lines 0 comments Download
M net/net.gypi View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
A net/url_request/certificate_report_sender.h View 1 2 3 4 5 6 7 8 1 chunk +76 lines, -0 lines 0 comments Download
A net/url_request/certificate_report_sender.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +76 lines, -0 lines 0 comments Download
A net/url_request/certificate_report_sender_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +274 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (11 generated)
estark
This is CL 2/4 split out from https://codereview.chromium.org/1211933005/.
5 years, 6 months ago (2015-06-26 19:28:33 UTC) #2
Ryan Sleevi
https://codereview.chromium.org/1212973002/diff/1/net/http/certificate_report_sender.h File net/http/certificate_report_sender.h (right): https://codereview.chromium.org/1212973002/diff/1/net/http/certificate_report_sender.h#newcode17 net/http/certificate_report_sender.h:17: // reports to a URI. nit: Seems like this ...
5 years, 6 months ago (2015-06-26 20:03:47 UTC) #4
estark
https://codereview.chromium.org/1212973002/diff/1/net/http/certificate_report_sender.h File net/http/certificate_report_sender.h (right): https://codereview.chromium.org/1212973002/diff/1/net/http/certificate_report_sender.h#newcode17 net/http/certificate_report_sender.h:17: // reports to a URI. On 2015/06/26 20:03:46, Ryan ...
5 years, 5 months ago (2015-07-07 21:59:29 UTC) #6
estark
Friendly ping... David, are you still in code review bankruptcy? Ryan, are you still catching ...
5 years, 5 months ago (2015-07-22 04:56:28 UTC) #7
davidben
https://codereview.chromium.org/1212973002/diff/140001/chrome/browser/net/certificate_error_reporter.h File chrome/browser/net/certificate_error_reporter.h (left): https://codereview.chromium.org/1212973002/diff/140001/chrome/browser/net/certificate_error_reporter.h#oldcode86 chrome/browser/net/certificate_error_reporter.h:86: static bool DecryptCertificateErrorReport( (Existing and soon to be irrelevant, ...
5 years, 5 months ago (2015-07-23 00:09:42 UTC) #8
estark
Thanks, David. https://codereview.chromium.org/1212973002/diff/140001/chrome/browser/net/certificate_error_reporter.h File chrome/browser/net/certificate_error_reporter.h (left): https://codereview.chromium.org/1212973002/diff/140001/chrome/browser/net/certificate_error_reporter.h#oldcode86 chrome/browser/net/certificate_error_reporter.h:86: static bool DecryptCertificateErrorReport( On 2015/07/23 00:09:41, David ...
5 years, 5 months ago (2015-07-23 02:41:22 UTC) #9
davidben
You'll want to update the CL description now that stuff has changed. lgtm with a ...
5 years, 5 months ago (2015-07-24 18:54:12 UTC) #10
davidben
https://codereview.chromium.org/1212973002/diff/180001/net/url_request/certificate_report_sender_unittest.cc File net/url_request/certificate_report_sender_unittest.cc (right): https://codereview.chromium.org/1212973002/diff/180001/net/url_request/certificate_report_sender_unittest.cc#newcode130 net/url_request/certificate_report_sender_unittest.cc:130: }; On 2015/07/24 18:54:12, David Benjamin wrote: > } ...
5 years, 5 months ago (2015-07-24 19:09:08 UTC) #11
estark
Thanks, David! mattm: could you please review //chrome/browser/net and //chrome/browser/safe_browsing? This CL factors code out ...
5 years, 5 months ago (2015-07-24 22:56:04 UTC) #13
davidben
https://codereview.chromium.org/1212973002/diff/280001/net/url_request/certificate_report_sender_unittest.cc File net/url_request/certificate_report_sender_unittest.cc (right): https://codereview.chromium.org/1212973002/diff/280001/net/url_request/certificate_report_sender_unittest.cc#newcode22 net/url_request/certificate_report_sender_unittest.cc:22: Oh, I actually meant this. You can nest anonymous ...
5 years, 5 months ago (2015-07-25 01:32:57 UTC) #14
estark
https://codereview.chromium.org/1212973002/diff/280001/net/url_request/certificate_report_sender_unittest.cc File net/url_request/certificate_report_sender_unittest.cc (right): https://codereview.chromium.org/1212973002/diff/280001/net/url_request/certificate_report_sender_unittest.cc#newcode22 net/url_request/certificate_report_sender_unittest.cc:22: On 2015/07/25 01:32:57, David Benjamin wrote: > Oh, I ...
5 years, 5 months ago (2015-07-25 06:28:27 UTC) #15
mattm
lgtm with nits and extraneous discussion https://codereview.chromium.org/1212973002/diff/300001/chrome/browser/net/certificate_error_reporter.cc File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/1212973002/diff/300001/chrome/browser/net/certificate_error_reporter.cc#newcode20 chrome/browser/net/certificate_error_reporter.cc:20: #include "net/url_request/url_request_context.h" The ...
5 years, 4 months ago (2015-07-28 00:15:54 UTC) #16
estark
Thanks Matt! https://codereview.chromium.org/1212973002/diff/300001/chrome/browser/net/certificate_error_reporter.cc File chrome/browser/net/certificate_error_reporter.cc (right): https://codereview.chromium.org/1212973002/diff/300001/chrome/browser/net/certificate_error_reporter.cc#newcode20 chrome/browser/net/certificate_error_reporter.cc:20: #include "net/url_request/url_request_context.h" On 2015/07/28 00:15:54, mattm wrote: ...
5 years, 4 months ago (2015-07-28 00:28:58 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212973002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1212973002/320001
5 years, 4 months ago (2015-07-28 00:29:45 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/21275)
5 years, 4 months ago (2015-07-28 00:41:11 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212973002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1212973002/320001
5 years, 4 months ago (2015-07-28 00:53:56 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/63737)
5 years, 4 months ago (2015-07-28 01:48:42 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212973002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1212973002/340001
5 years, 4 months ago (2015-07-28 03:47:05 UTC) #29
commit-bot: I haz the power
Committed patchset #17 (id:340001)
5 years, 4 months ago (2015-07-28 05:16:56 UTC) #30
commit-bot: I haz the power
5 years, 4 months ago (2015-07-28 05:18:02 UTC) #31
Message was sent while issue was closed.
Patchset 17 (id:??) landed as
https://crrev.com/19be6db116a497bc311a7faf80279a1233eb7ace
Cr-Commit-Position: refs/heads/master@{#340641}

Powered by Google App Engine
This is Rietveld 408576698