Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964283002:
Add chrome channel to cert logger reports (Closed)
DescriptionAdd chrome channel to cert logger reports.
The cert logger reports are used to gather information on certificate
failures, and are helpful for background research for the Enamel team's
interstitials. We've added chrome channels to these certificate logs in
order to better isolate the root of problems we might see arise.
BUG=738618
TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc.
Review-Url: https://codereview.chromium.org/2964283002
Cr-Commit-Position: refs/heads/master@{#486069}
Committed: https://chromium.googlesource.com/chromium/src/+/6f80e9efd510215e723a29e6260b98a6c5d5e403
Patch Set 1 #Patch Set 2 : Clean up test comments #
Total comments: 18
Patch Set 3 : Address CL comments #Patch Set 4 : Revert unneeded change #
Total comments: 14
Patch Set 5 : Add browser test to test end to end cert report workflow #
Total comments: 3
Patch Set 6 : Add dependency to build file #Patch Set 7 : More dependency isuses #Patch Set 8 : Rebase on master #Patch Set 9 : Fix build errors #Messages
Total messages: 54 (35 generated)
The CQ bit was checked by sperigo@chromium.org
The CQ bit was unchecked by sperigo@chromium.org
Description was changed from ========== Add chrome channel to cert logger reports BUG=38030340 ========== to ========== Add chrome channel to cert logger reports. TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc. BUG=38030340 ==========
sperigo@chromium.org changed reviewers: + meacer@chromium.org
sperigo@chromium.org changed reviewers: + felt@chromium.org
sperigo@chromium.org changed reviewers: + thestig@chromium.org
Description was changed from ========== Add chrome channel to cert logger reports. TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc. BUG=38030340 ========== to ========== Add chrome channel to cert logger reports. The cert logger reports are used to gather information on certificate failures, and are helpful for background research for the Enamel team's interstitials. We've added chrome channels to these certificate logs in order to better isolate the root of problems we might see arise. TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc. BUG=38030340 ==========
Hello! This is a CL to add chrome channel to cert logger reports. The cert logger reports are used to gather information on certificate failures, and are helpful for background research for the Enamel team's interstitials. We've added chrome channels to these certificate logs in order to better isolate the root of problems we might see arise.
sperigo@chromium.org changed reviewers: + estark@chromium.org - felt@chromium.org
sperigo@chromium.org changed reviewers: - thestig@chromium.org
thestig@chromium.org changed reviewers: + felt@chromium.org, thestig@chromium.org - estark@chromium.org
- When there are multiple reviewers, please indicate who is responsible for reviewing what. e.g. it wasn't obvious why I am on the reviewers list. I'm guessing it's for components/certificate_reporting/DEPS's +components/version_info dependency? - Please wrap the commit message at 72 columns if possible. - Usually with BUG=N, N maps to roughly crbug.com/N. If the bug is not a Chromium bug, the BUG= line needs to somehow denote it's not a Chromium bug. I guess b/N in your case? - Most commonly, the commit message has BUG=...\nTEST=...
https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/cert_logger.proto (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/cert_logger.proto:159: nit: Let's add both the enum and the field after is_retry_upload to keep the ordering. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/encrypted_cert_logger.proto (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/encrypted_cert_logger.proto:30: }; nit: This change does not seem to be needed https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/error_report.cc (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report.cc:182: code = CertLoggerRequest::STABLE; Don't forget "break;"s after each case :) https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/error_report.h (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report.h:25: } You'll want to include version_info instead of forward declaring it, since it's passed by value below. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report.h:80: void AddChromeChannel(const version_info::Channel channel); Since this is an enum, you can just pass it without the const. In Chrome value types are passed without const. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/error_report_unittest.cc (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:271: EXPECT_EQ(CertLoggerRequest::DEV, parsed.chrome_channel()); A bit of an overkill but you might want to test all of the enum values here. You can do that by creating a test struct that contains both the tested value and the expected value, make an array of all enum values and loop through them. Something like this: const struct TestCase { version_info::Channel channel; CertLoggerRequest::ChromeChannel expected_channel; } kTestCases[] = { {version_info::Channel::UNKNOWN, CertLoggerRequest::UNKNOWN}, {version_info::Channel::DEV, CertLoggerRequest::DEV}, ... } for (const TestCase& testCase: kTestCases) { // Create a report, set its channel value and check // if you get back testCase.expected_channel. } This would catch the bug with the break statements :)
DEPS change lgtm https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/error_report_unittest.cc (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:252: base::Thread io_thread("IO thread"); Does your test case need an IO thread? https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:270: EXPECT_TRUE(parsed.chrome_channel()); It is a bit weird to check chrome_channel() twice. Once as a boolean and once with the expected enum value.
Description was changed from ========== Add chrome channel to cert logger reports. The cert logger reports are used to gather information on certificate failures, and are helpful for background research for the Enamel team's interstitials. We've added chrome channels to these certificate logs in order to better isolate the root of problems we might see arise. TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc. BUG=38030340 ========== to ========== Add chrome channel to cert logger reports. The cert logger reports are used to gather information on certificate failures, and are helpful for background research for the Enamel team's interstitials. We've added chrome channels to these certificate logs in order to better isolate the root of problems we might see arise. BUG=738618 TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc. ==========
Sorry about adding you as a reviewer out of the blue here! I'm a new intern, so I'm still getting used to review processes. Thank you for all of your comments. They're very helpful! On 2017/06/30 at 22:17:33, thestig wrote: > - When there are multiple reviewers, please indicate who is responsible for reviewing what. e.g. it wasn't obvious why I am on the reviewers list. I'm guessing it's for components/certificate_reporting/DEPS's +components/version_info dependency? > > - Please wrap the commit message at 72 columns if possible. > > - Usually with BUG=N, N maps to roughly crbug.com/N. If the bug is not a Chromium bug, the BUG= line needs to somehow denote it's not a Chromium bug. I guess b/N in your case? > > - Most commonly, the commit message has BUG=...\nTEST=...
sperigo@chromium.org changed reviewers: + estark@chromium.org - felt@chromium.org
Addressed CL comments. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/cert_logger.proto (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/cert_logger.proto:159: On 2017/06/30 22:24:46, meacer wrote: > nit: Let's add both the enum and the field after is_retry_upload to keep the > ordering. Done. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/error_report.cc (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report.cc:182: code = CertLoggerRequest::STABLE; On 2017/06/30 at 22:24:46, meacer wrote: > Don't forget "break;"s after each case :) Oh no! I'm embarrassed. Good catch. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report.cc:182: code = CertLoggerRequest::STABLE; On 2017/06/30 22:24:46, meacer wrote: > Don't forget "break;"s after each case :) Done. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/error_report.h (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report.h:25: } On 2017/06/30 22:24:46, meacer wrote: > You'll want to include version_info instead of forward declaring it, since it's > passed by value below. Done. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report.h:80: void AddChromeChannel(const version_info::Channel channel); On 2017/06/30 22:24:46, meacer wrote: > Since this is an enum, you can just pass it without the const. In Chrome value > types are passed without const. Done. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... File components/certificate_reporting/error_report_unittest.cc (right): https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:252: base::Thread io_thread("IO thread"); On 2017/06/30 22:32:06, Lei Zhang wrote: > Does your test case need an IO thread? I guess not! Thanks. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:252: base::Thread io_thread("IO thread"); On 2017/06/30 22:32:06, Lei Zhang wrote: > Does your test case need an IO thread? Done. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:270: EXPECT_TRUE(parsed.chrome_channel()); On 2017/06/30 22:32:06, Lei Zhang wrote: > It is a bit weird to check chrome_channel() twice. Once as a boolean and once > with the expected enum value. Done. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:271: EXPECT_EQ(CertLoggerRequest::DEV, parsed.chrome_channel()); On 2017/06/30 at 22:24:46, meacer wrote: > A bit of an overkill but you might want to test all of the enum values here. You can do that by creating a test struct that contains both the tested value and the expected value, make an array of all enum values and loop through them. > > Something like this: > > const struct TestCase { > version_info::Channel channel; > CertLoggerRequest::ChromeChannel expected_channel; > } kTestCases[] = { > {version_info::Channel::UNKNOWN, CertLoggerRequest::UNKNOWN}, > {version_info::Channel::DEV, CertLoggerRequest::DEV}, > ... > } > > for (const TestCase& testCase: kTestCases) { > // Create a report, set its channel value and check > // if you get back testCase.expected_channel. > } > > This would catch the bug with the break statements :) I like this! I'm a fan of over testing over under testing. https://codereview.chromium.org/2964283002/diff/20001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:271: EXPECT_EQ(CertLoggerRequest::DEV, parsed.chrome_channel()); On 2017/06/30 22:24:46, meacer wrote: > A bit of an overkill but you might want to test all of the enum values here. You > can do that by creating a test struct that contains both the tested value and > the expected value, make an array of all enum values and loop through them. > > Something like this: > > const struct TestCase { > version_info::Channel channel; > CertLoggerRequest::ChromeChannel expected_channel; > } kTestCases[] = { > {version_info::Channel::UNKNOWN, CertLoggerRequest::UNKNOWN}, > {version_info::Channel::DEV, CertLoggerRequest::DEV}, > ... > } > > for (const TestCase& testCase: kTestCases) { > // Create a report, set its channel value and check > // if you get back testCase.expected_channel. > } > > This would catch the bug with the break statements :) Done.
Lgtm https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... File components/certificate_reporting/error_report.h (right): https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report.h:10: #include "components/version_info/version_info.h" nit: Add a blank line between <...> and " includes. https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... File components/certificate_reporting/error_report_unittest.cc (right): https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:254: }; nit: This works but could be slightly more compact if you move it inside the test and combine it with the array declaration. struct ChannelTestCase { version_info::Channel channel; CertLoggerRequest::ChromeChannel expected_channel; } kTestCases[] = {... } If you prefer to keep it here, wrap it with an anonymous namespace: namespace { struct ChannelTestCase { ... } } // namespace
https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... File chrome/browser/ssl/cert_report_helper.cc (right): https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... chrome/browser/ssl/cert_report_helper.cc:130: It would be nice to have a test for this line... but I can't think of any way to test it. :( meacer, can you think of anything? https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... File components/certificate_reporting/error_report.cc (right): https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report.cc:200: default: I think you can delete this case. (Better to leave off default when you can because then if someone comes along and adds a new value, this won't compile unless the new value is explicitly handled.) https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... File components/certificate_reporting/error_report_unittest.cc (right): https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:266: for (const ChannelTestCase& testCase : kTestCases) { nit: test_case (and don't forget to change the comment line 265 also)
meacer@google.com changed reviewers: + meacer@google.com
https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... File chrome/browser/ssl/cert_report_helper.cc (right): https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... chrome/browser/ssl/cert_report_helper.cc:130: On 2017/07/01 17:19:01, estark (OOO until Jul 10) wrote: > It would be nice to have a test for this line... but I can't think of any way to > test it. :( meacer, can you think of anything? The obvious way is to write a browser test that: - Initiates an https server - Enables the reporting option - Visits the a broken SSL URL and navigates away to trigger a report - Checks the report's contents CertificateReportingServiceBrowserTest does something similar. The simplest test there is OptedIn_ShouldSendSuccessfulReport, perhaps you can use it as a starting point? Actually, instead of setting up everything again in a new test file perhaps you can just add a new test to CertificateReportingServiceBrowserTest and check the report contents. estark: Would that work?
https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... File chrome/browser/ssl/cert_report_helper.cc (right): https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... chrome/browser/ssl/cert_report_helper.cc:130: On 2017/07/01 20:14:03, Mustafa Acer wrote: > On 2017/07/01 17:19:01, estark (OOO until Jul 10) wrote: > > It would be nice to have a test for this line... but I can't think of any way > to > > test it. :( meacer, can you think of anything? > > The obvious way is to write a browser test that: > - Initiates an https server > - Enables the reporting option > - Visits the a broken SSL URL and navigates away to trigger a report > - Checks the report's contents > > CertificateReportingServiceBrowserTest does something similar. The simplest test > there is OptedIn_ShouldSendSuccessfulReport, perhaps you can use it as a > starting point? > > Actually, instead of setting up everything again in a new test file perhaps you > can just add a new test to CertificateReportingServiceBrowserTest and check the > report contents. > > estark: Would that work? That sounds good! I was thinking that I'm not sure what chrome::GetChannel() will return in a test, but maybe it doesn't matter; we can just check that the report has a non-default value for the channel. (The only other thing is that I'm not seeing that any of those CertificateReportingServiceBrowserTests actually check the report contents. There's a private CheckReports method but it doesn't look like it's called anywhere; maybe that's supposed to be called from somewhere?) (Also, sorry for the weekend emails, Sasha -- just wanted to make sure I wasn't blocking this while I'm on vacation next week.)
https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... File chrome/browser/ssl/cert_report_helper.cc (right): https://codereview.chromium.org/2964283002/diff/60001/chrome/browser/ssl/cert... chrome/browser/ssl/cert_report_helper.cc:130: On 2017/07/02 00:09:12, estark (OOO until Jul 10) wrote: > On 2017/07/01 20:14:03, Mustafa Acer wrote: > > On 2017/07/01 17:19:01, estark (OOO until Jul 10) wrote: > > > It would be nice to have a test for this line... but I can't think of any > way > > to > > > test it. :( meacer, can you think of anything? > > > > The obvious way is to write a browser test that: > > - Initiates an https server > > - Enables the reporting option > > - Visits the a broken SSL URL and navigates away to trigger a report > > - Checks the report's contents > > > > CertificateReportingServiceBrowserTest does something similar. The simplest > test > > there is OptedIn_ShouldSendSuccessfulReport, perhaps you can use it as a > > starting point? > > > > Actually, instead of setting up everything again in a new test file perhaps > you > > can just add a new test to CertificateReportingServiceBrowserTest and check > the > > report contents. > > > > estark: Would that work? > > That sounds good! I was thinking that I'm not sure what chrome::GetChannel() > will return in a test, but maybe it doesn't matter; we can just check that the > report has a non-default value for the channel. Just chatted with Sasha. It seems easier to just augment MockSSLCertReporter::ReportInvalidCertificateChain() and add channel information to the report sent callback, and then check it in TestBrokenHTTPSReporting() in ssl_browser_tests.cc. Do you concur? > > (The only other thing is that I'm not seeing that any of those > CertificateReportingServiceBrowserTests actually check the report contents. > There's a private CheckReports method but it doesn't look like it's called > anywhere; maybe that's supposed to be called from somewhere?) I have little memory of this code, but I think I stopped checking report contents because this was already done elsewhere (SSLUITest and other places). It made the tests simpler and avoided duplicate test cases. CheckReports() is a remnant of that old code and is no longer necessary, I can delete it. > > (Also, sorry for the weekend emails, Sasha -- just wanted to make sure I wasn't > blocking this while I'm on vacation next week.)
I added on to the browser tests in SSLUITest in order to test the end to end cert report workflow in order to address meacer and estark's comments. https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... File components/certificate_reporting/error_report.cc (right): https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report.cc:200: default: On 2017/07/01 17:19:01, estark (OOO until Jul 10) wrote: > I think you can delete this case. (Better to leave off default when you can > because then if someone comes along and adds a new value, this won't compile > unless the new value is explicitly handled.) This is really smart. Thanks! https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report.cc:200: default: On 2017/07/01 17:19:01, estark (OOO until Jul 10) wrote: > I think you can delete this case. (Better to leave off default when you can > because then if someone comes along and adds a new value, this won't compile > unless the new value is explicitly handled.) Done. https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... File components/certificate_reporting/error_report.h (right): https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report.h:10: #include "components/version_info/version_info.h" On 2017/06/30 23:58:39, meacer wrote: > nit: Add a blank line between <...> and " includes. Done. https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... File components/certificate_reporting/error_report_unittest.cc (right): https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:254: }; On 2017/06/30 23:58:39, meacer wrote: > nit: This works but could be slightly more compact if you move it inside the > test and combine it with the array declaration. > > struct ChannelTestCase { > version_info::Channel channel; > CertLoggerRequest::ChromeChannel expected_channel; > } kTestCases[] = {... > } > > If you prefer to keep it here, wrap it with an anonymous namespace: > > namespace { > struct ChannelTestCase { > ... > } > } // namespace Done. https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:266: for (const ChannelTestCase& testCase : kTestCases) { On 2017/07/01 17:19:01, estark (OOO until Jul 10) wrote: > nit: test_case (and don't forget to change the comment line 265 also) Oops! Thank you. https://codereview.chromium.org/2964283002/diff/60001/components/certificate_... components/certificate_reporting/error_report_unittest.cc:266: for (const ChannelTestCase& testCase : kTestCases) { On 2017/07/01 17:19:01, estark (OOO until Jul 10) wrote: > nit: test_case (and don't forget to change the comment line 265 also) Done.
https://codereview.chromium.org/2964283002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2964283002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:584: EXPECT_EQ(certificate_reporting::CertLoggerRequest::CHROME_CHANNEL_NONE, Does this pass? I'd expect it to be CHROME_CHANNEL_UNKNOWN because AddChromeChannel never returns CHROME_CHANNEL_NONE.
https://codereview.chromium.org/2964283002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2964283002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:584: EXPECT_EQ(certificate_reporting::CertLoggerRequest::CHROME_CHANNEL_NONE, On 2017/07/10 at 20:43:01, estark wrote: > Does this pass? I'd expect it to be CHROME_CHANNEL_UNKNOWN because AddChromeChannel never returns CHROME_CHANNEL_NONE. It does! Mustafa and I spent some time designing this test late last week. Right now we're explicitly setting the Channel to CHROME_CHANNEL_NONE in the test help constructor in order to check that the Channel isn't set before AddChromeChannel executes.
https://codereview.chromium.org/2964283002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2964283002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:584: EXPECT_EQ(certificate_reporting::CertLoggerRequest::CHROME_CHANNEL_NONE, On 2017/07/10 22:43:23, sperigo wrote: > On 2017/07/10 at 20:43:01, estark wrote: > > Does this pass? I'd expect it to be CHROME_CHANNEL_UNKNOWN because > AddChromeChannel never returns CHROME_CHANNEL_NONE. > > It does! Mustafa and I spent some time designing this test late last week. Right > now we're explicitly setting the Channel to CHROME_CHANNEL_NONE in the test help > constructor in order to check that the Channel isn't set before AddChromeChannel > executes. Ohh, I see; I actually meant to put this comment on line 603, and I also misread line 603 as EXPECT_EQ. My bad (x2) -- lgtm.
sperigo@chromium.org changed reviewers: - meacer@google.com
The CQ bit was checked by sperigo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2964283002/#ps80001 (title: "Add browser test to test end to end cert report workflow")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sperigo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, meacer@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2964283002/#ps160001 (title: "Fix build errors")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499889821207810,
"parent_rev": "4d3622e28d891b3a445275d132a6817bb83973ed", "commit_rev":
"6f80e9efd510215e723a29e6260b98a6c5d5e403"}
Message was sent while issue was closed.
Description was changed from ========== Add chrome channel to cert logger reports. The cert logger reports are used to gather information on certificate failures, and are helpful for background research for the Enamel team's interstitials. We've added chrome channels to these certificate logs in order to better isolate the root of problems we might see arise. BUG=738618 TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc. ========== to ========== Add chrome channel to cert logger reports. The cert logger reports are used to gather information on certificate failures, and are helpful for background research for the Enamel team's interstitials. We've added chrome channels to these certificate logs in order to better isolate the root of problems we might see arise. BUG=738618 TEST=Unit test added in components/certificate_reporting/error_report_unittest.cc. Review-Url: https://codereview.chromium.org/2964283002 Cr-Commit-Position: refs/heads/master@{#486069} Committed: https://chromium.googlesource.com/chromium/src/+/6f80e9efd510215e723a29e6260b... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/6f80e9efd510215e723a29e6260b... |
