Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494103003:
RTCCertificate::Expires() and ::HasExpired() implemented (Closed)
DescriptionRTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime().
NOPRESUBMIT=true
BUG=chromium:544894
Committed: https://crrev.com/20ef654174e245b3a06c9e9045bb97be9acd90cf
Cr-Commit-Position: refs/heads/master@{#10930}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments, added unittest #
Total comments: 11
Patch Set 3 : Test w FakeSSLCertificate w expires setter. HasExpired tested with expires 10s from now #
Total comments: 3
Patch Set 4 : Injecting time @ HasExpired. Unittests /w seconds. SSLCertificate instead of FakeSSLCertificate. #
Total comments: 8
Patch Set 5 : Addressed nits #
Messages
Total messages: 52 (19 generated)
Description was changed from ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime() BUG=webrtc:544894 ========== to ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime(). BUG=chromium:544894 ==========
hbos@webrtc.org changed reviewers: + hta@webrtc.org, torbjorng@webrtc.org
Please take a look torbjorng and hta.
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494103003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494103003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc... webrtc/base/rtccertificate.cc:34: return static_cast<uint64_t>(expires * 1000); // s -> ms nit: if uint64_t vs int64_t typing different really matters, then it seems cleaner to do (expired) * 1000 instead. https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc... webrtc/base/rtccertificate.cc:35: // If the expiration time could not be retrieved return an expired timestamp. Might add why 0 is expired (i.e., say that this is 1970-01-01. https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc... webrtc/base/rtccertificate.cc:40: return Expires() <= TimeMicros(); Isn't Expires in ms and TimeMicros in ms/1000?
Looks good (modulo tests and other comments). https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.h File webrtc/base/rtccertificate.h (right): https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.h#... webrtc/base/rtccertificate.h:32: bool HasExpired() const; What's the argument for using CamelCase for these functions and lower_case_dashes for all the others?
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494103003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494103003/20001
PTAL torbjorng https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc... webrtc/base/rtccertificate.cc:34: return static_cast<uint64_t>(expires * 1000); // s -> ms On 2015/12/03 13:05:18, torbjorng (webrtc) wrote: > nit: if uint64_t vs int64_t typing different really matters, then it seems > cleaner to do > (expired) * 1000 > instead. Done. (It would work implicitly anyway just wanted to be extra clear about the fact that one is signed and the other is unsigned) https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc... webrtc/base/rtccertificate.cc:35: // If the expiration time could not be retrieved return an expired timestamp. On 2015/12/03 13:05:18, torbjorng (webrtc) wrote: > Might add why 0 is expired (i.e., say that this is 1970-01-01. Done. https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.cc... webrtc/base/rtccertificate.cc:40: return Expires() <= TimeMicros(); On 2015/12/03 13:05:18, torbjorng (webrtc) wrote: > Isn't Expires in ms and TimeMicros in ms/1000? Yes, oops, clumsy. https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.h File webrtc/base/rtccertificate.h (right): https://codereview.webrtc.org/1494103003/diff/1/webrtc/base/rtccertificate.h#... webrtc/base/rtccertificate.h:32: bool HasExpired() const; On 2015/12/03 13:49:07, hta-webrtc wrote: > What's the argument for using CamelCase for these functions and > lower_case_dashes for all the others? All functions should be UpperCamel except tiny and "cheap" ones like getters, https://google.github.io/styleguide/cppguide.html#Function_Names. Getting the Expires() time involves lower level crypto stuff and as it happens does some parsing so I'm not making them lower_snake like identity() and ssl_certificate(). https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:40: return Expires() <= TimeNanos() / kNumNanosecsPerMillisec; Note: The alternative Time(), which returns in ms, is uint32_t - not uint64_t - that's why I'm doing the silly conversion.
And PTAL hta also :) if it's confusing that I'm using ms units but seconds-precision in the unittest I could change all unittests to work entirely with seconds and wrap certificate->Expires() into a helper function that returns it in seconds instead. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Not happy with the tests - waiting and looping in a test is a sign of design problems. The fact that rtccertificate.cc calls a non-mockable function for time is a problem for testability. It's such a simple function that it seems like overkill to make the time function injectable - but if there's an injectable time function somewhere in your environment, consider using it. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittests.cc (right): https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:58: scoped_ptr<SSLIdentity> identity(rtc::SSLIdentity::GenerateForTest(params)); You can make the test a lot more isolated (not depend on rtc::SSLIdentity) if you use a mock certificate. You're not testing rtc::SSLIdentity here, you're testing the handling code. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:74: while (retry) { Looping until the times align is a Bad Thing. You have no idea how many times you will go around the loop. If you mock the certificate, you have much more precise control. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:94: Thread::Current()->SleepMs(1500); Argh. This is going to cost us heavily. Sleeping in tests is a Bad Thing - it wastes 1.5 seconds every time the tests are run. Good unit tests finish in milliseconds. I think this test could be much simpler - generate two certificates, one that expires at now-1000 and one that expires at now+1000, check that one has expired and one is not. There's a risk that the future-expired certificate will be flaky - you never know how much time will pass between any two lines in a test - but 1000 seconds should be enough to make it not-flaky. (Injectable time would make it not-flaky too, of course).
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494103003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494103003/40001
PTAL. Using FakeSSLCertificate I can set the expires time directly. Are you happy with this? (I'm still relying on getting the current time though) https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittests.cc (right): https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:58: scoped_ptr<SSLIdentity> identity(rtc::SSLIdentity::GenerateForTest(params)); On 2015/12/03 18:16:05, hta-webrtc wrote: > You can make the test a lot more isolated (not depend on rtc::SSLIdentity) if > you use a mock certificate. You're not testing rtc::SSLIdentity here, you're > testing the handling code. > Ok. I'll add a expiration setter to FakeSSLCertificate and use that. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:74: while (retry) { On 2015/12/03 18:16:05, hta-webrtc wrote: > Looping until the times align is a Bad Thing. > > You have no idea how many times you will go around the loop. > If you mock the certificate, you have much more precise control. Point taken, I'll simplify. Though as-is, one test run takes but milliseconds, so the odds of having to try again is very small. If you do try again you'll no longer be "on the edge" between second 1 and second 2 so the odds of having to try yet another time is exceedingly unlikely. But even if this could run for a 100 times that wouldn't be a performance problem. That being said if there is a regression and the assumption that one attempt is fast no longer holds we could get stuck in some undesirable looping. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:94: Thread::Current()->SleepMs(1500); On 2015/12/03 18:16:05, hta-webrtc wrote: > Argh. This is going to cost us heavily. Sleeping in tests is a Bad Thing - it > wastes 1.5 seconds every time the tests are run. Good unit tests finish in > milliseconds. > > I think this test could be much simpler - generate two certificates, one that > expires at now-1000 and one that expires at now+1000, check that one has expired > and one is not. > > There's a risk that the future-expired certificate will be flaky - you never > know how much time will pass between any two lines in a test - but 1000 seconds > should be enough to make it not-flaky. > (Injectable time would make it not-flaky too, of course). > Ok. I wanted to test going from non-expired to expired but I guess sleeping is not worth it. FakeSSLCertificate with expiration injection it is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_msan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/6412)
lgtm https://codereview.webrtc.org/1494103003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittests.cc (right): https://codereview.webrtc.org/1494103003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:70: uint64_t now = Now() + 10 * 1000; // 10s in the future 10 sec is probably OK. It's unlikely that we're going to see the bots choosing to sleep for 10 seconds right here, but if it turns out we're wrong, we can change it.
PTAL torbjorng
I'm afraid I disagree with hta and the latest test edits. I explain it carefully in a comment above. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittests.cc (right): https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:11: #include "webrtc/base/checks.h" nit: I don't think all these #Includes are needed. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:41: // number of ms since epoch, 1970-01-01T00:00:00Z. The expiration time is Note that a cert cannot store greater accuracy than 1 s. See RFC 5280. I really don't like that we mess around with ms for expiration, since this can only lead to confusion which may lead to failure to expire. I realise that the WebRTC draft defines expiration in ms. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:94: Thread::Current()->SleepMs(1500); On 2015/12/03 18:16:05, hta-webrtc wrote: > Argh. This is going to cost us heavily. Sleeping in tests is a Bad Thing - it > wastes 1.5 seconds every time the tests are run. Good unit tests finish in > milliseconds. > > I think this test could be much simpler - generate two certificates, one that > expires at now-1000 and one that expires at now+1000, check that one has expired > and one is not. > > There's a risk that the future-expired certificate will be flaky - you never > know how much time will pass between any two lines in a test - but 1000 seconds > should be enough to make it not-flaky. > (Injectable time would make it not-flaky too, of course). > I disagree (with the Argh and with the technical contents). My view is that tests which run as close to reality are better than those that take artificial approaches. For certificates, we will generate them, then they will be valid and we should recognise them as valid, and later they will expire and we should recognise them as expired. Generating expired certificates and certificates valid longer than we can check for the expiration to happen will not make adequate tests of the mechanism which we need to test. I'd suggest a slight modification to hbos' original code, but the code was fine already. My suggestion is that one does not loop in the case the cert is expired directly after creation, at least not indefinitely. Instead just skip this test. As a complement, one should generate a certificate with say 100 seconds validity and fail if that is expired. This will never be flaky. It will make sure that no second/millisecond confusion exists or is introduced, and it will verify that the expiration mechanism is sound. This test is important enough that it is worth 1.5 seconds of wall clock time. We already spend several minutes of CPU time on testing the crypto code.
I'm afraid I disagree strongly with Torbjörn here. Unit tests should be small tests. A test that waits 1.5 seconds is not a small test. https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate_unittests.cc (right): https://codereview.webrtc.org/1494103003/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate_unittests.cc:94: Thread::Current()->SleepMs(1500); On 2015/12/04 12:47:46, torbjorng (webrtc) wrote: > On 2015/12/03 18:16:05, hta-webrtc wrote: > > Argh. This is going to cost us heavily. Sleeping in tests is a Bad Thing - it > > wastes 1.5 seconds every time the tests are run. Good unit tests finish in > > milliseconds. > > > > I think this test could be much simpler - generate two certificates, one that > > expires at now-1000 and one that expires at now+1000, check that one has > expired > > and one is not. > > > > There's a risk that the future-expired certificate will be flaky - you never > > know how much time will pass between any two lines in a test - but 1000 > seconds > > should be enough to make it not-flaky. > > (Injectable time would make it not-flaky too, of course). > > > > I disagree (with the Argh and with the technical contents). > > My view is that tests which run as close to reality are better than those that > take artificial approaches. For certificates, we will generate them, then they > will be valid and we should recognise them as valid, and later they will expire > and we should recognise them as expired. > > Generating expired certificates and certificates valid longer than we can check > for the expiration to happen will not make adequate tests of the mechanism which > we need to test. > > I'd suggest a slight modification to hbos' original code, but the code was fine > already. My suggestion is that one does not loop in the case the cert is expired > directly after creation, at least not indefinitely. Instead just skip this > test. As a complement, one should generate a certificate with say 100 seconds > validity and fail if that is expired. > > This will never be flaky. It will make sure that no second/millisecond confusion > exists or is introduced, and it will verify that the expiration mechanism is > sound. > > This test is important enough that it is worth 1.5 seconds of wall clock time. > We already spend several minutes of CPU time on testing the crypto code. I'm afraid I disagree about the importance. This is an unit test for code that, in itself, has no state; the tests are already longer than the code it's testing. The test as written now tests that a certificate with an old expiry date shows as expired and that a certificate with an expiry date in the future shows as not expired; I think that's what we want to test. A real test that the same certificate will show as not-expired before some time and expired after that time is simple to write - but it requires an injected clock that is under the test's control. In my opinion, developers should run the unit tests for their module every time they make a change to that module, almost as an instinctive action. Each second spent extra here is a second of programmer's time lost. Some modules require, as you say, minutes of CPU time. Those are large tests, not unit tests. We won't ask programmers to run those on every change to their module while they're developing. https://wiki.corp.google.com/twiki/bin/view/Main/GoogleTestDefinitions#Defini... is old, but good. This is an unit test, and should be a small test.
If we leave out the definition of unittest, do we all agree that we should catch
confusion between ms and s in the various paths involved here?
And do we all agree that testing that generating a real certificate from a high
level ("WebRTC RFC level"), checking that the entire path for setting and
checking expiration?
Imagine that seconds and milliseconds are confused for setting or reading the
expiration. Now, the modified test code will very likely not discover that.
That's a test, not a unittest, but naming things is not the most important thing
here.
Torbjörn you're right in that testing a cert going from non-expired to expired is a good test case representative of real usage cases. But testing both expired and non-expired certificates separately is a pretty good test coverage too, and not having to sleep is good. I say good enough. It's a compromise. (We could check time with an injectable clock interface that can be mocked... but I think that's overkill considering the code being tested.) I'd be happy to make the unittest work with seconds to match the underlying precision. I also see the benefit of using a real SSLIdentity/SSLCertificate instead of a Fake one, I could revert that change.
So how about we make "now" a parameter of RTCCertificate::HasExpired and inject it in the unittest as 1 second later, without having to wait? And I make the unittest work with seconds instead of ms where seconds-precision is used? Everybody happy campers? :)
Your proposal sounds good; we will be back to a vertically checked expiration chain. https://codereview.webrtc.org/1494103003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1494103003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:34: return static_cast<uint64_t>(expires) * 1000; // s -> ms nit: use kNumMillisecsPerSec analogous to code below.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
PTAL torbjorng and hta. Trying to make both happy. I'm back to using SSLIdentity/SSLCertificate instead of mocks of them (FakeSSLIdentity/FakeSSLCertificate) to test closer to the real world scenario and to test generating a cert without specifying the expiration time. If you don't like this hta@ I can make it a FakeSSLCertificate again before landing but I think it makes sense in this case. https://codereview.webrtc.org/1494103003/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1494103003/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:34: return static_cast<uint64_t>(expires) * 1000; // s -> ms On 2015/12/07 12:43:19, torbjorng (webrtc) wrote: > nit: use kNumMillisecsPerSec analogous to code below. Done.
lgtm Some minor comments only. https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/fakesslident... File webrtc/base/fakesslidentity.h (right): https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/fakesslident... webrtc/base/fakesslidentity.h:89: int64_t expiration_time_; nit: Please add comment about unit (seconds?) https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... File webrtc/base/rtccertificate_unittests.cc (right): https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... webrtc/base/rtccertificate_unittests.cc:39: // seconds-precision is supported by the crypto library. To make the tests nit: "the crypto library" => X509 https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... webrtc/base/rtccertificate_unittests.cc:46: return TimeNanos() / (kNumNanosecsPerMillisec * kNumMillisecsPerSec); Please use just kNumNanosecsPerSec. https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... webrtc/base/rtccertificate_unittests.cc:86: SSLIdentity::Generate(kTestCertCommonName, KeyParams::ECDSA())); You specify EC_NIST_P256 explicitly a few lines before, with the same meaning. Please use either form consistently.
lgtm Now I'm happy!
https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/fakesslident... File webrtc/base/fakesslidentity.h (right): https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/fakesslident... webrtc/base/fakesslidentity.h:89: int64_t expiration_time_; On 2015/12/07 14:53:26, torbjorng (webrtc) wrote: > nit: Please add comment about unit (seconds?) Done. https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... File webrtc/base/rtccertificate_unittests.cc (right): https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... webrtc/base/rtccertificate_unittests.cc:39: // seconds-precision is supported by the crypto library. To make the tests On 2015/12/07 14:53:26, torbjorng (webrtc) wrote: > nit: "the crypto library" => X509 I'll make it say "SSLCertificate" instead since that is the interface used on this layer, X509 or not (though in practice = X509). https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... webrtc/base/rtccertificate_unittests.cc:46: return TimeNanos() / (kNumNanosecsPerMillisec * kNumMillisecsPerSec); On 2015/12/07 14:53:26, torbjorng (webrtc) wrote: > Please use just kNumNanosecsPerSec. Aj aj kapten https://codereview.webrtc.org/1494103003/diff/120001/webrtc/base/rtccertifica... webrtc/base/rtccertificate_unittests.cc:86: SSLIdentity::Generate(kTestCertCommonName, KeyParams::ECDSA())); On 2015/12/07 14:53:26, torbjorng (webrtc) wrote: > You specify EC_NIST_P256 explicitly a few lines before, with the same meaning. > Please use either form consistently. Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/1494103003/#ps140001 (title: "Addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494103003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494103003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2243)
hbos@webrtc.org changed reviewers: + kjellander@chromium.org
+kjellander I'm getting the following presubmit message: As you're changing GYP files: please make sure corresponding BUILD.gn files are also updated. Changed GYP files: webrtc/base/base_tests.gyp But I've just added a unittest. Which is GYP-only. Isn't this just a warning? Why can't I land? :S
On 2015/12/07 16:59:52, hbos wrote: > +kjellander > > I'm getting the following presubmit message: > > As you're changing GYP files: please make sure corresponding BUILD.gn files are > also updated. > Changed GYP files: > webrtc/base/base_tests.gyp > > But I've just added a unittest. Which is GYP-only. Isn't this just a warning? > Why can't I land? :S The GN dialog is just information, but the API check is a warning, which is wrong (it should also just be information). I'll update that in https://codereview.webrtc.org/1507723002/ Feel free to land this with NOPRESUBMIT=True in the meantime, and sorry about that!
Description was changed from ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime(). BUG=chromium:544894 ========== to ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime(). NOPRESUBMIT=true BUG=chromium:544894 ==========
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494103003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494103003/140001
Message was sent while issue was closed.
Description was changed from ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime(). NOPRESUBMIT=true BUG=chromium:544894 ========== to ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime(). NOPRESUBMIT=true BUG=chromium:544894 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime(). NOPRESUBMIT=true BUG=chromium:544894 ========== to ========== RTCCertificate::Expires() and ::HasExpired() implemented using SSLCertificate::CertificateExpirationTime(). NOPRESUBMIT=true BUG=chromium:544894 Committed: https://crrev.com/20ef654174e245b3a06c9e9045bb97be9acd90cf Cr-Commit-Position: refs/heads/master@{#10930} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/20ef654174e245b3a06c9e9045bb97be9acd90cf Cr-Commit-Position: refs/heads/master@{#10930}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:140001) has been created in https://codereview.webrtc.org/1506883005/ by hbos@webrtc.org. The reason for reverting is: RTCCertificate's expires_timestamp_ns was renamed to Expires but the old function is still used in one place in Chromium... https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Mac%20Bui.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
