Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2384143002:
RTCIceCandidateStats added. (Closed)
DescriptionRTCIceCandidateStats[1] added.
The RTCStatsCollector collects candidates from candidate pairs. Note
that there may be other candidates that are not paired with anything,
stats for these should also be produced before closing crbug.com/632723.
[1] https://w3c.github.io/webrtc-stats/#icecandidate-dict*
BUG=chromium:627816, chromium:632723
Committed: https://crrev.com/ab9f6e4dea4fa48f415e7c589209718918f8e3dd
Cr-Commit-Position: refs/heads/master@{#14565}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed comments #
Total comments: 1
Patch Set 3 : Rebase with master #
Total comments: 10
Patch Set 4 : Addressed comments #
Messages
Total messages: 28 (16 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== RTCIceCandidateStats added. BUG= ========== to ========== RTCIceCandidateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#icecandidate-dict* BUG=chromium:627816, chromium:632723 ==========
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
hbos@webrtc.org changed reviewers: + hta@webrtc.org
Please take a look, hta
Looks reasonable, but the style comments got to be many enough that I'm not LGTMing it yet. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:228: // TODO(hbos): RTCIceCandidatePairStats referencing the resulting pair. Complete sentence: "TODO(hbos): Produce RTCIceCandidatePairStats..." https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:243: std::string id = "RTCIceCandidate_" + candidate.id(); This uses the same namespace for local and remote candidates. Is that a problem? (If the id is locally assigned, it isn't.) Note - it's by design that you can't tell whether a candidate is local or remote; you need the candidate pair stats to tell you that. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:244: const RTCStats* s = report->Get(id); Bad variable name. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: } What's the "else"? Is it an error to have duplicate IDs, or did we just encounter the same candidate in two different contexts? https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.h:88: // RTCIceCandidateStats Try to make comments full sentences. If the comment says no more than the function name, consider omitting it. "This function produces both RTCIceCandidateStats and RTCIceCandidatePairStats." is a fine summary. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.h:120: const char* CandidateTypeToRTCIceCandidateType(const std::string& type); If this function is only exposed for testing, add a comment saying so. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:590: "remote_stflx's protocol", nit: remote_srflx (server reflexive). https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:618: &b_local, &b_remote](SessionStats* stats) { Isn't it simpler to construct the SessionStats before the call, and copy the object? https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/stats/rtcstats... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/stats/rtcstats... webrtc/api/stats/rtcstats_objects.h:63: class RTCRemoteIceCandidateStats final : public RTCIceCandidateStats { Blank line between top level constructs (such as classes).
Addressed comments! But I think I should add another unittest that tests the case when you encounter the same candidates multiple times, including in both local and remote contexts. If that is something that should be possible for candidates? Otherwise I should just DCHECK. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:228: // TODO(hbos): RTCIceCandidatePairStats referencing the resulting pair. On 2016/10/04 14:12:45, hta-webrtc wrote: > Complete sentence: "TODO(hbos): Produce RTCIceCandidatePairStats..." Done. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:243: std::string id = "RTCIceCandidate_" + candidate.id(); On 2016/10/04 14:12:45, hta-webrtc wrote: > This uses the same namespace for local and remote candidates. Is that a problem? > (If the id is locally assigned, it isn't.) > > Note - it's by design that you can't tell whether a candidate is local or > remote; you need the candidate pair stats to tell you that. > Since the ID should be based on the object that produced it - a candidate - and by design you can't tell whether a candidate is local or remote - I think it makes sense that the ID is just "RTCIceCandidate_X". If, however, a candidate can be both local and remote depending on the context, then the ID needs to be different to avoid a name clash. This may very well be the case, I don't know? I updated the code to handle this, but if a candidate can't be both local and remote I'd rather DCHECK. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:244: const RTCStats* s = report->Get(id); On 2016/10/04 14:12:45, hta-webrtc wrote: > Bad variable name. Done. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: } On 2016/10/04 14:12:45, hta-webrtc wrote: > What's the "else"? Is it an error to have duplicate IDs, or did we just > encounter the same candidate in two different contexts? It means we encountered the same candidate twice, no need to duplicate its stats since it's identical. NOTE: I'm assuming candidate.id() is a unique identifier. Perhaps we should add a second reviewer, someone who knows about candidates, their IDs and whether or not the same candidate can be encountered multiple times and if they can be both local and remote at the same time in different contexts. If so, we should have another unittest for this. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.h:88: // RTCIceCandidateStats On 2016/10/04 14:12:45, hta-webrtc wrote: > Try to make comments full sentences. If the comment says no more than the > function name, consider omitting it. > > "This function produces both RTCIceCandidateStats and RTCIceCandidatePairStats." > is a fine summary. Done. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.h:120: const char* CandidateTypeToRTCIceCandidateType(const std::string& type); On 2016/10/04 14:12:45, hta-webrtc wrote: > If this function is only exposed for testing, add a comment saying so. Done. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:590: "remote_stflx's protocol", On 2016/10/04 14:12:46, hta-webrtc wrote: > nit: remote_srflx (server reflexive). Done. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:618: &b_local, &b_remote](SessionStats* stats) { On 2016/10/04 14:12:46, hta-webrtc wrote: > Isn't it simpler to construct the SessionStats before the call, and copy the > object? Done. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/stats/rtcstats... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/stats/rtcstats... webrtc/api/stats/rtcstats_objects.h:63: class RTCRemoteIceCandidateStats final : public RTCIceCandidateStats { On 2016/10/04 14:12:46, hta-webrtc wrote: > Blank line between top level constructs (such as classes). Done.
Now looks good to me, but agree that having someone who knows the ICE candidate structures look over it is useful. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:243: std::string id = "RTCIceCandidate_" + candidate.id(); On 2016/10/05 10:16:30, hbos wrote: > On 2016/10/04 14:12:45, hta-webrtc wrote: > > This uses the same namespace for local and remote candidates. Is that a > problem? > > (If the id is locally assigned, it isn't.) > > > > Note - it's by design that you can't tell whether a candidate is local or > > remote; you need the candidate pair stats to tell you that. > > > > Since the ID should be based on the object that produced it - a candidate - and > by design you can't tell whether a candidate is local or remote - I think it > makes sense that the ID is just "RTCIceCandidate_X". > > If, however, a candidate can be both local and remote depending on the context, > then the ID needs to be different to avoid a name clash. This may very well be > the case, I don't know? I updated the code to handle this, but if a candidate > can't be both local and remote I'd rather DCHECK. DCHECK seems good. What I'm not sure about is where the candidate ID comes from, so I can't check that it's actually designed to be unique. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: } On 2016/10/05 10:16:30, hbos wrote: > On 2016/10/04 14:12:45, hta-webrtc wrote: > > What's the "else"? Is it an error to have duplicate IDs, or did we just > > encounter the same candidate in two different contexts? > > It means we encountered the same candidate twice, no need to duplicate its stats > since it's identical. > NOTE: I'm assuming candidate.id() is a unique identifier. > > Perhaps we should add a second reviewer, someone who knows about candidates, > their IDs and whether or not the same candidate can be encountered multiple > times and if they can be both local and remote at the same time in different > contexts. > > If so, we should have another unittest for this. Adding a reviewer who knows the cricket::Candidate structure seems like a good idea. Please do. deadbeef, honghaiz and pthatcher seem to have been recent editors to the .h file. https://codereview.webrtc.org/2384143002/diff/90001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2384143002/diff/90001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:646: [this, &session_stats](SessionStats* stats) { Does the lambda actually touch "this"?
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org
Please take a look, deadbeef. Especially the discussions where i replied +deadbeef. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:243: std::string id = "RTCIceCandidate_" + candidate.id(); On 2016/10/05 11:17:14, hta-webrtc wrote: > On 2016/10/05 10:16:30, hbos wrote: > > On 2016/10/04 14:12:45, hta-webrtc wrote: > > > This uses the same namespace for local and remote candidates. Is that a > > problem? > > > (If the id is locally assigned, it isn't.) > > > > > > Note - it's by design that you can't tell whether a candidate is local or > > > remote; you need the candidate pair stats to tell you that. > > > > > > > Since the ID should be based on the object that produced it - a candidate - > and > > by design you can't tell whether a candidate is local or remote - I think it > > makes sense that the ID is just "RTCIceCandidate_X". > > > > If, however, a candidate can be both local and remote depending on the > context, > > then the ID needs to be different to avoid a name clash. This may very well be > > the case, I don't know? I updated the code to handle this, but if a candidate > > can't be both local and remote I'd rather DCHECK. > > DCHECK seems good. What I'm not sure about is where the candidate ID comes from, > so I can't check that it's actually designed to be unique. +deadbeef https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: } On 2016/10/05 11:17:14, hta-webrtc wrote: > On 2016/10/05 10:16:30, hbos wrote: > > On 2016/10/04 14:12:45, hta-webrtc wrote: > > > What's the "else"? Is it an error to have duplicate IDs, or did we just > > > encounter the same candidate in two different contexts? > > > > It means we encountered the same candidate twice, no need to duplicate its > stats > > since it's identical. > > NOTE: I'm assuming candidate.id() is a unique identifier. > > > > Perhaps we should add a second reviewer, someone who knows about candidates, > > their IDs and whether or not the same candidate can be encountered multiple > > times and if they can be both local and remote at the same time in different > > contexts. > > > > If so, we should have another unittest for this. > > Adding a reviewer who knows the cricket::Candidate structure seems like a good > idea. Please do. deadbeef, honghaiz and pthatcher seem to have been recent > editors to the .h file. +deadbeef
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm waiting for deadbeef. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:337: : "RTCRemoteIceCandidate_" + candidate.id()); Not something to fix now, but.... this tests a property of the interface (that you can guess the id of the entries) that isn't required by the specification. The specification places no requirements on the id; it's intended to be meaningless. Requiring meaning in meaningless fields is something we should normally avoid doing when we write tests. But as I said - not needed now.
lgtm, though see comments before submitting. https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: } On 2016/10/06 08:07:27, hbos wrote: > On 2016/10/05 11:17:14, hta-webrtc wrote: > > On 2016/10/05 10:16:30, hbos wrote: > > > On 2016/10/04 14:12:45, hta-webrtc wrote: > > > > What's the "else"? Is it an error to have duplicate IDs, or did we just > > > > encounter the same candidate in two different contexts? > > > > > > It means we encountered the same candidate twice, no need to duplicate its > > stats > > > since it's identical. > > > NOTE: I'm assuming candidate.id() is a unique identifier. > > > > > > Perhaps we should add a second reviewer, someone who knows about candidates, > > > their IDs and whether or not the same candidate can be encountered multiple > > > times and if they can be both local and remote at the same time in different > > > contexts. > > > > > > If so, we should have another unittest for this. > > > > Adding a reviewer who knows the cricket::Candidate structure seems like a good > > idea. Please do. deadbeef, honghaiz and pthatcher seem to have been recent > > editors to the .h file. > > +deadbeef The same candidate can't be both a local and remote candidate. And the ID *should* be unique. In practice it's just a random string with 48 bits of entropy, so collisions are possible. But this isn't the stats collector's fault; the stats collector should assume it's unique. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:227: for (const cricket::ConnectionInfo& info : channel.connection_infos) { It's not sufficient to make candidate stats from ConnectionInfos (which are effectively candidate pairs). The channel could have candidates that aren't paired with anything. Really we need to add individual candidate lists to TransportChannelStats, and implement forming the list in P2PTransportChannel. Which isn't completely trivial, since there aren't ready-built lists; local candidates come from Port objects, and prflx candidates (both local and remote) are only stored in candidate pairs. I'm fine with landing this CL as-is, but with a big TODO and documenting this on the bug and CL description. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:243: std::string id = is_local ? "RTCLocalIceCandidate_" + candidate.id() Instead of making a copy of the id and then std::move'ing it, why not store a const ref to the id and use the normal constructor for RTCIceCandidateStats? https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/stats/rtcstat... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/stats/rtcstat... webrtc/api/stats/rtcstats_objects.h:39: // TODO(hbos): Support enum types? "RTCStatsMember<RTCIceCandidateType>"? That seems like a good idea, since it's an enum in WebIDL. Even if it eventually ends up a string, that can happen further up the Chromium stack. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/stats/rtcstat... webrtc/api/stats/rtcstats_objects.h:53: class RTCLocalIceCandidateStats final : public RTCIceCandidateStats { Is this just to avoid storing the "type" string multiple times? If so it seems like a premature optimization. You could also go further and define "RTCLocalSrflxIceCandidateStats" with a static const candidate_type, etc. But I think it would be simpler for the stats collector to just set each field. That said, this is just my opinion, take it or leave it.
Description was changed from ========== RTCIceCandidateStats[1] added. [1] https://w3c.github.io/webrtc-stats/#icecandidate-dict* BUG=chromium:627816, chromium:632723 ========== to ========== RTCIceCandidateStats[1] added. The RTCStatsCollector collects candidates from candidate pairs. Note that there may be other candidates that are not paired with anything, stats for these should also be produced before closing crbug.com/632723. [1] https://w3c.github.io/webrtc-stats/#icecandidate-dict* BUG=chromium:627816, chromium:632723 ==========
https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: } On 2016/10/06 19:09:07, Taylor Brandstetter wrote: > On 2016/10/06 08:07:27, hbos wrote: > > On 2016/10/05 11:17:14, hta-webrtc wrote: > > > On 2016/10/05 10:16:30, hbos wrote: > > > > On 2016/10/04 14:12:45, hta-webrtc wrote: > > > > > What's the "else"? Is it an error to have duplicate IDs, or did we just > > > > > encounter the same candidate in two different contexts? > > > > > > > > It means we encountered the same candidate twice, no need to duplicate its > > > stats > > > > since it's identical. > > > > NOTE: I'm assuming candidate.id() is a unique identifier. > > > > > > > > Perhaps we should add a second reviewer, someone who knows about > candidates, > > > > their IDs and whether or not the same candidate can be encountered > multiple > > > > times and if they can be both local and remote at the same time in > different > > > > contexts. > > > > > > > > If so, we should have another unittest for this. > > > > > > Adding a reviewer who knows the cricket::Candidate structure seems like a > good > > > idea. Please do. deadbeef, honghaiz and pthatcher seem to have been recent > > > editors to the .h file. > > > > +deadbeef > > The same candidate can't be both a local and remote candidate. And the ID > *should* be unique. In practice it's just a random string with 48 bits of > entropy, so collisions are possible. But this isn't the stats collector's fault; > the stats collector should assume it's unique. Acknowledged. Changing stats ID back to "RTCIceCandidate_..." and DCHECKing localness. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:227: for (const cricket::ConnectionInfo& info : channel.connection_infos) { On 2016/10/06 19:09:07, Taylor Brandstetter wrote: > It's not sufficient to make candidate stats from ConnectionInfos (which are > effectively candidate pairs). The channel could have candidates that aren't > paired with anything. > > Really we need to add individual candidate lists to TransportChannelStats, and > implement forming the list in P2PTransportChannel. Which isn't completely > trivial, since there aren't ready-built lists; local candidates come from Port > objects, and prflx candidates (both local and remote) are only stored in > candidate pairs. > > I'm fine with landing this CL as-is, but with a big TODO and documenting this on > the bug and CL description. OK. Adding TODOs and documenting. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:243: std::string id = is_local ? "RTCLocalIceCandidate_" + candidate.id() On 2016/10/06 19:09:07, Taylor Brandstetter wrote: > Instead of making a copy of the id and then std::move'ing it, why not store a > const ref to the id and use the normal constructor for RTCIceCandidateStats? Done. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:337: : "RTCRemoteIceCandidate_" + candidate.id()); On 2016/10/06 12:53:26, hta-webrtc wrote: > Not something to fix now, but.... > this tests a property of the interface (that you can guess the id of the > entries) that isn't required by the specification. The specification places no > requirements on the id; it's intended to be meaningless. > > Requiring meaning in meaningless fields is something we should normally avoid > doing when we write tests. > > But as I said - not needed now. Changed back to RTCIceCandidate_X based on deadbeef's comment about a candidate not being able to be both. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/stats/rtcstat... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/stats/rtcstat... webrtc/api/stats/rtcstats_objects.h:39: // TODO(hbos): Support enum types? "RTCStatsMember<RTCIceCandidateType>"? On 2016/10/06 19:09:07, Taylor Brandstetter wrote: > That seems like a good idea, since it's an enum in WebIDL. Even if it eventually > ends up a string, that can happen further up the Chromium stack. Agreed. It's on my TODO-list but it turned out not to be as trivial as it sounds. https://codereview.webrtc.org/2384143002/diff/110001/webrtc/api/stats/rtcstat... webrtc/api/stats/rtcstats_objects.h:53: class RTCLocalIceCandidateStats final : public RTCIceCandidateStats { On 2016/10/06 19:09:07, Taylor Brandstetter wrote: > Is this just to avoid storing the "type" string multiple times? If so it seems > like a premature optimization. You could also go further and define > "RTCLocalSrflxIceCandidateStats" with a static const candidate_type, etc. But I > think it would be simpler for the stats collector to just set each field. > > That said, this is just my opinion, take it or leave it. Good point. The static variable used as a class identifier does not have to be the same as the RTCStatsType string. I used the same variable for both because I thought the mapping was unique in both directions and it seems to be in all cases except for here where two string values map to the same dictionary type. It should be addressed, but I won't do that in this CL.
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, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2384143002/#ps130001 (title: "Addressed comments")
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 ========== RTCIceCandidateStats[1] added. The RTCStatsCollector collects candidates from candidate pairs. Note that there may be other candidates that are not paired with anything, stats for these should also be produced before closing crbug.com/632723. [1] https://w3c.github.io/webrtc-stats/#icecandidate-dict* BUG=chromium:627816, chromium:632723 ========== to ========== RTCIceCandidateStats[1] added. The RTCStatsCollector collects candidates from candidate pairs. Note that there may be other candidates that are not paired with anything, stats for these should also be produced before closing crbug.com/632723. [1] https://w3c.github.io/webrtc-stats/#icecandidate-dict* BUG=chromium:627816, chromium:632723 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== RTCIceCandidateStats[1] added. The RTCStatsCollector collects candidates from candidate pairs. Note that there may be other candidates that are not paired with anything, stats for these should also be produced before closing crbug.com/632723. [1] https://w3c.github.io/webrtc-stats/#icecandidate-dict* BUG=chromium:627816, chromium:632723 ========== to ========== RTCIceCandidateStats[1] added. The RTCStatsCollector collects candidates from candidate pairs. Note that there may be other candidates that are not paired with anything, stats for these should also be produced before closing crbug.com/632723. [1] https://w3c.github.io/webrtc-stats/#icecandidate-dict* BUG=chromium:627816, chromium:632723 Committed: https://crrev.com/ab9f6e4dea4fa48f415e7c589209718918f8e3dd Cr-Commit-Position: refs/heads/master@{#14565} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ab9f6e4dea4fa48f415e7c589209718918f8e3dd Cr-Commit-Position: refs/heads/master@{#14565}
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
