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

Issue 20001: Bug 869869.

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

Description

Bug 869869.

Patch Set 1 #

Total comments: 15

Patch Set 2 : Next revision from sc #

Total comments: 85

Patch Set 3 : REvised version from SC #

Total comments: 42

Patch Set 4 : REvised patch #

Total comments: 29

Patch Set 5 : Further Revisions #

Total comments: 8

Patch Set 6 : Another revision #

Total comments: 30

Patch Set 7 : Another revision #

Total comments: 29

Patch Set 8 : REvision 8.1 #

Total comments: 21

Patch Set 9 : New revision #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M media/mtransport/nr_socket_prsock.cpp View 1 2 3 4 5 6 7 8 9 chunks +533 lines, -53 lines 4 comments Download
M media/mtransport/nr_socket_prsock.h View 1 2 3 4 5 6 7 8 1 chunk +124 lines, -21 lines 0 comments Download

Messages

Total messages: 23
ekr-webrtc
Here are some initial comments on this patch. LMK if you want to discuss. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socket_prsock.cpp ...
7 years, 7 months ago #1
ekr-webrtc
http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_socket_prsock.cpp#newcode139 media/mtransport/nr_socket_prsock.cpp:139: addPollFlags(flag); Why not just directly frob mPollFlags http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_socket_prsock.cpp#newcode445 media/mtransport/nr_socket_prsock.cpp:445: ...
7 years, 7 months ago #2
schien
feedback is updated. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_socket_prsock.cpp#newcode139 media/mtransport/nr_socket_prsock.cpp:139: addPollFlags(flag); On 2013/06/20 14:38:33, ekr-webrtc wrote: ...
7 years, 7 months ago #3
ekr-webrtc
Responses below. I think the mPollFlags thing will be a lot simpler when you change ...
7 years, 7 months ago #4
ekr-webrtc
SC: more comments below. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_socket_prsock.cpp#newcode630 media/mtransport/nr_socket_prsock.cpp:630: if ((r=nr_transport_addr_get_addrstring(addr, addr_string, sizeof(addr_string)))) { ...
7 years, 6 months ago #5
schien
https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_socket_prsock.cpp#newcode202 media/mtransport/nr_socket_prsock.cpp:202: mPollFlags = poll_flags(); On 2013/07/01 04:55:33, ekr-webrtc wrote: > ...
7 years, 6 months ago #6
ekr-webrtc
https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_socket_prsock.h File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_socket_prsock.h#newcode167 media/mtransport/nr_socket_prsock.h:167: nr_udp_message(const nr_udp_message &other) { On 2013/07/07 15:44:59, schien wrote: ...
7 years, 6 months ago #7
schien
https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_socket_prsock.h File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_socket_prsock.h#newcode167 media/mtransport/nr_socket_prsock.h:167: nr_udp_message(const nr_udp_message &other) { On 2013/07/07 16:19:16, ekr-webrtc wrote: ...
7 years, 6 months ago #8
ekr-webrtc
This is looking pretty good. I'm still a little concerned about how you're handling errors. ...
7 years, 6 months ago #9
schien
https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_socket_prsock.cpp#newcode269 media/mtransport/nr_socket_prsock.cpp:269: static int nr_transport_addr_to_netaddr(nr_transport_addr *addr, On 2013/07/17 10:19:03, ekr-webrtc wrote: ...
7 years, 6 months ago #10
ekr-webrtc
https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_socket_prsock.cpp#newcode574 media/mtransport/nr_socket_prsock.cpp:574: return NS_OK; On 2013/07/18 07:09:13, schien wrote: > On ...
7 years, 6 months ago #11
ekr
A few small nits. I think we should be good in the next cycle. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_socket_prsock.cpp ...
7 years, 6 months ago #12
schien
On 2013/07/26 01:39:48, ekr wrote: > A few small nits. I think we should be ...
7 years, 6 months ago #13
ekr
Isn't this in CAllListenerArrayBuffer? On Thu, Jul 25, 2013 at 6:51 PM, <schien@mozilla.com> wrote: > ...
7 years, 6 months ago #14
schien
https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_socket_prsock.cpp#newcode269 media/mtransport/nr_socket_prsock.cpp:269: static int nr_transport_addr_to_netaddr(nr_transport_addr *addr, On 2013/07/26 01:39:48, ekr wrote: ...
7 years, 5 months ago #15
ekr-webrtc
I went through the whole bug and found a few more issues. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp ...
7 years, 5 months ago #16
schien
https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_socket_prsock.cpp#newcode283 media/mtransport/nr_socket_prsock.cpp:283: } On 2013/08/08 22:26:10, ekr-webrtc wrote: > This needs ...
7 years, 5 months ago #17
ekr-webrtc
https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_socket_prsock.cpp#newcode586 media/mtransport/nr_socket_prsock.cpp:586: NS_IMETHODIMP NrSocketIpc::CallListenerVoid(const nsACString &type) { On 2013/08/09 10:13:41, schien ...
7 years, 5 months ago #18
schien
https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_socket_prsock.cpp#newcode308 media/mtransport/nr_socket_prsock.cpp:308: praddr_to_netaddr(&praddr, naddr); On 2013/08/09 19:28:14, ekr-webrtc wrote: > Check ...
7 years, 5 months ago #19
ekr-webrtc
This is looking good. I don't see any tests, however. Can you tell me what ...
7 years, 5 months ago #20
schien
http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_socket_prsock.cpp File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_socket_prsock.cpp#newcode376 media/mtransport/nr_socket_prsock.cpp:376: int nr_transport_addr_get_addrstring_and_port(nr_transport_addr *addr, On 2013/08/12 19:50:07, ekr-webrtc wrote: > ...
7 years, 5 months ago #21
ekr-webrtc
This looks basically fine. You should be able to land it once it's tested http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_socket_prsock.cpp ...
7 years, 5 months ago #22
schien
7 years, 5 months ago #23
http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so...
File media/mtransport/nr_socket_prsock.cpp (right):

http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:646:
nr_transport_addr_copy(&expected_addr, &my_addr_);
On 2013/08/15 21:06:54, ekr-webrtc wrote:
> Please check for errors here.

Done.

http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:819: RefPtr<nr_udp_message>
msg(received_msgs_.front());
On 2013/08/15 21:06:54, ekr-webrtc wrote:
> Why did you change this?

Originally msg is a reference to RefPtr, which doesn't increase the reference
count. Doing pop before use might cause SEGV, therefore, I change the type of
msg for explicitly holding the reference.
Furthermore, namespace "mozilla" is redundant so I simply remove it.
Sign in to reply to this message.

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