Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2207453003:
Track SSRCs configured for each media type in event log visualization tool. (Closed)
DescriptionTrack SSRCs configured for each media type in event log visualization tool.
Committed: https://crrev.com/0740a2040a167b62e70f8f24f695263bde4a971a
Cr-Commit-Position: refs/heads/master@{#13675}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comments from reviewers #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Track SSRCs configured for each in event log visualization tool. ========== to ========== Track SSRCs configured for each media type in event log visualization tool. ==========
terelius@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
Please take a look.
lg, but is there a specific reason you moved StreamId out of EventLogAnalyzer? https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:31: : ssrc_(ssrc), direction_(direction) {} Move implementation to .cc file.
lgtm https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:313: return (rtx_ssrcs_.count(stream_id) == 1); I think you can remove (). I'd also prefer using find() instead of count().
lgtm lgtm
lgtm lgtm lgtm
Oops. lgtm, but please address my comment :)
On 2016/08/04 12:56:02, philipel wrote: > lg, but is there a specific reason you moved StreamId out of EventLogAnalyzer? > The newly added functions take a StreamId as argument, so the StreamId has to be defined before those functions. There are various ways to do this, but moving it out of the class is the cleanest solution if the new functions should be public. I could make the new functions private for now.
https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:313: return (rtx_ssrcs_.count(stream_id) == 1); On 2016/08/04 14:22:55, stefan-webrtc (holmer) wrote: > I think you can remove (). > > I'd also prefer using find() instead of count(). Removed the parenthesis. I prefer count() since that value is less powerful than an iterator into the map. https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:31: : ssrc_(ssrc), direction_(direction) {} On 2016/08/04 12:56:02, philipel wrote: > Move implementation to .cc file. I prefer to keep trivial constructors in the class definition.
https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:31: : ssrc_(ssrc), direction_(direction) {} On 2016/08/05 12:24:57, terelius wrote: > On 2016/08/04 12:56:02, philipel wrote: > > Move implementation to .cc file. > > I prefer to keep trivial constructors in the class definition. I was thinking of moving the full implementation to the .cc file. Now the implementation is both in the .h and .cc file. Also, any specific reason to keep trivial constructors in the .h file?
https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2207453003/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:31: : ssrc_(ssrc), direction_(direction) {} On 2016/08/05 12:43:21, philipel wrote: > On 2016/08/05 12:24:57, terelius wrote: > > On 2016/08/04 12:56:02, philipel wrote: > > > Move implementation to .cc file. > > > > I prefer to keep trivial constructors in the class definition. > > I was thinking of moving the full implementation to the .cc file. Now the > implementation is both in the .h and .cc file. > > Also, any specific reason to keep trivial constructors in the .h file? It makes it easier to verify that all members are initialized properly. The code also becomes shorter since it avoids boiler-plate code in two places. I moved the remaining two definitions from the .cc file to the header file.
lgtm
The CQ bit was checked by terelius@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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/15422)
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2207453003/#ps20001 (title: "Comments from reviewers")
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Track SSRCs configured for each media type in event log visualization tool. ========== to ========== Track SSRCs configured for each media type in event log visualization tool. Committed: https://crrev.com/0740a2040a167b62e70f8f24f695263bde4a971a Cr-Commit-Position: refs/heads/master@{#13675} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0740a2040a167b62e70f8f24f695263bde4a971a Cr-Commit-Position: refs/heads/master@{#13675} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
