Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2408363002:
RTCTransportStats added. (Closed)
DescriptionRTCTransportStats[1] added, supporting all members.
Address TODO in rtcstatscollector_unittest.cc before closing 653873.
[1] https://w3c.github.io/webrtc-stats/#transportstats-dict*
BUG=chromium:653873, chromium:633550, chromium:627816
Committed: https://crrev.com/2fa7c67675c1ad0b4a2a04eec152c4657798485a
Cr-Commit-Position: refs/heads/master@{#14740}
Patch Set 1 #
Total comments: 11
Patch Set 2 : PrepareCertificateStatsMap #
Total comments: 16
Patch Set 3 : Addressed comments, RTP and RTCP #
Total comments: 7
Patch Set 4 : nits and rebase with master #
Messages
Total messages: 34 (20 generated)
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/v2/patch-status/codereview.webrtc.org/...
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look, hta and deadbeef.
Description was changed from ========== RTCTransportStats[1] added. Note: rtcp_transport_stats_id is not defined, it needs to be defined before closing crbug.com/653873. [1] // https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ========== to ========== RTCTransportStats[1] added. Note: rtcp_transport_stats_id is not defined, it needs to be defined before closing crbug.com/653873. [1] // https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
Description was changed from ========== RTCTransportStats[1] added. Note: rtcp_transport_stats_id is not defined, it needs to be defined before closing crbug.com/653873. [1] // https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ========== to ========== RTCTransportStats[1] added. Note: rtcp_transport_stats_id is not defined, it needs to be defined before closing crbug.com/653873. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ==========
Not happy with the structure of this one. Using the Collect*Stats functions do double duty makes both duties harder to read. Can you find a cleaner way? https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:192: RTCStatsCollector::ProduceCertificateStats_s( I don't like this function. It's doing two things, one of which is like all the other functions named like this, the other one isn't. Can we break it up into an internal function "map = PrepareCertificateStatsMap()" and ProduceCertificateStats_s(map) so that the two functions are clearly separated? https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:224: RTCStatsCollector::ProduceCertificateStatsFromSSLCertificateAndChain_s( This one too is now dual-function. https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.h File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.h:91: RTCStatsReport* report) const; This is now doing more than producing certificate stats. Please add a comment about it. https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector_... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector_... webrtc/api/rtcstatscollector_unittest.cc:862: "42.42.42.42", 42, "protocol", cricket::LOCAL_PORT_TYPE, 42); Curious: Can a remote cnadidate really have cricket::LOCAL_PORT_TYPE? https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/stats/rtcstats_obj... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/stats/rtcstats_obj... webrtc/api/stats/rtcstats_objects.h:94: // TODO(hbos): Tracking bug crbug.com/632723 Complete sentence: TODO(hbos): Finish implementation. Tracking bug...
Patchset #2 (id:20001) has been deleted
PTAL hta, deadebeef. https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:192: RTCStatsCollector::ProduceCertificateStats_s( On 2016/10/12 22:52:35, hta-webrtc wrote: > I don't like this function. It's doing two things, one of which is like all the > other functions named like this, the other one isn't. > > Can we break it up into an internal function "map = > PrepareCertificateStatsMap()" and ProduceCertificateStats_s(map) so that the two > functions are clearly separated? Yes, that's much nicer. Done. https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:224: RTCStatsCollector::ProduceCertificateStatsFromSSLCertificateAndChain_s( On 2016/10/12 22:52:35, hta-webrtc wrote: > This one too is now dual-function. Done. https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.h File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.h:91: RTCStatsReport* report) const; On 2016/10/12 22:52:35, hta-webrtc wrote: > This is now doing more than producing certificate stats. Please add a comment > about it. Code changed to not do two things. https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector_... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector_... webrtc/api/rtcstatscollector_unittest.cc:862: "42.42.42.42", 42, "protocol", cricket::LOCAL_PORT_TYPE, 42); On 2016/10/12 22:52:35, hta-webrtc wrote: > Curious: Can a remote cnadidate really have cricket::LOCAL_PORT_TYPE? Good question. There plenty of TODOs in our codebase that LOCAL_PORT_TYPE should be replaced by "host" (RTCIceCandidateType) and it is translated as such here and in other places, so treating it as such makes sense in these unittests. (Though in this test it does not matter what type I use.) As for cricket code and LOCAL_PORT_TYPE usage I'm not sure. https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/stats/rtcstats_obj... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/stats/rtcstats_obj... webrtc/api/stats/rtcstats_objects.h:94: // TODO(hbos): Tracking bug crbug.com/632723 On 2016/10/12 22:52:35, hta-webrtc wrote: > Complete sentence: TODO(hbos): Finish implementation. Tracking bug... Done.
https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector_... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2408363002/diff/1/webrtc/api/rtcstatscollector_... webrtc/api/rtcstatscollector_unittest.cc:862: "42.42.42.42", 42, "protocol", cricket::LOCAL_PORT_TYPE, 42); On 2016/10/17 19:33:13, hbos wrote: > On 2016/10/12 22:52:35, hta-webrtc wrote: > > Curious: Can a remote cnadidate really have cricket::LOCAL_PORT_TYPE? > > Good question. There plenty of TODOs in our codebase that LOCAL_PORT_TYPE should > be replaced by "host" (RTCIceCandidateType) and it is translated as such here > and in other places, so treating it as such makes sense in these unittests. > (Though in this test it does not matter what type I use.) > As for cricket code and LOCAL_PORT_TYPE usage I'm not sure. Yes, LOCAL_PORT_TYPE means "host" candidate. So it's not using the same definition of local/remote. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:27: std::string RTCCertificateID(const std::string& fingerprint) { nit: "RTCCertificateID" sounds more like a class name than a function name, to me. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:203: for (const auto& transport_stats : session_stats.transport_stats) { nit: Instead of iterating the transport stats and finding certificate stats with a matching name, would it be possible to directly iterate certificate_stats_map? https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:360: for (const auto& transport : session_stats.transport_stats) { Actually, if I understand the spec correctly, there should be a "transport stats" for each "channel" (for our definition of channel). The ID would be a combination of the transport name and component (RTP or RTCP). I realize this is confusing based on the wording in the spec, so I filed an issue: https://github.com/w3c/webrtc-stats/issues/72 https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:380: // crbug.com/653873 There's no real reason why this couldn't be done in the current CL. You could do something like the following: // Get reference to RTCP channel, if it exists. auto rtcp_channel = std::find_if(channel_stats.begin(), channel_stats.end(), [] (const TransportChannelStats& ch) { return ch.component == COMPONENT_RTCP; }); // Later, while iterating channels... // If there are separate RTP and RTCP channels (due to RTCP mux being // disabled), link the two via rtcp_transport_stats_id. if (channel.component == COMPONENT_RTP && rtcp_channel != channel_stats.end()) { transport_stats->rtcp_transport_stats_id = MakeRTCTransportStatsID(*rtcp_channel); } https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:448: EXPECT_TRUE(stats); Won't the test crash if |stats| isn't found? Seems appropriate to use ASSERT_TRUE. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:462: } I won't ask you to change everything, since it's so far along, but a general comment about the test methodology: I think it's better if tests minimize the amount of code used to determine the expected output. Here we have "Expect" functions here that essentially convert "cricket::" stats to stats reports, which is the same thing that the code under test is doing. So the unit tests verify that the two conversions produce the same result, but both conversions could be wrong without it being obvious (especially if test code was produced by copy/pasting implementation code). So an ideal unit test, to me, would have hard-coded RTCStatsReports that it expects to see, and each of these "ExpectContains" methods would simply look up the report by ID and do an EXPECT_EQ. This also has the advantage that one can look at a unit test and easily see the expected output for a given input. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:886: b_transport_channel_stats.connection_infos.push_back(b_connection_info); One of these channel stats should be component RTCP. There will never be two channels under a transport with the same component (which is a good argument for making "channel_stats" a map, but it's probably a little late for that).
Patchset #3 (id:60001) has been deleted
PTAL deadbeef https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:27: std::string RTCCertificateID(const std::string& fingerprint) { On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > nit: "RTCCertificateID" sounds more like a class name than a function name, to > me. Done. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:203: for (const auto& transport_stats : session_stats.transport_stats) { On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > nit: Instead of iterating the transport stats and finding certificate stats with > a matching name, would it be possible to directly iterate certificate_stats_map? Oh yeah, done. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:360: for (const auto& transport : session_stats.transport_stats) { On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > Actually, if I understand the spec correctly, there should be a "transport > stats" for each "channel" (for our definition of channel). The ID would be a > combination of the transport name and component (RTP or RTCP). > > I realize this is confusing based on the wording in the spec, so I filed an > issue: https://github.com/w3c/webrtc-stats/issues/72 Thanks. Code updated. If this is the case then RTCTransportStats for both the RTP and RTCP case have the same local and remote certificates. Is it the case that, for each transport, we always have exactly 1 RTCTransportStats for the RTP case, and 0 or 1 RTCTransportStats for the RTCP case? Or could you have 0, 1 or more for RTP? https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:380: // crbug.com/653873 On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > There's no real reason why this couldn't be done in the current CL. You could do > something like the following: > > // Get reference to RTCP channel, if it exists. > auto rtcp_channel = std::find_if(channel_stats.begin(), channel_stats.end(), > [] (const TransportChannelStats& ch) { > return ch.component == COMPONENT_RTCP; > }); > > // Later, while iterating channels... > > // If there are separate RTP and RTCP channels (due to RTCP mux being > // disabled), link the two via rtcp_transport_stats_id. > if (channel.component == COMPONENT_RTP && rtcp_channel != channel_stats.end()) { > transport_stats->rtcp_transport_stats_id = > MakeRTCTransportStatsID(*rtcp_channel); > } Done. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:448: EXPECT_TRUE(stats); On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > Won't the test crash if |stats| isn't found? Seems appropriate to use > ASSERT_TRUE. Done. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:462: } On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > I won't ask you to change everything, since it's so far along, but a general > comment about the test methodology: I think it's better if tests minimize the > amount of code used to determine the expected output. > > Here we have "Expect" functions here that essentially convert "cricket::" stats > to stats reports, which is the same thing that the code under test is doing. So > the unit tests verify that the two conversions produce the same result, but both > conversions could be wrong without it being obvious (especially if test code was > produced by copy/pasting implementation code). So an ideal unit test, to me, > would have hard-coded RTCStatsReports that it expects to see, and each of these > "ExpectContains" methods would simply look up the report by ID and do an > EXPECT_EQ. > > This also has the advantage that one can look at a unit test and easily see the > expected output for a given input. I think that is a good point. There's no operator== for RTCStats objects, how about I add it in a follow-up and do EXPECT_EQ with reports hard-coded in the unittests, removing the ExpectReportContainsBlahBlah functions? But doing so separately. https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:886: b_transport_channel_stats.connection_infos.push_back(b_connection_info); On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > One of these channel stats should be component RTCP. There will never be two > channels under a transport with the same component (which is a good argument for > making "channel_stats" a map, but it's probably a little late for that). Acknowledged.
lgtm https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:360: for (const auto& transport : session_stats.transport_stats) { On 2016/10/19 16:30:00, hbos wrote: > On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > > Actually, if I understand the spec correctly, there should be a "transport > > stats" for each "channel" (for our definition of channel). The ID would be a > > combination of the transport name and component (RTP or RTCP). > > > > I realize this is confusing based on the wording in the spec, so I filed an > > issue: https://github.com/w3c/webrtc-stats/issues/72 > > Thanks. Code updated. > > If this is the case then RTCTransportStats for both the RTP and RTCP case have > the same local and remote certificates. > Is it the case that, for each transport, we always have exactly 1 > RTCTransportStats for the RTP case, and 0 or 1 RTCTransportStats for the RTCP > case? Or could you have 0, 1 or more for RTP? You will always have 1 for RTP, and 0 or 1 for RTCP (depending on whether or not RTCP mux was negotiated, causing RTP and RTCP to use the same transport). https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2408363002/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:462: } On 2016/10/19 16:30:00, hbos wrote: > On 2016/10/17 21:19:03, Taylor Brandstetter wrote: > > I won't ask you to change everything, since it's so far along, but a general > > comment about the test methodology: I think it's better if tests minimize the > > amount of code used to determine the expected output. > > > > Here we have "Expect" functions here that essentially convert "cricket::" > stats > > to stats reports, which is the same thing that the code under test is doing. > So > > the unit tests verify that the two conversions produce the same result, but > both > > conversions could be wrong without it being obvious (especially if test code > was > > produced by copy/pasting implementation code). So an ideal unit test, to me, > > would have hard-coded RTCStatsReports that it expects to see, and each of > these > > "ExpectContains" methods would simply look up the report by ID and do an > > EXPECT_EQ. > > > > This also has the advantage that one can look at a unit test and easily see > the > > expected output for a given input. > > I think that is a good point. > > There's no operator== for RTCStats objects, how about I add it in a follow-up > and do EXPECT_EQ with reports hard-coded in the unittests, removing the > ExpectReportContainsBlahBlah functions? But doing so separately. Sounds good. Though if that seems like more effort than it's worth, I'd understand, I leave it to your judgement. https://codereview.webrtc.org/2408363002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2408363002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:912: // Get stats with RTCP and without an active connection or certificates. nit: For consistency, should this comment go above "ExpectReportContainsTransportStats"?
PTAL hta
Description was changed from ========== RTCTransportStats[1] added. Note: rtcp_transport_stats_id is not defined, it needs to be defined before closing crbug.com/653873. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ========== to ========== RTCTransportStats[1] added. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ==========
Description was changed from ========== RTCTransportStats[1] added. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ========== to ========== RTCTransportStats[1] added, supporting all members. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ==========
Description was changed from ========== RTCTransportStats[1] added, supporting all members. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550 ========== to ========== RTCTransportStats[1] added, supporting all members. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550, chromium:627816 ==========
lgtm, only nits. https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... File webrtc/api/rtcstatscollector.h (right): https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... webrtc/api/rtcstatscollector.h:112: // Helper functions to stats producing functions. Nit: Hyphenate stats-producing. Hyper-nit: Only one function - so functions -> function. https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... webrtc/api/rtcstatscollector_unittest.cc:478: best_connection_info->remote_candidate.id()); I'm a bit skeptical of this method of testing that we have the right pair, since we're testing something (the ID generation method) that isn't part of the documented API. I would prefer a test that looks up the candidate pair in the stats based on the transport_stats.selected_candidate_pair_id (which we do on the next line) and checks that the IP addresses in that stats record are equal to the IP addresses of the best_connection_info data. But I'm OK with leaving that as a TODO for now. https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... webrtc/api/rtcstatscollector_unittest.cc:912: // Get stats with RTCP and without an active connection or certificates. On 2016/10/19 19:03:07, Taylor Brandstetter wrote: > nit: For consistency, should this comment go above > "ExpectReportContainsTransportStats"? Agree that comment seems misplaced. This stuff sets up a new set of connection data, and it's not clear which Expect* calls are using this particular setup.
Description was changed from ========== RTCTransportStats[1] added, supporting all members. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550, chromium:627816 ========== to ========== RTCTransportStats[1] added, supporting all members. Address TODO in rtcstatscollector_unittest.cc before closing 653873. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550, chromium:627816 ==========
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.chromium.org/2408363002/#ps100001 (title: "nits and rebase with master")
https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... File webrtc/api/rtcstatscollector.h (right): https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... webrtc/api/rtcstatscollector.h:112: // Helper functions to stats producing functions. On 2016/10/21 08:37:51, hta-webrtc wrote: > Nit: Hyphenate stats-producing. > Hyper-nit: Only one function - so functions -> function. > Done. https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... webrtc/api/rtcstatscollector_unittest.cc:478: best_connection_info->remote_candidate.id()); On 2016/10/21 08:37:51, hta-webrtc wrote: > I'm a bit skeptical of this method of testing that we have the right pair, since > we're testing something (the ID generation method) that isn't part of the > documented API. > > I would prefer a test that looks up the candidate pair in the stats based on the > transport_stats.selected_candidate_pair_id (which we do on the next line) and > checks that the IP addresses in that stats record are equal to the IP addresses > of the best_connection_info data. > > But I'm OK with leaving that as a TODO for now. Added TODO. https://codereview.chromium.org/2408363002/diff/80001/webrtc/api/rtcstatscoll... webrtc/api/rtcstatscollector_unittest.cc:912: // Get stats with RTCP and without an active connection or certificates. On 2016/10/21 08:37:51, hta-webrtc wrote: > On 2016/10/19 19:03:07, Taylor Brandstetter wrote: > > nit: For consistency, should this comment go above > > "ExpectReportContainsTransportStats"? > > Agree that comment seems misplaced. This stuff sets up a new set of connection > data, and it's not clear which Expect* calls are using this particular setup. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
hbos@webrtc.org changed reviewers: - hbos@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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 ========== RTCTransportStats[1] added, supporting all members. Address TODO in rtcstatscollector_unittest.cc before closing 653873. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550, chromium:627816 ========== to ========== RTCTransportStats[1] added, supporting all members. Address TODO in rtcstatscollector_unittest.cc before closing 653873. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550, chromium:627816 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== RTCTransportStats[1] added, supporting all members. Address TODO in rtcstatscollector_unittest.cc before closing 653873. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550, chromium:627816 ========== to ========== RTCTransportStats[1] added, supporting all members. Address TODO in rtcstatscollector_unittest.cc before closing 653873. [1] https://w3c.github.io/webrtc-stats/#transportstats-dict* BUG=chromium:653873, chromium:633550, chromium:627816 Committed: https://crrev.com/2fa7c67675c1ad0b4a2a04eec152c4657798485a Cr-Commit-Position: refs/heads/master@{#14740} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2fa7c67675c1ad0b4a2a04eec152c4657798485a Cr-Commit-Position: refs/heads/master@{#14740} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
