Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2241093002:
RTCStats and RTCStatsReport added (webrtc/stats) (Closed)
|
Created:
4 years, 4 months ago by hbos Modified:
4 years ago Reviewers:
tommi, nisse-webrtc, hta-webrtc, AlexG, hta - Chromium, Taylor Brandstetter, kjellander_webrtc, pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRTCStats and RTCStatsReport added (webrtc/stats).
The old and new getStats are very different. This CL proposes rewriting
the new getStats from scratch with a bottom-up approach, starting with
the fundamental stats classes. This will allow cleaner and more
efficient code that is more aligned with the spec.
RTCStats and subclasses are the equivalent to RTCStats and RTCStats-
-derived dictionaries from the specs[1][2]. The dictionary members are
public member variables of type RTCStatsMember<T>, where T is one of the
supported types. All members derive from RTCStatsMemberInterface and
iteration of members is possible with RTCStats::Members().
The members are not stored in a map for performance and readability.
Type checking is supported with static class variables, kType.
Only the supported member types T are specialized and may be
instantiated, and sequences are supported with std::vector<...>. Type
checking is again supported with static class variables, kType.
RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id
to RTCStats-objects. RTCStatsReport is reference counted. It and its
contained stats may be destroyed on any thread. When the
RTCStatsCollector is added in a follow-up CL, it will return const
references to the RTCStatsReports. This means copies don't have to be
made for multiple stats observers or when jumping threads. In fact, no
copies of any stats will have to be made in surfacing stats to Blink.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary
[2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html
[3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object
This adds the new folder webrtc/stats/, with target rtc_stats and binary
rtc_stats_unittests. Public api headers are placed in webrtc/api/ and
.cc files are placed in webrtc/stats/.
BUG=chromium:627816
Committed: https://crrev.com/615d3013de815e52a1536a8c4c068b91c814ccdd
Cr-Commit-Position: refs/heads/master@{#13879}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed kjellander's comments #Patch Set 3 : Removed find_bad_constructs and GYP android test dep (but not GN android) #
Total comments: 26
Patch Set 4 : Addressed hta's comments #Patch Set 5 : WEBRTC_RTCSTATS_IMPL macro avoiding any boilerplate code in RTCStats subclasses #Patch Set 6 : Updated comments #
Total comments: 14
Patch Set 7 : Addressed hta's comments #Patch Set 8 : #if condition for EXPECT_DEATH tests #
Total comments: 30
Patch Set 9 : Addressed comments #
Messages
Total messages: 48 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== RTCStats BUG= ========== to ========== RTCStats and RTCStatsReport added (webrtc/stats). The old and new getStats are very different. This CL proposes rewriting the new getStats from scratch with a bottom-up approach, starting with the fundamental stats classes. This will allow cleaner and more efficient code that is more aligned with the spec. RTCStats and subclasses are the equivalent to RTCStats and RTCStats- -derived dictionaries from the specs[1][2]. The dictionary members are public member variables of type RTCStatsMember<T>, where T is one of the supported types. All members derive from RTCStatsMemberInterface and iteration of members is possible with RTCStats::Members(). The members are not stored in a map for performance and readability. Type checking is supported with static class variables, kType. Only the supported member types T are specialized and may be instantiated, and sequences are supported with std::vector<...>. Type checking is again supported with static class variables, kType. RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id to RTCStats-objects. RTCStatsReport is reference counted. It and its contained stats may be destroyed on any thread. When the RTCStatsCollector is added in a follow-up CL, it will return const references to the RTCStatsReports. This means copies don't have to be made for multiple stats observers or when jumping threads. In fact, no copies of any stats will have to be made in surfacing stats the Blink. [1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary [2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html [3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object This adds the new folder webrtc/stats/, with target rtc_stats and binary rtc_stats_unittests. Public api headers are placed in webrtc/api/ and .cc files are placed in webrtc/stats/. BUG=chromium:627816 ==========
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: Try jobs failed on following builders: ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/9524)
hbos@webrtc.org changed reviewers: + hta@webrtc.org, kjellander@webrtc.org, tommi@webrtc.org
Please take a look, tommi, hta and kjellander. tommi/hta for everything. kjellander primarily for the aspects of adding a new folder webrtc/stats and new unittest binary rtc_stats_unittests. https://codereview.webrtc.org/2241093002/diff/40001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/BUILD.gn#newcode262 webrtc/BUILD.gn:262: "stats:rtc_stats", Should I list the unittest (rtc_stats_unittests) too, here or somewhere else? In all.gyp wildcards are listed (e.g. stats.gyp:*) but that is not he trend here. In any case, rtc_stats_unittests is globally known without explicitly being listed. https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn File webrtc/stats/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:35: test("rtc_stats_unittests") { This is a new unittest built from a new folder, webrtc/stats/. I assume bots don't run it? Is a new binary OK? Do we enable it on bots before or after this CL has landed?
Description was changed from ========== RTCStats and RTCStatsReport added (webrtc/stats). The old and new getStats are very different. This CL proposes rewriting the new getStats from scratch with a bottom-up approach, starting with the fundamental stats classes. This will allow cleaner and more efficient code that is more aligned with the spec. RTCStats and subclasses are the equivalent to RTCStats and RTCStats- -derived dictionaries from the specs[1][2]. The dictionary members are public member variables of type RTCStatsMember<T>, where T is one of the supported types. All members derive from RTCStatsMemberInterface and iteration of members is possible with RTCStats::Members(). The members are not stored in a map for performance and readability. Type checking is supported with static class variables, kType. Only the supported member types T are specialized and may be instantiated, and sequences are supported with std::vector<...>. Type checking is again supported with static class variables, kType. RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id to RTCStats-objects. RTCStatsReport is reference counted. It and its contained stats may be destroyed on any thread. When the RTCStatsCollector is added in a follow-up CL, it will return const references to the RTCStatsReports. This means copies don't have to be made for multiple stats observers or when jumping threads. In fact, no copies of any stats will have to be made in surfacing stats the Blink. [1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary [2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html [3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object This adds the new folder webrtc/stats/, with target rtc_stats and binary rtc_stats_unittests. Public api headers are placed in webrtc/api/ and .cc files are placed in webrtc/stats/. BUG=chromium:627816 ========== to ========== RTCStats and RTCStatsReport added (webrtc/stats). The old and new getStats are very different. This CL proposes rewriting the new getStats from scratch with a bottom-up approach, starting with the fundamental stats classes. This will allow cleaner and more efficient code that is more aligned with the spec. RTCStats and subclasses are the equivalent to RTCStats and RTCStats- -derived dictionaries from the specs[1][2]. The dictionary members are public member variables of type RTCStatsMember<T>, where T is one of the supported types. All members derive from RTCStatsMemberInterface and iteration of members is possible with RTCStats::Members(). The members are not stored in a map for performance and readability. Type checking is supported with static class variables, kType. Only the supported member types T are specialized and may be instantiated, and sequences are supported with std::vector<...>. Type checking is again supported with static class variables, kType. RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id to RTCStats-objects. RTCStatsReport is reference counted. It and its contained stats may be destroyed on any thread. When the RTCStatsCollector is added in a follow-up CL, it will return const references to the RTCStatsReports. This means copies don't have to be made for multiple stats observers or when jumping threads. In fact, no copies of any stats will have to be made in surfacing stats to Blink. [1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary [2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html [3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object This adds the new folder webrtc/stats/, with target rtc_stats and binary rtc_stats_unittests. Public api headers are placed in webrtc/api/ and .cc files are placed in webrtc/stats/. BUG=chromium:627816 ==========
hbos@webrtc.org changed reviewers: + pthatcher@webrtc.org
pthatcher, can you take a look? tommi is OOO for two weeks.
https://codereview.webrtc.org/2241093002/diff/40001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/BUILD.gn#newcode262 webrtc/BUILD.gn:262: "stats:rtc_stats", On 2016/08/13 08:31:39, hbos wrote: > Should I list the unittest (rtc_stats_unittests) too, here or somewhere else? In > all.gyp wildcards are listed (e.g. stats.gyp:*) but that is not he trend here. > In any case, rtc_stats_unittests is globally known without explicitly being > listed. If you just put "stats" here, GN will make the assumption that you have a 'stats' target inside the webrtc/stats/BUILD.gn (I even think it'll complain if one is missing). I suggest you follow the pattern with a group entry as in https://chromium.googlesource.com/external/webrtc/+/master/webrtc/media/BUILD.gn. Since we don't want to include our test targets in Chromium builds, make sure to just have it inside a rtc_include_tests condition, then it'll be processed (as you've already noticed) https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn File webrtc/stats/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:25: configs -= [ "//build/config/clang:find_bad_constructs" ] Can't you fix the warnings instead of having to add this? https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:35: test("rtc_stats_unittests") { Can you make this target run on Android as well? Then it needs if (is_android) { deps += [ "//testing/android/native_test:native_test_native_code" ] } https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:46: configs -= [ "//build/config/clang:find_bad_constructs" ] Please fix the warnings instead of adding this (unless they're a flood of warnings that comes from dependencies, making it unfeasible to do in this CL). https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/stats.gyp File webrtc/stats/stats.gyp (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/stats.gyp#ne... webrtc/stats/stats.gyp:26: 'target_name': 'rtc_stats_unittests', Please add include_tests condition for this target (even if Chromium doesn't use GYP anymore, for completeness we should still follow that pattern).
PTAL kjellander https://codereview.webrtc.org/2241093002/diff/40001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/BUILD.gn#newcode262 webrtc/BUILD.gn:262: "stats:rtc_stats", On 2016/08/15 11:21:51, kjellander_webrtc wrote: > On 2016/08/13 08:31:39, hbos wrote: > > Should I list the unittest (rtc_stats_unittests) too, here or somewhere else? > In > > all.gyp wildcards are listed (e.g. stats.gyp:*) but that is not he trend here. > > In any case, rtc_stats_unittests is globally known without explicitly being > > listed. > > If you just put "stats" here, GN will make the assumption that you have a > 'stats' target inside the webrtc/stats/BUILD.gn (I even think it'll complain if > one is missing). > I suggest you follow the pattern with a group entry as in > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/media/BUILD.gn. > > Since we don't want to include our test targets in Chromium builds, make sure to > just have it inside a rtc_include_tests condition, then it'll be processed (as > you've already noticed) Done. https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn File webrtc/stats/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:25: configs -= [ "//build/config/clang:find_bad_constructs" ] On 2016/08/15 11:21:51, kjellander_webrtc wrote: > Can't you fix the warnings instead of having to add this? Done. https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:35: test("rtc_stats_unittests") { On 2016/08/15 11:21:51, kjellander_webrtc wrote: > Can you make this target run on Android as well? > > Then it needs > if (is_android) { > deps += [ "//testing/android/native_test:native_test_native_code" ] > } Done. https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:46: configs -= [ "//build/config/clang:find_bad_constructs" ] On 2016/08/15 11:21:51, kjellander_webrtc wrote: > Please fix the warnings instead of adding this (unless they're a flood of > warnings that comes from dependencies, making it unfeasible to do in this CL). Done. https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/stats.gyp File webrtc/stats/stats.gyp (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/stats.gyp#ne... webrtc/stats/stats.gyp:26: 'target_name': 'rtc_stats_unittests', On 2016/08/15 11:21:51, kjellander_webrtc wrote: > Please add include_tests condition for this target (even if Chromium doesn't use > GYP anymore, for completeness we should still follow that pattern). Done.
https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn File webrtc/stats/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:25: configs -= [ "//build/config/clang:find_bad_constructs" ] On 2016/08/15 13:27:11, hbos wrote: > On 2016/08/15 11:21:51, kjellander_webrtc wrote: > > Can't you fix the warnings instead of having to add this? > > Done. If you fixed the warnings, remove this line to prove it's sufficient :) Same below.
There we go. https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn File webrtc/stats/BUILD.gn (right): https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:25: configs -= [ "//build/config/clang:find_bad_constructs" ] On 2016/08/15 13:34:20, kjellander_webrtc wrote: > On 2016/08/15 13:27:11, hbos wrote: > > On 2016/08/15 11:21:51, kjellander_webrtc wrote: > > > Can't you fix the warnings instead of having to add this? > > > > Done. > > If you fixed the warnings, remove this line to prove it's sufficient :) > Same below. Done. (Oops, lost removing those lines when switching PC.) https://codereview.webrtc.org/2241093002/diff/40001/webrtc/stats/BUILD.gn#new... webrtc/stats/BUILD.gn:35: test("rtc_stats_unittests") { On 2016/08/15 13:27:12, hbos wrote: > On 2016/08/15 11:21:51, kjellander_webrtc wrote: > > Can you make this target run on Android as well? > > > > Then it needs > > if (is_android) { > > deps += [ "//testing/android/native_test:native_test_native_code" ] > > } > > Done. I made it work for GN, but not GYP as discussed offline (removed GYP's dependency).
lgtm
Cool, looksies pthatcher and hta? :)
hta@chromium.org changed reviewers: + hta@chromium.org
Looks good, as usual a bunch of nits wrt (hopefully) making the code more readable. This review brought to you by GoGo InFligt Internet. (and this comment was not added by an intercepting proxy :-) https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h File webrtc/api/rtcstats.h (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:27: // http://rawgit.com/w3c/webrtc-stats/master/webrtc-stats.html. Please use the w3c.github.io link - it's not as up to date, but it's less likely to be in a broken state. (At the moment at least.) https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:47: // printf("%s = %s\n", member->name(), member->ValueToString().c_str()); Even examples in comments should follow the style guide ... use braces. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:59: // Time in milliseconds relative to the UNIX epoch (Jan 1, 1970, UTC). Please use seconds when the type is double, unless you have a VERY powerful precedent for not doing so. The advantage of using a floating-point type is that scale doesn't matter. Also delete the word "milliseconds" - doubles have a lot more precision than that. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:70: std::string ToString() const; What representation does ToString() return? Is it useful for display only, or is it supposed to parse as legal JSON? https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:73: T& to() const { Is this function intended for downcasts (from the base type to the subtype)? If so, make a comment that it's so. This is RTTI (run time type information), which most of the time is a bad practice, so let's make sure we need it. Should it be cast_to, or is to() in common usage? https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:120: T& to() const { Same comment about name of function. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstatsreport.h File webrtc/api/rtcstatsreport.h (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstatsreport... webrtc/api/rtcstatsreport.h:25: // A collection of stats, maps from |RTCStats::id| to |RTCStats|. Grammar nit: "A collection of stats. This is accessible as [if it was?] a map from |RTCStats::id| to |RTCStats|." https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats.cc File webrtc/stats/rtcstats.cc (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats.cc#... webrtc/stats/rtcstats.cc:29: oss << "{ "; It's simpler to put the "oss << {" outside the loop. Keep the same checking for whether comma should be emitted. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats.cc#... webrtc/stats/rtcstats.cc:51: is_first = false; Same comment. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:46: const char* type_name() const override { return kType; } Is there any chance that these could be different? If not, can we declare them in the base class template? (less chance of later errors) There might be C++ reasons why that wouldn't work. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:122: }; For completeness, derive a class from RTCTestStats3 and see that it works as expected. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:150: stats.m_sequence_static_string = std::vector<const char*>(); Might be nice to thrown in a test that stats.m_sequence_string doesn't have a value yet (catches the case of some are instantiated, some aren't). https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:168: EXPECT_EQ(*stats.m_sequence_string, std::vector<std::string>()); Throw in a case where you give a value to a sequence member too. I don't think you need more than one, but you should have one.
PTAL hta, pthatcher. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h File webrtc/api/rtcstats.h (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:27: // http://rawgit.com/w3c/webrtc-stats/master/webrtc-stats.html. On 2016/08/15 15:14:57, hta - Chromium wrote: > Please use the w3c.github.io link - it's not as up to date, but it's less likely > to be in a broken state. (At the moment at least.) Done. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:47: // printf("%s = %s\n", member->name(), member->ValueToString().c_str()); On 2016/08/15 15:14:57, hta - Chromium wrote: > Even examples in comments should follow the style guide ... use braces. Done. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:59: // Time in milliseconds relative to the UNIX epoch (Jan 1, 1970, UTC). On 2016/08/15 15:14:56, hta - Chromium wrote: > Please use seconds when the type is double, unless you have a VERY powerful > precedent for not doing so. The advantage of using a floating-point type is that > scale doesn't matter. > > Also delete the word "milliseconds" - doubles have a lot more precision than > that. Done. Now I say that it is in seconds, without - I hope - implying that it is in whole seconds (integers). https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:70: std::string ToString() const; On 2016/08/15 15:14:57, hta - Chromium wrote: > What representation does ToString() return? Is it useful for display only, or is > it supposed to parse as legal JSON? Creates a string like so: peer-connection { id: "peerConnectionId" timestamp: 0.0 dataChannelsOpened: 1 dataChannelsClosed: 0 } Useful for debugging. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:73: T& to() const { On 2016/08/15 15:14:57, hta - Chromium wrote: > Is this function intended for downcasts (from the base type to the subtype)? If > so, make a comment that it's so. > > This is RTTI (run time type information), which most of the time is a bad > practice, so let's make sure we need it. > > Should it be cast_to, or is to() in common usage? > Updated to cast_to. Yes, downcasting, e.g. RTCStats -> RTCFooStats. This is for the case when you want to inspect specific types of stats, meaning you already have the type and members in mind. I think RTTI is the right approach for that. You wouldn't use this for general handling of stats, such as if you want to loop through every stat, you can do that with Members(). This allows filtering for RTCFooStats from a collection of RTCStats and then reading the value of foo.x directly without having to first search for x amongst a generic set of members. Casting yields cleaner code and compile-time errors if you use wrong member name/type. The benefit of this over casting yourself is we have a DCHECK. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstats.h#new... webrtc/api/rtcstats.h:120: T& to() const { On 2016/08/15 15:14:57, hta - Chromium wrote: > Same comment about name of function. Done. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstatsreport.h File webrtc/api/rtcstatsreport.h (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/api/rtcstatsreport... webrtc/api/rtcstatsreport.h:25: // A collection of stats, maps from |RTCStats::id| to |RTCStats|. On 2016/08/15 15:14:57, hta - Chromium wrote: > Grammar nit: > "A collection of stats. > This is accessible as [if it was?] a map from |RTCStats::id| to |RTCStats|." Done. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats.cc File webrtc/stats/rtcstats.cc (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats.cc#... webrtc/stats/rtcstats.cc:29: oss << "{ "; On 2016/08/15 15:14:57, hta - Chromium wrote: > It's simpler to put the "oss << {" outside the loop. Keep the same checking for > whether comma should be emitted. Done. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats.cc#... webrtc/stats/rtcstats.cc:51: is_first = false; On 2016/08/15 15:14:57, hta - Chromium wrote: > Same comment. Done. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:46: const char* type_name() const override { return kType; } On 2016/08/15 15:14:57, hta - Chromium wrote: > Is there any chance that these could be different? If not, can we declare them > in the base class template? (less chance of later errors) > > There might be C++ reasons why that wouldn't work. It is possible to have default implementations, but because it is a static class variable the implementation needs to know of the subclass type (T::kType, not RTCStats::kType). I added: template<typename T> class RTCStatsSubclass : public RTCStats { ... }; Where T is the deriving class: class RTCFooStats : public RTCStatsSubclass<RTCFooStats> { ... }; I think only with template can I define the boilerplate methods copy(), type() and type_name(). I updated the code like that and added comments. For subclasses of subclasses though you have to override again. The only alternative I can think of is using macros... https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:122: }; On 2016/08/15 15:14:57, hta - Chromium wrote: > For completeness, derive a class from RTCTestStats3 and see that it works as > expected. Done. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:150: stats.m_sequence_static_string = std::vector<const char*>(); On 2016/08/15 15:14:57, hta - Chromium wrote: > Might be nice to thrown in a test that stats.m_sequence_string doesn't have a > value yet (catches the case of some are instantiated, some aren't). The for loop above has already tested all members' |has_value|. An empty sequence counts as a value, was that a confusion? I'll rename it to |is_defined|. https://codereview.webrtc.org/2241093002/diff/80001/webrtc/stats/rtcstats_uni... webrtc/stats/rtcstats_unittest.cc:168: EXPECT_EQ(*stats.m_sequence_string, std::vector<std::string>()); On 2016/08/15 15:14:57, hta - Chromium wrote: > Throw in a case where you give a value to a sequence member too. I don't think > you need more than one, but you should have one. Done.
Update, patch set 5: I was able to solve the problem of having to write boilerplate implementations of kType, copy, type, type_name and ThisMembers. This would work for both subclasses and subclasses of subclasses, unlike a template solution (patch set 4). I like it, but macros are generally to be avoided, so it's debatable if this is an OK exception. What do you think?
Patchset #5 (id:120001) has been deleted
Bump, PTAL pthatcher and hta.
Reviewed the files that changed since my last review. I would like someone with more C++ wizardry to take a look at the macro to say whether it's a necessary evil or not. The rest is comments on comments and requests for more tests, I think. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h File webrtc/api/rtcstats.h (right): https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:103: // definition. The macro starts with "public", so it doesn't have to be in a public section - what about "All |RTCStats| subclasses must use this macro. It declares public class members, so should be in the public section of the declaration." https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:139: #define WEBRTC_RTCSTATS_IMPL(this_class, parent_class, ...) \ Argh. This is a macro, and it's a macro in an .h file. Since this macro is used in defining the objects on the public API of the class, I don't see a way to hide it. Can you ask nisse to take a look? (I believe he knows a bit of C++...) https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:145: const char* const* type() const override { return &this_class::kType; } \ Is it possible to use "typeof(this)" or some such construct to avoid the ugliness of having to pass the class name to a macro that can only be used inside the class definition? https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:148: std::vector<const webrtc::RTCStatsMemberInterface*> ThisMembers( \ ThisMembers is such an ugly name. What about MembersOfThisObjectAndAncestors? https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:162: // Interface for |RTCStats| members, which have a name and an optional value. Value type, not value? I don't think "optional" is the right word here. "and a value of a type defined in a subclass" is more correct. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/stats/rtcstats_un... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.webrtc.org/2241093002/diff/160001/webrtc/stats/rtcstats_un... webrtc/stats/rtcstats_unittest.cc:125: stats.m_sequence_uint64 = std::vector<uint64_t>(); Reiterating: The reason I want a test that something is unset at this point is that if someone at a later time thinks that this object is either uninitialized or initialized, they could try to store the "is_defined" in a single bit rather than a per-member-variable bit. I want to have a test that would break if they try that. At the moment, all tests would pass if you did it. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/stats/rtcstats_un... webrtc/stats/rtcstats_unittest.cc:167: EXPECT_EQ(*copy.grandchild_int, *stats.grandchild_int); Can you add a test (death test, needs to be separate?) that cast_to<RTCChildStats> causes a runtime error?
Patchset #7 (id:180001) has been deleted
hbos@webrtc.org changed reviewers: + nisse@webrtc.org
Please take a look, nisse. rtcstats.[h/cc] uses the C++ wizardry of macros and templates. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h File webrtc/api/rtcstats.h (right): https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:103: // definition. On 2016/08/22 07:01:53, hta-webrtc wrote: > The macro starts with "public", so it doesn't have to be in a public section - > what about > > "All |RTCStats| subclasses must use this macro. It declares public class > members, so should be in the public section of the declaration." But the macro changes the protection level (? -> public -> protected -> public). It would be confusing if the macro was used anywhere but the public section since it would otherwise "sneakingly" change the protection level to public. I think it should be required to be in the pubic section, macros shouldn't be used to change protection level. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:139: #define WEBRTC_RTCSTATS_IMPL(this_class, parent_class, ...) \ On 2016/08/22 07:01:53, hta-webrtc wrote: > Argh. This is a macro, and it's a macro in an .h file. > > Since this macro is used in defining the objects on the public API of the class, > I don't see a way to hide it. Can you ask nisse to take a look? (I believe he > knows a bit of C++...) I'll ask nisse to take a look. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:145: const char* const* type() const override { return &this_class::kType; } \ On 2016/08/22 07:01:53, hta-webrtc wrote: > Is it possible to use "typeof(this)" or some such construct to avoid the > ugliness of having to pass the class name to a macro that can only be used > inside the class definition? Hmm.. I don't think that is possible, and we don't compile with RTTI (typeid) and neither that or decltype can be used to reach static member variables AFAIK. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:148: std::vector<const webrtc::RTCStatsMemberInterface*> ThisMembers( \ On 2016/08/22 07:01:53, hta-webrtc wrote: > ThisMembers is such an ugly name. What about MembersOfThisObjectAndAncestors? > > Done. https://codereview.webrtc.org/2241093002/diff/160001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:162: // Interface for |RTCStats| members, which have a name and an optional value. On 2016/08/22 07:01:53, hta-webrtc wrote: > Value type, not value? > I don't think "optional" is the right word here. "and a value of a type defined > in a subclass" is more correct. Updated comment. (By optional I meant that the value can be defined or undefined.) https://codereview.webrtc.org/2241093002/diff/160001/webrtc/stats/rtcstats_un... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.webrtc.org/2241093002/diff/160001/webrtc/stats/rtcstats_un... webrtc/stats/rtcstats_unittest.cc:125: stats.m_sequence_uint64 = std::vector<uint64_t>(); On 2016/08/22 07:01:53, hta-webrtc wrote: > Reiterating: The reason I want a test that something is unset at this point is > that if someone at a later time thinks that this object is either uninitialized > or initialized, they could try to store the "is_defined" in a single bit rather > than a per-member-variable bit. I want to have a test that would break if they > try that. At the moment, all tests would pass if you did it. Oh okay, an EXPECT_FALSE(something->is_defined()) after having defined other members? https://codereview.webrtc.org/2241093002/diff/160001/webrtc/stats/rtcstats_un... webrtc/stats/rtcstats_unittest.cc:167: EXPECT_EQ(*copy.grandchild_int, *stats.grandchild_int); On 2016/08/22 07:01:53, hta-webrtc wrote: > Can you add a test (death test, needs to be separate?) that > cast_to<RTCChildStats> causes a runtime error? Done. But EXPECT_DEATH tests are surprisingly slow, ~2100 ms.
I've had a first read, focusing on the macrology. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h File webrtc/api/rtcstats.h (right): https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:66: virtual const char* const* type() const = 0; Do you really need to return a pointer to a pointer? As far as I see, static const char kType[] = "integer"; const char *type() { return kType; } should be sufficient to ensure that stat_A->type() == stat_B->type() is true if and only if stat_A and stat_B are of the same type. (Might be subtly different if you do static const char* kType = "integer"; instead, then compiler might unify with other equal string literals, so don't do that if you think that is a problem. But with kType[], you ought to get unique storage for each class, even if for some reason type names are the same.) https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:81: RTC_DCHECK_EQ(type(), &T::kType); I would expect that you don't do cast and then modify. If you agree, I'd suggest making this method and its return value const. Nice that you can do this poor-man's-dynamic_cast without macros. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:87: return static_cast<T&>(*this); And this one really ought to have a const return value, const T&, right? https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:141: #define WEBRTC_RTCSTATS_IMPL(parent_class, this_class, ...) \ You have checked that variadic macros, with ..., are standard enough to be used? (There are ugly workarounds if they aren't). https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:143: static const char* const kType; \ As above, I'd suggest making this static const char kType[] = name; Where name should be an additional argument to the macro. Or you could even let the preprocessor fill it out automatically using static const char kType[] = #this_class; if you think that stringified name is pretty enough. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:153: const webrtc::RTCStatsMemberInterface* members[] = { \ In general when doing large macros, you have to take care to name local variables to that they are highly unlikely to collides with names used outside of the macro. Consider the case where one of the macro arguments in __VA_ARGS__ happens to be &members. Not sure if the styleguide proposes a particular style here, but to be safe, you'd need to replace "members" with something more painful, like "rtcstats_impl_members" and similarly for all other local variables. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:159: additional_capacity + members_count); \ How do you test this allocation logic? Is there some test case that will raise an assert in case the vector is nevertheless resized? https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:161: &members[0], &members[members_count]); \ There's an extra copy, first into the members array, then into members_vec. Maybe not a big deal, but I wonder if there's some way to get around that. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:173: enum Type { Hmm, this was a bit confusing at first. Here types are identified by enums, while above they're identified by strings (or pointers thereto), which seems inconsistent. But the enums are used for primitive types, while strings are used for rtc-specific aggregates, right? https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:196: virtual bool is_string() const = 0; // true for sequences of strings too. That's kind of surprising. Why? Only use I've seen is if (is_string() && !is_sequence()) so it seems it makes things more complicated, not less. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:201: T& cast_to() const { I think the return type should be const T&. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats.cc File webrtc/stats/rtcstats.cc (right): https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats.cc... webrtc/stats/rtcstats.cc:29: if (!is_first) I think the is_first logic is unnecessary state. You handle the empty case up-front anyway, so you could do the loop as something like oss << "{" << vector.first(); for (loop over indices 1 to size-1) oss << ", " << element; (if there's a nice and concise way to do that with C++ iterators). Alternatively, replace the if (!is_first) by if (&element != vector.start()) or so. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats.cc... webrtc/stats/rtcstats.cc:90: // int32_t (kInt32) I'd consider using a local macro to reduce code duplication here... DEFINE_TYPE(int32_t, kInt32, false, false) DEFINE_TYPE(...)
lgtm I think I like this one now (modulo nisse's commentary + a few nits). Signing off. https://codereview.chromium.org/2241093002/diff/220001/webrtc/stats/rtcstats_... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.chromium.org/2241093002/diff/220001/webrtc/stats/rtcstats_... webrtc/stats/rtcstats_unittest.cc:205: #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) Can you add a comment (perhaps with a bug ID) on why these restrictions exist, so that we can remove the #if if the reasons to have it go away? https://codereview.chromium.org/2241093002/diff/220001/webrtc/stats/rtcstats_... webrtc/stats/rtcstats_unittest.cc:207: TEST_F(RTCStatsTest, ValueOfUndefinedMemberShouldCrash) { I think it's recommended to keep death tests in its own test class. (Not sure - I may be remembering things from long ago.) I don't think you use the fixture at all here, so these could be TEST macros rather than TEST_F macros.
Patchset #9 (id:240001) has been deleted
PTAL nisse, pthatcher. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h File webrtc/api/rtcstats.h (right): https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:66: virtual const char* const* type() const = 0; On 2016/08/23 08:27:28, nisse-webrtc wrote: > Do you really need to return a pointer to a pointer? As far as I see, > > static const char kType[] = "integer"; > const char *type() { return kType; } > > should be sufficient to ensure that stat_A->type() == stat_B->type() is true if > and only if stat_A and stat_B are of the same type. > > (Might be subtly different if you do > > static const char* kType = "integer"; > > instead, then compiler might unify with other equal string literals, so don't do > that if you think that is a problem. But with kType[], you ought to get unique > storage for each class, even if for some reason type names are the same.) Done. (You're right, const char[] works but not const char*/const char* const.) https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:81: RTC_DCHECK_EQ(type(), &T::kType); On 2016/08/23 08:27:28, nisse-webrtc wrote: > I would expect that you don't do cast and then modify. If you agree, I'd suggest > making this method and its return value const. > > Nice that you can do this poor-man's-dynamic_cast without macros. With a const return value on the other |cast_to| method, this one can be removed. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:87: return static_cast<T&>(*this); On 2016/08/23 08:27:28, nisse-webrtc wrote: > And this one really ought to have a const return value, const T&, right? Done. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:141: #define WEBRTC_RTCSTATS_IMPL(parent_class, this_class, ...) \ On 2016/08/23 08:27:28, nisse-webrtc wrote: > You have checked that variadic macros, with ..., are standard enough to be used? > (There are ugly workarounds if they aren't). Yes, it is used by other code (e.g. webrtc/base/logging.h) and compiles on all trybots. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:143: static const char* const kType; \ On 2016/08/23 08:27:28, nisse-webrtc wrote: > As above, I'd suggest making this > > static const char kType[] = name; > > Where name should be an additional argument to the macro. Or you could even let > the preprocessor fill it out automatically using > > static const char kType[] = #this_class; > > if you think that stringified name is pretty enough. Unfortunately, the variable can only be declared in the class definition, the variable must be defined separately in a .cc file and " = ...;" is as such not part of the macro. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:153: const webrtc::RTCStatsMemberInterface* members[] = { \ On 2016/08/23 08:27:28, nisse-webrtc wrote: > In general when doing large macros, you have to take care to name local > variables to that they are highly unlikely to collides with names used outside > of the macro. Consider the case where one of the macro arguments in __VA_ARGS__ > happens to be &members. > > Not sure if the styleguide proposes a particular style here, but to be safe, > you'd need to replace "members" with something more painful, like > "rtcstats_impl_members" and similarly for all other local variables. Done. Nice catch. I couldn't find anything in the style guide about this and code I found for reference had no special variable names. I renamed them to "local_var_...". https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:159: additional_capacity + members_count); \ On 2016/08/23 08:27:28, nisse-webrtc wrote: > How do you test this allocation logic? Is there some test case that will raise > an assert in case the vector is nevertheless resized? I added a DCHECK that ensures the capacity is enough to avoid resize. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:161: &members[0], &members[members_count]); \ On 2016/08/23 08:27:28, nisse-webrtc wrote: > There's an extra copy, first into the members array, then into members_vec. > Maybe not a big deal, but I wonder if there's some way to get around that. Variadic functions come to mind, but I don't think it would improve performance, only make it harder to read. (I'm guessing va_... stuff works like an array behind the scenes anyway.) https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:173: enum Type { On 2016/08/23 08:27:28, nisse-webrtc wrote: > Hmm, this was a bit confusing at first. Here types are identified by enums, > while above they're identified by strings (or pointers thereto), which seems > inconsistent. > > But the enums are used for primitive types, while strings are used for > rtc-specific aggregates, right? That is correct. The RTCStats type is for RTCStats subclasses. If an enum was used (original patch set had that) then this file/the RTCStats class would have to know of all subclasses, and adding custom "stats dictionaries" would require modifying this file, kind of like modifying an interface in the act of implementing it. const char kType[] was a means to decouple rtcstats.h from implementation files. We could have done the same thing for RTCStatsMemberInterface, but since the type of members is a short list that is likely to be fixed, an enum made sense for readability and simplicity. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:196: virtual bool is_string() const = 0; // true for sequences of strings too. On 2016/08/23 08:27:28, nisse-webrtc wrote: > That's kind of surprising. Why? > > Only use I've seen is > > if (is_string() && !is_sequence()) > > so it seems it makes things more complicated, not less. Hmm, yeah, strange decision, I'll make it false instead. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/api/rtcstats.h#ne... webrtc/api/rtcstats.h:201: T& cast_to() const { On 2016/08/23 08:27:28, nisse-webrtc wrote: > I think the return type should be const T&. Done. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats.cc File webrtc/stats/rtcstats.cc (right): https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats.cc... webrtc/stats/rtcstats.cc:29: if (!is_first) On 2016/08/23 08:27:28, nisse-webrtc wrote: > I think the is_first logic is unnecessary state. > > You handle the empty case up-front anyway, so you could do the loop as something > like > > oss << "{" << vector.first(); > for (loop over indices 1 to size-1) > oss << ", " << element; > > (if there's a nice and concise way to do that with C++ iterators). > > Alternatively, replace the if (!is_first) by > if (&element != vector.start()) or so. > Done. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats.cc... webrtc/stats/rtcstats.cc:90: // int32_t (kInt32) On 2016/08/23 08:27:28, nisse-webrtc wrote: > I'd consider using a local macro to reduce code duplication here... > > DEFINE_TYPE(int32_t, kInt32, false, false) > DEFINE_TYPE(...) Done. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats_un... File webrtc/stats/rtcstats_unittest.cc (right): https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats_un... webrtc/stats/rtcstats_unittest.cc:205: #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) On 2016/08/23 14:25:13, hta - Chromium wrote: > Can you add a comment (perhaps with a bug ID) on why these restrictions exist, > so that we can remove the #if if the reasons to have it go away? There are many #ifs such as this in the webrtc repo, but none of them have a comment saying why or with a bug number. The only explanation I found was a comment in base/test/gtest_util.h saying "Death tests misbehave on Android." I referene this file in a comment. https://codereview.webrtc.org/2241093002/diff/220001/webrtc/stats/rtcstats_un... webrtc/stats/rtcstats_unittest.cc:207: TEST_F(RTCStatsTest, ValueOfUndefinedMemberShouldCrash) { On 2016/08/23 14:25:13, hta - Chromium wrote: > I think it's recommended to keep death tests in its own test class. (Not sure - > I may be remembering things from long ago.) I don't think you use the fixture at > all here, so these could be TEST macros rather than TEST_F macros. Done.
lgtm
lgtm
hbos@webrtc.org changed reviewers: + glaznev@webrtc.org
glaznev, can you take a look as a webrtc/api OWNER? (Both pthatcher and tommi have gone OOO before looking.)
glaznev@webrtc.org changed reviewers: + deadbeef@webrtc.org
On 2016/08/23 16:31:42, hbos wrote: > glaznev, can you take a look as a webrtc/api OWNER? (Both pthatcher and tommi > have gone OOO before looking.) +Taylor for api OWNER
lgtm
lgtm
On 2016/08/23 22:37:53, AlexG wrote: > On 2016/08/23 16:31:42, hbos wrote: > > glaznev, can you take a look as a webrtc/api OWNER? (Both pthatcher and tommi > > have gone OOO before looking.) > > +Taylor for api OWNER Huh? You are too? I have the LGTMs, landing.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, hta@chromium.org Link to the patchset: https://codereview.webrtc.org/2241093002/#ps260001 (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 ========== RTCStats and RTCStatsReport added (webrtc/stats). The old and new getStats are very different. This CL proposes rewriting the new getStats from scratch with a bottom-up approach, starting with the fundamental stats classes. This will allow cleaner and more efficient code that is more aligned with the spec. RTCStats and subclasses are the equivalent to RTCStats and RTCStats- -derived dictionaries from the specs[1][2]. The dictionary members are public member variables of type RTCStatsMember<T>, where T is one of the supported types. All members derive from RTCStatsMemberInterface and iteration of members is possible with RTCStats::Members(). The members are not stored in a map for performance and readability. Type checking is supported with static class variables, kType. Only the supported member types T are specialized and may be instantiated, and sequences are supported with std::vector<...>. Type checking is again supported with static class variables, kType. RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id to RTCStats-objects. RTCStatsReport is reference counted. It and its contained stats may be destroyed on any thread. When the RTCStatsCollector is added in a follow-up CL, it will return const references to the RTCStatsReports. This means copies don't have to be made for multiple stats observers or when jumping threads. In fact, no copies of any stats will have to be made in surfacing stats to Blink. [1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary [2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html [3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object This adds the new folder webrtc/stats/, with target rtc_stats and binary rtc_stats_unittests. Public api headers are placed in webrtc/api/ and .cc files are placed in webrtc/stats/. BUG=chromium:627816 ========== to ========== RTCStats and RTCStatsReport added (webrtc/stats). The old and new getStats are very different. This CL proposes rewriting the new getStats from scratch with a bottom-up approach, starting with the fundamental stats classes. This will allow cleaner and more efficient code that is more aligned with the spec. RTCStats and subclasses are the equivalent to RTCStats and RTCStats- -derived dictionaries from the specs[1][2]. The dictionary members are public member variables of type RTCStatsMember<T>, where T is one of the supported types. All members derive from RTCStatsMemberInterface and iteration of members is possible with RTCStats::Members(). The members are not stored in a map for performance and readability. Type checking is supported with static class variables, kType. Only the supported member types T are specialized and may be instantiated, and sequences are supported with std::vector<...>. Type checking is again supported with static class variables, kType. RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id to RTCStats-objects. RTCStatsReport is reference counted. It and its contained stats may be destroyed on any thread. When the RTCStatsCollector is added in a follow-up CL, it will return const references to the RTCStatsReports. This means copies don't have to be made for multiple stats observers or when jumping threads. In fact, no copies of any stats will have to be made in surfacing stats to Blink. [1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary [2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html [3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object This adds the new folder webrtc/stats/, with target rtc_stats and binary rtc_stats_unittests. Public api headers are placed in webrtc/api/ and .cc files are placed in webrtc/stats/. BUG=chromium:627816 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== RTCStats and RTCStatsReport added (webrtc/stats). The old and new getStats are very different. This CL proposes rewriting the new getStats from scratch with a bottom-up approach, starting with the fundamental stats classes. This will allow cleaner and more efficient code that is more aligned with the spec. RTCStats and subclasses are the equivalent to RTCStats and RTCStats- -derived dictionaries from the specs[1][2]. The dictionary members are public member variables of type RTCStatsMember<T>, where T is one of the supported types. All members derive from RTCStatsMemberInterface and iteration of members is possible with RTCStats::Members(). The members are not stored in a map for performance and readability. Type checking is supported with static class variables, kType. Only the supported member types T are specialized and may be instantiated, and sequences are supported with std::vector<...>. Type checking is again supported with static class variables, kType. RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id to RTCStats-objects. RTCStatsReport is reference counted. It and its contained stats may be destroyed on any thread. When the RTCStatsCollector is added in a follow-up CL, it will return const references to the RTCStatsReports. This means copies don't have to be made for multiple stats observers or when jumping threads. In fact, no copies of any stats will have to be made in surfacing stats to Blink. [1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary [2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html [3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object This adds the new folder webrtc/stats/, with target rtc_stats and binary rtc_stats_unittests. Public api headers are placed in webrtc/api/ and .cc files are placed in webrtc/stats/. BUG=chromium:627816 ========== to ========== RTCStats and RTCStatsReport added (webrtc/stats). The old and new getStats are very different. This CL proposes rewriting the new getStats from scratch with a bottom-up approach, starting with the fundamental stats classes. This will allow cleaner and more efficient code that is more aligned with the spec. RTCStats and subclasses are the equivalent to RTCStats and RTCStats- -derived dictionaries from the specs[1][2]. The dictionary members are public member variables of type RTCStatsMember<T>, where T is one of the supported types. All members derive from RTCStatsMemberInterface and iteration of members is possible with RTCStats::Members(). The members are not stored in a map for performance and readability. Type checking is supported with static class variables, kType. Only the supported member types T are specialized and may be instantiated, and sequences are supported with std::vector<...>. Type checking is again supported with static class variables, kType. RTCStatsReport is the equivalent from the spec[3], and maps RTCStats::id to RTCStats-objects. RTCStatsReport is reference counted. It and its contained stats may be destroyed on any thread. When the RTCStatsCollector is added in a follow-up CL, it will return const references to the RTCStatsReports. This means copies don't have to be made for multiple stats observers or when jumping threads. In fact, no copies of any stats will have to be made in surfacing stats to Blink. [1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstats-dictionary [2] https://w3c.github.io/webrtc-stats/archives/20160526/webrtc-stats.html [3] https://www.w3.org/TR/2016/WD-webrtc-20160531/#rtcstatsreport-object This adds the new folder webrtc/stats/, with target rtc_stats and binary rtc_stats_unittests. Public api headers are placed in webrtc/api/ and .cc files are placed in webrtc/stats/. BUG=chromium:627816 Committed: https://crrev.com/615d3013de815e52a1536a8c4c068b91c814ccdd Cr-Commit-Position: refs/heads/master@{#13879} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/615d3013de815e52a1536a8c4c068b91c814ccdd Cr-Commit-Position: refs/heads/master@{#13879} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
