Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
972203002:
Support finch experiment WebRTC-UDPServerSocketAsync for Windows. (Closed)
|
Created:
5 years, 9 months ago by John L. Miller Modified:
5 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport finch experiment WebRTC-UDPServerSocketAsync for Windows.
BUG=463472
Committed: https://crrev.com/6dff163a2658c891d39a53f8bc128f76d128780f
Cr-Commit-Position: refs/heads/master@{#322464}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add code to support WebRTC-UDPServerSocketAsync finch experiment. #Patch Set 3 : Set UDP server port async for experiment. #
Total comments: 6
Patch Set 4 : Change experiment name for Windows async UDP finch. #Patch Set 5 : Enable Windows-only async socket experiment in constructor. #Patch Set 6 : Move UDP socket experiment check to constructor. #
Total comments: 2
Patch Set 7 : Move UDP socket experimental config to constructor. #Messages
Total messages: 25 (9 generated)
jlmiller@chromium.org changed reviewers: + guoweis@chromium.org, pthatcher@chromium.org
jlmiller@chromium.org changed required reviewers: + guoweis@chromium.org
https://codereview.chromium.org/972203002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/972203002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host_udp.cc:120: if (use_nonblocking_io > 0) { Instead of using 0 to stand for disabled, could we change to "Enabled" and "Disabled"? You could see WebRTC-IPv6Default as an example.
jlmiller@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:114: if (base::FieldTrialList::FindFullName("WebRTC-UDPServerSocketAsync") == This is not a good name for the experiment: overlapped IO that is used by default is asynchronous too. Also you don't need to call them "ServerSocket". They are just UDP sockets. I would suggest "WebRTC-UDPSocketNonBlockingIO". It's still a bit ambiguous, but at least it's more consistent with the code. https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:116: static_cast<UdpServerSocket *>(socket_.get())->UseNonBlockingIO(); You can avoid upcast if you put this in the constructor (socket_ initialization would need to be moved to the constructor body, out of the initializer list)
As per request on corresponding code review, renaming this file and experiment 'WebRTC-UDPSocketNonBlockingIO https://codereview.chromium.org/972203002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/972203002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/p2p/socket_host_udp.cc:120: if (use_nonblocking_io > 0) { On 2015/03/03 17:43:39, guoweis_chromium wrote: > Instead of using 0 to stand for disabled, could we change to "Enabled" and > "Disabled"? You could see WebRTC-IPv6Default as an example. Done. https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:114: if (base::FieldTrialList::FindFullName("WebRTC-UDPServerSocketAsync") == On 2015/03/13 18:28:51, Sergey Ulanov wrote: > This is not a good name for the experiment: overlapped IO that is used by > default is asynchronous too. Also you don't need to call them "ServerSocket". > They are just UDP sockets. I would suggest "WebRTC-UDPSocketNonBlockingIO". It's > still a bit ambiguous, but at least it's more consistent with the code. Done. https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:114: if (base::FieldTrialList::FindFullName("WebRTC-UDPServerSocketAsync") == On 2015/03/13 18:28:51, Sergey Ulanov wrote: > This is not a good name for the experiment: overlapped IO that is used by > default is asynchronous too. Also you don't need to call them "ServerSocket". > They are just UDP sockets. I would suggest "WebRTC-UDPSocketNonBlockingIO". It's > still a bit ambiguous, but at least it's more consistent with the code. Done. https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:116: static_cast<UdpServerSocket *>(socket_.get())->UseNonBlockingIO(); On 2015/03/13 18:28:51, Sergey Ulanov wrote: > You can avoid upcast if you put this in the constructor (socket_ initialization > would need to be moved to the constructor body, out of the initializer list) I'm nervous about changing the type when socket_ is used many other places. Going to leave this way if that's OK.
Hi Sergey - if you're OK with the review as it stands, can you LGTM? thanks! john
https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/972203002/diff/40001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:116: static_cast<UdpServerSocket *>(socket_.get())->UseNonBlockingIO(); On 2015/03/23 14:11:28, John L. Miller wrote: > On 2015/03/13 18:28:51, Sergey Ulanov wrote: > > You can avoid upcast if you put this in the constructor (socket_ > initialization > > would need to be moved to the constructor body, out of the initializer list) > > I'm nervous about changing the type when socket_ is used many other places. > Going to leave this way if that's OK. I wasn't suggesting to change type of |socket_|. I'm just saying you can avoid this upcast by moving this code in the constructor. So in the construction you would have something like this: net::UDPServerSocket* socket = new net::UDPServerSocket(...); socket_ = socket; if (experiment enabled) { socket->UseNonBlockingIO(); }
Moved async call for UDP socket experiment to constructor.
LGTM when my comment is addressed https://codereview.chromium.org/972203002/diff/100001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/972203002/diff/100001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:71: P2PSocketHostUdp::P2PSocketHostUdp(IPC::Sender* message_sender, int socket_id, Formatting was correct here. One argument per line in function definition. See https://www.chromium.org/developers/coding-style
https://codereview.chromium.org/972203002/diff/100001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/972203002/diff/100001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:71: P2PSocketHostUdp::P2PSocketHostUdp(IPC::Sender* message_sender, int socket_id, On 2015/03/26 18:15:32, Sergey Ulanov wrote: > Formatting was correct here. One argument per line in function definition. See > https://www.chromium.org/developers/coding-style Done.
The CQ bit was checked by jlmiller@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/972203002/#ps120001 (title: "Move UDP socket experimental config to constructor.")
The CQ bit was unchecked by jlmiller@chromium.org
The CQ bit was checked by jlmiller@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972203002/120001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Guowei - your name has a star next to it :) can I get an LGTM? On Thu, Mar 26, 2015 at 12:21 PM, <commit-bot@chromium.org> wrote: > All required reviewers (with asterisk prefixes) have not yet approved this > CL. > > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. > > https://codereview.chromium.org/972203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/03/26 19:25:45, chromium-reviews wrote: > Guowei - your name has a star next to it :) can I get an LGTM? > > On Thu, Mar 26, 2015 at 12:21 PM, <mailto:commit-bot@chromium.org> wrote: > > > All required reviewers (with asterisk prefixes) have not yet approved this > > CL. > > > > No LGTM from a valid reviewer yet. Only full committers are accepted. > > Even if an LGTM may have been provided, it was from a non-committer, > > _not_ a full super star committer. > > See http://www.chromium.org/getting-involved/become-a-committer > > Note that this has nothing to do with OWNERS files. > > > > https://codereview.chromium.org/972203002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. lgtm.
The CQ bit was checked by jlmiller@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972203002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6dff163a2658c891d39a53f8bc128f76d128780f Cr-Commit-Position: refs/heads/master@{#322464} |
