Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(33)

Issue 30001: Re-upload of patch

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 5 months ago by ekr-webrtc
Modified:
7 years, 5 months ago
Visibility:
Public.

Description

Re-upload of patch

Patch Set 1 #

Total comments: 14

Patch Set 2 : Revised version #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M media/mtransport/nricectx.cpp View 2 chunks +15 lines, -0 lines 0 comments Download
A media/mtransport/nrinterfaceprioritizer.cpp View 1 1 chunk +189 lines, -0 lines 0 comments Download
A media/mtransport/nrinterfaceprioritizer.h View 1 chunk +18 lines, -0 lines 0 comments Download
M media/mtransport/objs.mk View 2 chunks +3 lines, -1 line 0 comments Download
M media/mtransport/third_party/nICEr/nicer.gyp View 1 chunk +2 lines, -0 lines 0 comments Download
M media/mtransport/third_party/nICEr/src/ice/ice_candidate.c View 1 1 chunk +27 lines, -13 lines 0 comments Download
M media/mtransport/third_party/nICEr/src/ice/ice_component.c View 1 1 chunk +1 line, -9 lines 0 comments Download
M media/mtransport/third_party/nICEr/src/ice/ice_component.h View 1 1 chunk +1 line, -1 line 0 comments Download
M media/mtransport/third_party/nICEr/src/ice/ice_ctx.c View 1 3 chunks +40 lines, -1 line 0 comments Download
M media/mtransport/third_party/nICEr/src/ice/ice_ctx.h View 3 chunks +3 lines, -0 lines 0 comments Download
M media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c View 1 1 chunk +2 lines, -2 lines 0 comments Download
M media/mtransport/third_party/nICEr/src/ice/ice_media_stream.h View 1 1 chunk +1 line, -1 line 0 comments Download
A media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c View 1 1 chunk +88 lines, -0 lines 0 comments Download
A media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.h View 1 chunk +66 lines, -0 lines 0 comments Download
M media/mtransport/third_party/nICEr/src/net/transport_addr.c View 1 chunk +21 lines, -0 lines 0 comments Download
M media/mtransport/third_party/nICEr/src/net/transport_addr.h View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 3
ekr-webrtc
This is looking pretty good. Please fix the minor issues noted below, but I think ...
7 years, 5 months ago #1
kk1fff-patrick-wang
https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_party/nICEr/src/ice/ice_component.c File media/mtransport/third_party/nICEr/src/ice/ice_component.c (right): https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_party/nICEr/src/ice/ice_component.c#newcode201 media/mtransport/third_party/nICEr/src/ice/ice_component.c:201: nr_interface_prioritizer_add_interface(ctx->interface_prioritizer,addrs+i); On 2013/08/23 01:33:50, ekr-webrtc wrote: > Error check ...
7 years, 5 months ago #2
kk1fff-patrick-wang
7 years, 5 months ago #3
https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterf...
File media/mtransport/nrinterfaceprioritizer.cpp (right):

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterf...
media/mtransport/nrinterfaceprioritizer.cpp:4: #include <algorithm>
On 2013/08/23 01:33:50, ekr-webrtc wrote:
> Do we still need <algorithm>

It was for std::sort(), I forgot to remove it.

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterf...
media/mtransport/nrinterfaceprioritizer.cpp:49: if (is_vpn_ != rhs.is_vpn_) {
On 2013/08/23 01:33:50, ekr-webrtc wrote:
> I wonder if in this case it would make sense to have is_vpn_ be an integer
with
> 0 being not and 1 being yes. 
> 
> In that case you could do a comparison directly.
> 
> Whatcha think?

Sounds good to me. It makes this comparison easier to read.

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterf...
media/mtransport/nrinterfaceprioritizer.cpp:66: }
On 2013/08/23 01:33:50, ekr-webrtc wrote:
> Whitespace here please.

Done.

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/nrinterf...
media/mtransport/nrinterfaceprioritizer.cpp:96: int add(nr_local_addr *iface) {
On 2013/08/23 01:33:50, ekr-webrtc wrote:
> const?

Done.

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_pa...
File media/mtransport/third_party/nICEr/src/ice/ice_candidate.c (right):

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_pa...
media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:385:
if(r=nr_transport_addr_fmt_ifname_addr_string(&cand->base,key_of_interface,
On 2013/08/23 01:33:50, ekr-webrtc wrote:
> Vertical whitespace here please.

Done.

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_pa...
File media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c
(right):

https://firefox-codereview.appspot.com/30001/diff/1/media/mtransport/third_pa...
media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c:62: if
(!ifpp || !*ifpp)
On 2013/08/23 01:33:50, ekr-webrtc wrote:
> Whitespace between variables and code.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 905+:3555e821a5cd+