Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243123002:
RTCCertificateStats added. (Closed)
|
Created:
4 years, 4 months ago by hbos Modified:
4 years, 2 months ago Reviewers:
hta-webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRTCCertificateStats[1] added.
[1] https://w3c.github.io/webrtc-stats/#certificatestats-dict*
BUG=chromium:627816, chromium:629436
NOTRY=True
Committed: https://crrev.com/6ab97ce0b9564e31c84ea0dbf72e347499d13aa1
Cr-Commit-Position: refs/heads/master@{#14484}
Patch Set 1 #Patch Set 2 : Spec links added to rtcstats_objects.h (no need to rerun bots yet) #Patch Set 3 : Added tracking bug reference to RTCPeerConnectionStats and Rebase master #
Total comments: 10
Patch Set 4 : Addressed comments #
Total comments: 8
Patch Set 5 : Addressed comments #
Messages
Total messages: 33 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
hbos@webrtc.org changed reviewers: + hta@webrtc.org
Please take a look, hta.
Description was changed from ========== RTCCertificateStats added. BUG=chromium:627816, chromium:629436 ========== to ========== RTCCertificateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#certificatestats-dict* BUG=chromium:627816, chromium:629436 ==========
Ping hta
Code looks reasonable (but needs argument swapping to conform to C++ style guide). I'm not that happy with the test code. https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.h:79: void ProduceCertificateStats_s( Why the _s suffix? (here and elsewhere) https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.h:81: const SessionStats& session_stats) const; Order of arguments: First inputs, then outputs. https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:51: CertificateInfo CreateFakeCertificateAndInfoFromDers( This is in the test, so the copying overhead doesn't matter, but I still wince when returning large structs by value. Can we return a std::unique_ptr instead? https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:109: EXPECT_CALL(pc_, session()).WillRepeatedly(Return(&session_)); This number of calls installed with EXPECT_CALL indicates that you're not really using a mock, you're using a fake created via gmock. Are you sure this is the right tool for the job? https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:150: bool session_GetTransportStats(SessionStats* stats) { This is a butt-ugly name for a function. Can you please use a standard-case name for it? GetTransportStatsFromSession might be a better one. As is, this function accesses transport_certificates_, not session_, so the name is misleading too.
On 2016/09/29 13:22:58, hta-webrtc wrote: > Code looks reasonable (but needs argument swapping to conform to C++ style > guide). > > I'm not that happy with the test code. > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > File webrtc/api/rtcstatscollector.h (right): > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > webrtc/api/rtcstatscollector.h:79: void ProduceCertificateStats_s( > Why the _s suffix? (here and elsewhere) > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > webrtc/api/rtcstatscollector.h:81: const SessionStats& session_stats) const; > Order of arguments: First inputs, then outputs. > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > File webrtc/api/rtcstatscollector_unittest.cc (right): > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > webrtc/api/rtcstatscollector_unittest.cc:51: CertificateInfo > CreateFakeCertificateAndInfoFromDers( > This is in the test, so the copying overhead doesn't matter, but I still wince > when returning large structs by value. Can we return a std::unique_ptr instead? > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > webrtc/api/rtcstatscollector_unittest.cc:109: EXPECT_CALL(pc_, > session()).WillRepeatedly(Return(&session_)); > This number of calls installed with EXPECT_CALL indicates that you're not really > using a mock, you're using a fake created via gmock. > > Are you sure this is the right tool for the job? > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > webrtc/api/rtcstatscollector_unittest.cc:150: bool > session_GetTransportStats(SessionStats* stats) { > This is a butt-ugly name for a function. Can you please use a standard-case name > for it? > > GetTransportStatsFromSession might be a better one. > > As is, this function accesses transport_certificates_, not session_, so the name > is misleading too. <a href="https://en.wikipedia.org">wiki</a>
On 2016/10/03 09:07:30, ThomasJones wrote: > On 2016/09/29 13:22:58, hta-webrtc wrote: > > Code looks reasonable (but needs argument swapping to conform to C++ style > > guide). > > > > I'm not that happy with the test code. > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > File webrtc/api/rtcstatscollector.h (right): > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector.h:79: void ProduceCertificateStats_s( > > Why the _s suffix? (here and elsewhere) > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector.h:81: const SessionStats& session_stats) const; > > Order of arguments: First inputs, then outputs. > > > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > File webrtc/api/rtcstatscollector_unittest.cc (right): > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:51: CertificateInfo > > CreateFakeCertificateAndInfoFromDers( > > This is in the test, so the copying overhead doesn't matter, but I still wince > > when returning large structs by value. Can we return a std::unique_ptr > instead? > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:109: EXPECT_CALL(pc_, > > session()).WillRepeatedly(Return(&session_)); > > This number of calls installed with EXPECT_CALL indicates that you're not > really > > using a mock, you're using a fake created via gmock. > > > > Are you sure this is the right tool for the job? > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:150: bool > > session_GetTransportStats(SessionStats* stats) { > > This is a butt-ugly name for a function. Can you please use a standard-case > name > > for it? > > > > GetTransportStatsFromSession might be a better one. > > > > As is, this function accesses transport_certificates_, not session_, so the > name > > is misleading too. > > > <a href="https://en.wikipedia.org">wiki</a> https://en.wikipedia.org
On 2016/10/03 09:07:30, ThomasJones wrote: > On 2016/09/29 13:22:58, hta-webrtc wrote: > > Code looks reasonable (but needs argument swapping to conform to C++ style > > guide). > > > > I'm not that happy with the test code. > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > File webrtc/api/rtcstatscollector.h (right): > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector.h:79: void ProduceCertificateStats_s( > > Why the _s suffix? (here and elsewhere) > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector.h:81: const SessionStats& session_stats) const; > > Order of arguments: First inputs, then outputs. > > > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > File webrtc/api/rtcstatscollector_unittest.cc (right): > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:51: CertificateInfo > > CreateFakeCertificateAndInfoFromDers( > > This is in the test, so the copying overhead doesn't matter, but I still wince > > when returning large structs by value. Can we return a std::unique_ptr > instead? > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:109: EXPECT_CALL(pc_, > > session()).WillRepeatedly(Return(&session_)); > > This number of calls installed with EXPECT_CALL indicates that you're not > really > > using a mock, you're using a fake created via gmock. > > > > Are you sure this is the right tool for the job? > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:150: bool > > session_GetTransportStats(SessionStats* stats) { > > This is a butt-ugly name for a function. Can you please use a standard-case > name > > for it? > > > > GetTransportStatsFromSession might be a better one. > > > > As is, this function accesses transport_certificates_, not session_, so the > name > > is misleading too. > > > <a href="https://en.wikipedia.org">wiki</a> https://en.wikipedia.org
On 2016/10/03 09:07:30, ThomasJones wrote: > On 2016/09/29 13:22:58, hta-webrtc wrote: > > Code looks reasonable (but needs argument swapping to conform to C++ style > > guide). > > > > I'm not that happy with the test code. > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > File webrtc/api/rtcstatscollector.h (right): > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector.h:79: void ProduceCertificateStats_s( > > Why the _s suffix? (here and elsewhere) > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector.h:81: const SessionStats& session_stats) const; > > Order of arguments: First inputs, then outputs. > > > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > File webrtc/api/rtcstatscollector_unittest.cc (right): > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:51: CertificateInfo > > CreateFakeCertificateAndInfoFromDers( > > This is in the test, so the copying overhead doesn't matter, but I still wince > > when returning large structs by value. Can we return a std::unique_ptr > instead? > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:109: EXPECT_CALL(pc_, > > session()).WillRepeatedly(Return(&session_)); > > This number of calls installed with EXPECT_CALL indicates that you're not > really > > using a mock, you're using a fake created via gmock. > > > > Are you sure this is the right tool for the job? > > > > > https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... > > webrtc/api/rtcstatscollector_unittest.cc:150: bool > > session_GetTransportStats(SessionStats* stats) { > > This is a butt-ugly name for a function. Can you please use a standard-case > name > > for it? > > > > GetTransportStatsFromSession might be a better one. > > > > As is, this function accesses transport_certificates_, not session_, so the > name > > is misleading too. > > > <a href="https://en.wikipedia.org">wiki</a> https://en.wikipedia.org
PTAL hta. https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.h:79: void ProduceCertificateStats_s( On 2016/09/29 13:22:58, hta-webrtc wrote: > Why the _s suffix? (here and elsewhere) _s for stuff to be executed on the signaling thread, _w for worker thread and _n for network thread. So far everything is on the signaling thread, but with some refactoring of classes used to get the stats I think we can spread the work and avoid several synchronous thread Invokes (lower priority than adding stats atm). https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.h:81: const SessionStats& session_stats) const; On 2016/09/29 13:22:58, hta-webrtc wrote: > Order of arguments: First inputs, then outputs. > > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering Done. https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:51: CertificateInfo CreateFakeCertificateAndInfoFromDers( On 2016/09/29 13:22:58, hta-webrtc wrote: > This is in the test, so the copying overhead doesn't matter, but I still wince > when returning large structs by value. Can we return a std::unique_ptr instead? Done. https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:109: EXPECT_CALL(pc_, session()).WillRepeatedly(Return(&session_)); On 2016/09/29 13:22:58, hta-webrtc wrote: > This number of calls installed with EXPECT_CALL indicates that you're not really > using a mock, you're using a fake created via gmock. > > Are you sure this is the right tool for the job? You're right. I changed it so that only the certificate-related tests do EXPECT_CALL, mocking it to return just what that test expects, instead of a "faking" type of mock setup. https://codereview.webrtc.org/2243123002/diff/140001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:150: bool session_GetTransportStats(SessionStats* stats) { On 2016/09/29 13:22:58, hta-webrtc wrote: > This is a butt-ugly name for a function. Can you please use a standard-case name > for it? > > GetTransportStatsFromSession might be a better one. > > As is, this function accesses transport_certificates_, not session_, so the name > is misleading too. Functions removed.
lgtm modulo some style comments. https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.h:83: int64_t timestamp_us, const rtc::SSLCertificate* certificate, Any particular reason this isn't a reference? Can it be null? (styleguide again) I see that the local cert is formed by &-ing a value, but the remote cert is passed from a pointer - but it's within a null-check, so "should be safe". https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:112: // Default return values for mocks. Move comment before all the EXPECT_CALL lines? https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:394: local = CreateFakeCertificateAndInfoFromDers(local->ders); Here you are creating an unique_ptr with a generic name (local), manipulating it, and then overwriting it based on itself. I think it would be more readable if you called it local_certificate and just wrote to it once, and called the temp that you currently store in |local| temp_certificate. Similarly for remote below. You repeat this pattern a number of times, so perhaps a helper function is indicated? https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:477: ExpectReportContainsCertificateInfo(report, *video_remote.get()); Good rename. More readable.
https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.h:83: int64_t timestamp_us, const rtc::SSLCertificate* certificate, On 2016/10/03 14:51:36, hta-webrtc wrote: > Any particular reason this isn't a reference? Can it be null? (styleguide again) > > I see that the local cert is formed by &-ing a value, but the remote cert is > passed from a pointer - but it's within a null-check, so "should be safe". Mistake. Made it const&. https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:112: // Default return values for mocks. On 2016/10/03 14:51:36, hta-webrtc wrote: > Move comment before all the EXPECT_CALL lines? Done. https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:394: local = CreateFakeCertificateAndInfoFromDers(local->ders); On 2016/10/03 14:51:36, hta-webrtc wrote: > Here you are creating an unique_ptr with a generic name (local), manipulating > it, and then overwriting it based on itself. > I think it would be more readable if you called it local_certificate and just > wrote to it once, and called the temp that you currently store in |local| > temp_certificate. Similarly for remote below. > > You repeat this pattern a number of times, so perhaps a helper function is > indicated? Constructing using a vector instead. https://codereview.webrtc.org/2243123002/diff/160001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:477: ExpectReportContainsCertificateInfo(report, *video_remote.get()); On 2016/10/03 14:51:36, hta-webrtc wrote: > Good rename. More readable. Acknowledged.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/2243123002/#ps180001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8903)
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by hbos@webrtc.org
Description was changed from ========== RTCCertificateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#certificatestats-dict* BUG=chromium:627816, chromium:629436 ========== to ========== RTCCertificateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#certificatestats-dict* BUG=chromium:627816, chromium:629436 NOTRY=True ==========
On 2016/10/03 19:58:43, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... It's all green, just android_arm64_rel that is not running and will timeout, landing with NOTRY instead.
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== RTCCertificateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#certificatestats-dict* BUG=chromium:627816, chromium:629436 NOTRY=True ========== to ========== RTCCertificateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#certificatestats-dict* BUG=chromium:627816, chromium:629436 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== RTCCertificateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#certificatestats-dict* BUG=chromium:627816, chromium:629436 NOTRY=True ========== to ========== RTCCertificateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#certificatestats-dict* BUG=chromium:627816, chromium:629436 NOTRY=True Committed: https://crrev.com/6ab97ce0b9564e31c84ea0dbf72e347499d13aa1 Cr-Commit-Position: refs/heads/master@{#14484} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6ab97ce0b9564e31c84ea0dbf72e347499d13aa1 Cr-Commit-Position: refs/heads/master@{#14484} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
