Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12684008:
Multicast socket API (Closed)
|
Created:
7 years, 9 months ago by Bei Zhang Modified:
7 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce Multicast Socket API
Allow Chrome Apps developer to receive multicast socket packets by exposing join/leave group ability to UDP socket.
Introducing:
1. chrome.socket.joinGroup / chrome.socket.leaveGroup / chrome.socket.getJoinedGroups to manipulate multicast group membership.
2. Socket permission 'udp-multicast-membership'.
3. chrome.socket.setMulticastTimeToLive / chrome.socket.setMulticastLoopbackMode to control the multicast packet sending for UDP sender.
To expose the ability of manipulating multicast group membership and controlling multicast packet sending, new methods are added into network stack (net::UDPSocket class):
1. JoinGroup/LeaveGroup
2. SetMulticastTimeToLive/SetMulticastLoopbackMode
To demo the ability, a demo app is created:
https://github.com/GoogleChrome/chrome-app-samples/pull/92
TEST=Open the demo app at /chrome-app-samples/multicast in two machines of the same network. Open them and chat, they can talk to each other.
NOTRY=true
BUG=140681
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197192
Patch Set 1 : Muticast socket API #Patch Set 2 : Rebase lkgr #
Total comments: 21
Patch Set 3 : Update documents #Patch Set 4 : Minor fixes #Patch Set 5 : Fix tests #
Total comments: 73
Patch Set 6 : Fix code style #
Total comments: 52
Patch Set 7 : Fix as per wtc and mmenke #Patch Set 8 : Fix windows build #
Total comments: 19
Patch Set 9 : Fix builds #Patch Set 10 : Disable multicast test on android #
Total comments: 26
Patch Set 11 : Fix tests #
Total comments: 13
Patch Set 12 : Changing DCHECKs to returning error code. #
Total comments: 8
Patch Set 13 : Minor fixes #
Total comments: 51
Patch Set 14 : Fixes #
Total comments: 4
Patch Set 15 : Changing extensions::UDPSocket::multicast_groups_ to a vector #Patch Set 16 : Merge to master/origin #Patch Set 17 : Add License info to multicast.js #Messages
Total messages: 34 (0 generated)
Hi Vince, Please review. Thanks, Bei
Hi Mike, Please review. Thanks, Bei
Initial review - though I'm quite unfamiliar with multicast, so expect and seek out more scrutiny from someone with networking domain knowledge. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:117: callback JoinGroupCallback = void (long result); Document what the result values mean, and that -1 will be returned in error cases. Etc. for other callbacks. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:274: // Leave the multicast group previously joined using <code>joinGroup</code> Period at end of line. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:280: // the host is joined to the group. Wording is a bit awkward. Perhaps: "Leaving the group before destroying the socket will prevent the router from sending multicast [[?datagrams, or packets?]] to the local host, presuming no other process on the host is still joined to the group." https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:306: // Get the multicast group adrresses the socket currently joining. addresses the socket is currently joined to. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... File chrome/common/extensions/permissions/socket_permission_data.h (right): https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/permissions/socket_permission_data.h:31: // The multicast membership permission implies to a permission to any address. "The multicast membership permission implies a permission to any address."? https://codereview.chromium.org/12684008/diff/29009/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/socket/api/background.js (right): https://codereview.chromium.org/12684008/diff/29009/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/background.js:238: var kMulticastAddress = "237.132.100." + (Math.random() * 250 >> 0); Random in tests tends to result in flaky tests. Pick a few stable random addresses and hard code them if needed. https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.h:116: We should have tests in the /net/ directory. Probably they should have enough coverage to land the additions to net first, then follow up with extensions support using it. https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.h:122: // Join the multicast group. Leave ... https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.h:130: // Set the time-to-live option for udp packets sent to the multicast What unit is this int? seconds? ms? https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:119: int JoinGroup(const net::IPAddressNumber& group_address) const; Comment these member functions.
https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:117: callback JoinGroupCallback = void (long result); I checked other files and found out that callbacks are not typically documented here. They are sometimes documented in the functions accepting them. In this case, as we have a lot of callbacks, it might be a little bit different though. On 2013/04/02 18:25:17, scheib wrote: > Document what the result values mean, and that -1 will be returned in error > cases. Etc. for other callbacks. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:274: // Leave the multicast group previously joined using <code>joinGroup</code> On 2013/04/02 18:25:17, scheib wrote: > Period at end of line. Done. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:280: // the host is joined to the group. On 2013/04/02 18:25:17, scheib wrote: > Wording is a bit awkward. Perhaps: > "Leaving the group before destroying the socket will prevent the router from > sending multicast [[?datagrams, or packets?]] to the local host, presuming no > other process on the host is still joined to the group." Done. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:306: // Get the multicast group adrresses the socket currently joining. On 2013/04/02 18:25:17, scheib wrote: > addresses the socket is currently joined to. Done. https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... File chrome/common/extensions/permissions/socket_permission_data.h (right): https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/permissions/socket_permission_data.h:31: // The multicast membership permission implies to a permission to any address. On 2013/04/02 18:25:17, scheib wrote: > "The multicast membership permission implies a permission to any address."? Done. https://codereview.chromium.org/12684008/diff/29009/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/socket/api/background.js (right): https://codereview.chromium.org/12684008/diff/29009/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/background.js:238: var kMulticastAddress = "237.132.100." + (Math.random() * 250 >> 0); My concern is, as multiple slaves might be running in the same subnet, they may send to each other thus make the result flaky if they are joining the same group. Maybe I should ask someone who knows how the machines are deployed? On 2013/04/02 18:25:17, scheib wrote: > Random in tests tends to result in flaky tests. Pick a few stable random > addresses and hard code them if needed. https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.h:122: // Join the multicast group. On 2013/04/02 18:25:17, scheib wrote: > Leave ... Done. https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.h:130: // Set the time-to-live option for udp packets sent to the multicast This is measured by hops. So it's a unitless. On 2013/04/02 18:25:17, scheib wrote: > What unit is this int? seconds? ms? https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/12684008/diff/29009/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:119: int JoinGroup(const net::IPAddressNumber& group_address) const; On 2013/04/02 18:25:17, scheib wrote: > Comment these member functions. Done.
https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/12684008/diff/29009/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:117: callback JoinGroupCallback = void (long result); On 2013/04/05 00:38:59, Bei Zhang wrote: > I checked other files and found out that callbacks are not typically documented > here. They are sometimes documented in the functions accepting them. In this > case, as we have a lot of callbacks, it might be a little bit different though. > On 2013/04/02 18:25:17, scheib wrote: > > Document what the result values mean, and that -1 will be returned in error > > cases. Etc. for other callbacks. > In at least some place, document what the return values mean. https://codereview.chromium.org/12684008/diff/29009/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/socket/api/background.js (right): https://codereview.chromium.org/12684008/diff/29009/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/background.js:238: var kMulticastAddress = "237.132.100." + (Math.random() * 250 >> 0); On 2013/04/05 00:38:59, Bei Zhang wrote: > My concern is, as multiple slaves might be running in the same subnet, they may > send to each other thus make the result flaky if they are joining the same > group. Maybe I should ask someone who knows how the machines are deployed? > > On 2013/04/02 18:25:17, scheib wrote: > > Random in tests tends to result in flaky tests. Pick a few stable random > > addresses and hard code them if needed. > Good concern, I don't know a good solution, consult domain experts. ;)
Hi Vince, I've edited the test cases according to the comments of jar and network stack team. Thanks, Bei
lgtm on the topics I raised earlier. Again, I'm don't have enough networking domain knowledge to thoroughly review this -- so attention is needed by later reviewers.
Hi owners, please review: miket: chrome/browser/extensions/api/socket/socket_api.cc chrome/browser/extensions/api/socket/socket_api.h chrome/browser/extensions/api/socket/socket_apitest.cc chrome/browser/extensions/api/socket/udp_socket.cc chrome/browser/extensions/api/socket/udp_socket.h chrome/browser/extensions/api/socket/udp_socket_unittest.cc chrome/browser/extensions/extension_function_histogram_value.h chrome/common/extensions/api/socket.idl chrome/common/extensions/permissions/socket_permission_data.cc chrome/common/extensions/permissions/socket_permission_data.h chrome/common/extensions/permissions/socket_permission_unittest.cc mmenke: net/udp/udp_socket_libevent.cc net/udp/udp_socket_libevent.h net/udp/udp_socket_win.cc net/udp/udp_socket_win.h piman: content/public/common/socket_permission_request.h Thanks, Bei
LGTM, but adding jam for final vetting of public API
On 2013/04/12 20:47:05, piman wrote: > LGTM, but adding jam for final vetting of public API lgtm
Bunch of style nits (No actual review of code yet). https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.cc:689: : params_(NULL) {} 4 space indent. Comment applies to whole file. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.cc:714: ¶m)) { Should either be aligned with APIPermission::kSocket, or "APIPermission::kSocket" should be on the next line, and both should be indented 4 spaces (Can optionally put them both on the same line, but should be indented 4 spaces more than the start of the "!GetExtension()"). Comment applies to whole file. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.cc:893: } // namespace extensions Line break before end of namespace. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:356: SocketJoinGroupFunction(); 2 space indent. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:373: SocketLeaveGroupFunction(); 2 space indent. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:391: SocketSetMulticastTimeToLiveFunction(); 2 space indent. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:409: SocketSetMulticastLoopbackModeFunction(); 2 space indent. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:427: SocketGetJoinedGroupsFunction(); 2 space indent. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket.cc:234: multicast_groups_.find(normalized_address); 4-space indent when continued from previous line. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket.cc:255: multicast_groups_.find(normalized_address); 4-space indent when continued from previous line. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: EXPECT_EQ(result, 0); Expected value should be the first argument, the value you're testing the second. Goes for the rest of this file, too. (Order doesn't matter for anything other than EQ and NE) https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:59: MessageLoopForIO io_loop; // for RecvFrom to do its threaded work. Comments should generally start with a capital. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:77: EXPECT_NE(src.SetMulticastTimeToLive(-1), 0); // Negative TTL shall fail Comments should generally end with a period. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:77: EXPECT_NE(src.SetMulticastTimeToLive(-1), 0); // Negative TTL shall fail 2 spaces before comment. https://codereview.chromium.org/12684008/diff/80001/chrome/common/extensions/... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/12684008/diff/80001/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:270: // |callback| : Called when the join group operation is done with an integer Is this line too long? Comment should also end with a period. https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/socket/api/background.js (right): https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/background.js:237: nit: Remove extra blank line. https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/socket/api/multicast.js (right): https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/multicast.js:4: for(var i=0;i<count;i++) { Space after for and after each semi-colon. https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/multicast.js:63: "Cannot bind client udp socket."); This should be either indented 4-spaces, with clientSocketId one line down and indented the same on the next line, or it should be lined up with clientSocketId. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.cc (left): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:480: return MapSystemError(errno); Why get rid of this? https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:484: sizeof(true_value)); I don't think we need to do this twice on platforms other than OSX https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:532: switch(group_address.size()) { Space after switch. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:536: } Braces are generally discouraged in single line if statements (Same for the rest of these files). https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:575: &mreq, sizeof(mreq)); Fix indent. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:586: &mreq, sizeof(mreq)); Fix indent. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:594: int UDPSocketLibevent::SetMulticastTimeToLive(int timeToLive) { Rename, per comment in header. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:599: if(is_connected()) { Space after if. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:614: if(is_connected()) { Space after if. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.h:133: int SetMulticastTimeToLive(int timeToLive); time_to_live, per google naming rules. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:634: switch(group_address.size()) { Space after switch. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:697: int UDPSocketWin::SetMulticastTimeToLive(int timeToLive) { time_to_live https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:699: if(is_connected()) { Space after if https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:714: Remove blank line. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:719: if(is_connected()) { Space after if https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:726: sizeof(loop))); 4-space indent. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:135: int SetMulticastTimeToLive(int timeToLive); time_to_live https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:187: int sock_addr_family_; This should be initialized in the constructor. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:196: bool multicast_loopback_mode_; These, too.
Thanks for review! Also, I added two forgotten unit tests for net/udp for the new methods. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.cc:689: : params_(NULL) {} On 2013/04/12 21:07:41, mmenke wrote: > 4 space indent. Comment applies to whole file. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.cc:714: ¶m)) { On 2013/04/12 21:07:41, mmenke wrote: > Should either be aligned with APIPermission::kSocket, or > "APIPermission::kSocket" should be on the next line, and both should be indented > 4 spaces (Can optionally put them both on the same line, but should be indented > 4 spaces more than the start of the "!GetExtension()"). > > Comment applies to whole file. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.cc:893: } // namespace extensions On 2013/04/12 21:07:41, mmenke wrote: > Line break before end of namespace. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:356: SocketJoinGroupFunction(); On 2013/04/12 21:07:41, mmenke wrote: > 2 space indent. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:373: SocketLeaveGroupFunction(); On 2013/04/12 21:07:41, mmenke wrote: > 2 space indent. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:391: SocketSetMulticastTimeToLiveFunction(); On 2013/04/12 21:07:41, mmenke wrote: > 2 space indent. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:409: SocketSetMulticastLoopbackModeFunction(); On 2013/04/12 21:07:41, mmenke wrote: > 2 space indent. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/socket_api.h:427: SocketGetJoinedGroupsFunction(); On 2013/04/12 21:07:41, mmenke wrote: > 2 space indent. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket.cc:234: multicast_groups_.find(normalized_address); On 2013/04/12 21:07:41, mmenke wrote: > 4-space indent when continued from previous line. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: EXPECT_EQ(result, 0); On 2013/04/12 21:07:41, mmenke wrote: > Expected value should be the first argument, the value you're testing the > second. > > Goes for the rest of this file, too. (Order doesn't matter for anything other > than EQ and NE) Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:59: MessageLoopForIO io_loop; // for RecvFrom to do its threaded work. On 2013/04/12 21:07:41, mmenke wrote: > Comments should generally start with a capital. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:77: EXPECT_NE(src.SetMulticastTimeToLive(-1), 0); // Negative TTL shall fail On 2013/04/12 21:07:41, mmenke wrote: > Comments should generally end with a period. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/browser/extensions... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:77: EXPECT_NE(src.SetMulticastTimeToLive(-1), 0); // Negative TTL shall fail On 2013/04/12 21:07:41, mmenke wrote: > Comments should generally end with a period. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/common/extensions/... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/12684008/diff/80001/chrome/common/extensions/... chrome/common/extensions/api/socket.idl:270: // |callback| : Called when the join group operation is done with an integer On 2013/04/12 21:07:41, mmenke wrote: > Is this line too long? Comment should also end with a period. This comment continues to the next line. https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/socket/api/background.js (right): https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/background.js:237: On 2013/04/12 21:07:41, mmenke wrote: > nit: Remove extra blank line. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/socket/api/multicast.js (right): https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/multicast.js:4: for(var i=0;i<count;i++) { On 2013/04/12 21:07:41, mmenke wrote: > Space after for and after each semi-colon. Done. https://codereview.chromium.org/12684008/diff/80001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/socket/api/multicast.js:63: "Cannot bind client udp socket."); On 2013/04/12 21:07:41, mmenke wrote: > This should be either indented 4-spaces, with clientSocketId one line down and > indented the same on the next line, or it should be lined up with > clientSocketId. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.cc (left): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:480: return MapSystemError(errno); On 2013/04/12 21:07:41, mmenke wrote: > Why get rid of this? I simply moved this out side of the #if to eliminate some duplicate code. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:484: sizeof(true_value)); On 2013/04/12 21:07:41, mmenke wrote: > I don't think we need to do this twice on platforms other than OSX Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:532: switch(group_address.size()) { On 2013/04/12 21:07:41, mmenke wrote: > Space after switch. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:536: } On 2013/04/12 21:07:41, mmenke wrote: > Braces are generally discouraged in single line if statements (Same for the rest > of these files). Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:575: &mreq, sizeof(mreq)); On 2013/04/12 21:07:41, mmenke wrote: > Fix indent. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:586: &mreq, sizeof(mreq)); On 2013/04/12 21:07:41, mmenke wrote: > Fix indent. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:594: int UDPSocketLibevent::SetMulticastTimeToLive(int timeToLive) { On 2013/04/12 21:07:41, mmenke wrote: > Rename, per comment in header. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:599: if(is_connected()) { On 2013/04/12 21:07:41, mmenke wrote: > Space after if. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.cc:614: if(is_connected()) { On 2013/04/12 21:07:41, mmenke wrote: > Space after if. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_libeve... net/udp/udp_socket_libevent.h:133: int SetMulticastTimeToLive(int timeToLive); On 2013/04/12 21:07:41, mmenke wrote: > time_to_live, per google naming rules. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:634: switch(group_address.size()) { On 2013/04/12 21:07:41, mmenke wrote: > Space after switch. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:697: int UDPSocketWin::SetMulticastTimeToLive(int timeToLive) { On 2013/04/12 21:07:41, mmenke wrote: > time_to_live Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:699: if(is_connected()) { On 2013/04/12 21:07:41, mmenke wrote: > Space after if Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:714: On 2013/04/12 21:07:41, mmenke wrote: > Remove blank line. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:719: if(is_connected()) { On 2013/04/12 21:07:41, mmenke wrote: > Space after if Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.cc... net/udp/udp_socket_win.cc:726: sizeof(loop))); On 2013/04/12 21:07:41, mmenke wrote: > 4-space indent. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:135: int SetMulticastTimeToLive(int timeToLive); On 2013/04/12 21:07:41, mmenke wrote: > time_to_live Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:187: int sock_addr_family_; On 2013/04/12 21:07:41, mmenke wrote: > This should be initialized in the constructor. Done. https://codereview.chromium.org/12684008/diff/80001/net/udp/udp_socket_win.h#... net/udp/udp_socket_win.h:196: bool multicast_loopback_mode_; On 2013/04/12 21:07:41, mmenke wrote: > These, too. Done.
While this looks reasonable to me, I feel that I'm not sufficiently familiar with the specifics of UDP multicast to adequately review this code. [+wtc]: Mind taking a look? https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:485: sizeof(true_value)); This needs to be done on OSX, too. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:530: int rv = -1; Sending a -1 through MapSystemError on fall through seems rather random to me. My suggestion is to have: default: NOTREACHED(); return ERR_INVALID_ARGUMENT; in the switch statement instead. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:537: memcpy(&mreq.imr_multiaddr, &group_address[0], kIPv4AddressSize); Seems like we should be explicitely using IN_ADDRANY for mreq.imr_interface. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:549: &mreq, sizeof(mreq)); Seems like we should be explicitely using in6addr_any for mreq.imr_interface. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:593: if (is_connected()) { Do we need these to be dynamically configurable? My only contern here is that makes for an inconsistent API, as everything else has to be set before bind, which I think makes for a confusing API. If it is needed, seems like we should be setting multicast_loopback_mode_ on success, to be consistent. Can easily see someone expecting multicast_time_to_live_ to be accurate in a future CL. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:600: } else { Preferred style in Chrome is not to have an else here, as it's not needed. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:133: int SetMulticastTimeToLive(int time_to_live); Think it's worth mentioned that this is safe to call either before or after the socket is bound. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:535: net::IPAddressNumber groupIp; nit: group_ip
Review comments on patch set 6: Thank you for writing the CL. I only reviewed the files in net/udp (except the unit tests). I only commented on the _win files, but all of my comments also apply to the _libevent files. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:591: if (multicast_time_to_live_ != 1) { Can you cite a MSDN page or some Microsoft documentation that says the default value of IP_MULTICAST_TTL is 1 on Windows? Or is this default value specified by an RFC? https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:636: int rv = -1; Nit: initialize |rv| to SOCKET_ERROR (which has the value -1). https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:658: break; NOTE: all of my comments on JoinGroup also apply to LeaveGroup. For brevity I won't repeat them. Add a default case: default: NOTREACHED(); return ERR_ADDRESS_INVALID; https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:661: return MapSystemError(rv); BUG: this should be: if (rv != 0) return MapSystemError(WSAGetLastError()); return OK; https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:700: return ERR_INVALID_ARGUMENT; This check should be moved to line 709, before we save time_to_live in multicast_time_to_live_. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:717: uint8 loop = loopback; NOTE: all of my comments on SetMulticastLoopbackMode also apply to SetMulticastTimeToLive. For brevity, I won't repeat them. BUG?: Are you sure |loop| should be a uint8? Winsock usually uses 'int' or 'BOOL' for socket options that are boolean flags. These two MSDN pages say |loop| should be a DWORD: http://msdn.microsoft.com/en-us/library/windows/desktop/ms738586(v=vs.85).aspx http://msdn.microsoft.com/en-us/library/windows/desktop/ms738574(v=vs.85).aspx The second MSDN page says IPV6_MULTICAST_LOOP should be used with IPPROTO_IPV6. Also note the "Note" in this MSDN page about the difference of IP_MULTICAST_LOOP between Windows and Unix: http://msdn.microsoft.com/en-us/library/windows/desktop/ms739161(v=vs.85).aspx https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:719: return MapSystemError(setsockopt(socket_, BUG: MapSystemError() takes a socket error code (WSAGetLastError(), or errno for POSIX). But setsockopt() returns a success/failure status (0 or -1/SOCKET_ERROR). https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:725: multicast_loopback_mode_ = loopback; I think this function should always store |loopback| in multicast_loopback_mode_, like this: int UDPSocketWin::SetMulticastLoopbackMode(bool loopback) { DCHECK(CalledOnValidThread()); multicast_loopback_mode_ = loopback; if (is_connected()) { // call setsockopt ... } return OK; } Are you sure this socket option cannot be set before the UDP socket is connected? https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:10: #include "base/hash_tables.h" I believe "base/hash_tables.h" should be removed because I don't see any use of base::hash_* in this file. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:121: // IPv4 or IPv6 address. Nit: add "an" before "IPv4". Make the same change to line 126. Please document the 'int' return value of all the new functions. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:122: int JoinGroup(const net::IPAddressNumber& group_address) const; I suggest naming these two methods JoinMulticastGroup and LeaveMulticastGroup. Would that be too wordy? Please remove "net::" throughout this file because it is inside the net namespace. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:132: // Set the time-to-live option for udp packets sent to the multicast Nit: udp => UDP Also on line 137. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:133: // group address. Initially this value is 1. Cannot be negative or Initially => By default ? Or we can say "The default value of this option is 1." Also on line 139. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:134: // more than 255. Can this value be 0? https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:138: // will receive package sent to the joined group from itself. package => packets ? https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:187: int sock_addr_family_; Nit: you can omit "sock_" in this member's name because that is implied. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:194: // Cannot be used after Bind(). "Cannot be used after Bind()" seems to contradicts the implementation in the .cc file. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:196: bool multicast_loopback_mode_; I think multicast_loopback_mode_ should be replaced by a bit flag in socket_options_. See lines 143-146 and 189-191.
https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:485: sizeof(true_value)); On 2013/04/16 18:09:49, mmenke wrote: > This needs to be done on OSX, too. It's done in the other #if branch. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:530: int rv = -1; On 2013/04/16 18:09:49, mmenke wrote: > Sending a -1 through MapSystemError on fall through seems rather random to me. > > My suggestion is to have: > > default: > NOTREACHED(); > return ERR_INVALID_ARGUMENT; > > in the switch statement instead. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:537: memcpy(&mreq.imr_multiaddr, &group_address[0], kIPv4AddressSize); On 2013/04/16 18:09:49, mmenke wrote: > Seems like we should be explicitely using IN_ADDRANY for mreq.imr_interface. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:549: &mreq, sizeof(mreq)); On 2013/04/16 18:09:49, mmenke wrote: > Seems like we should be explicitely using in6addr_any for mreq.imr_interface. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:593: if (is_connected()) { I thought about this for a while. I think you are right, I cannot think of a scenario that user cannot do this before bind/connect. Changing it now. On 2013/04/16 18:09:49, mmenke wrote: > Do we need these to be dynamically configurable? My only contern here is that > makes for an inconsistent API, as everything else has to be set before bind, > which I think makes for a confusing API. > > If it is needed, seems like we should be setting multicast_loopback_mode_ on > success, to be consistent. Can easily see someone expecting > multicast_time_to_live_ to be accurate in a future CL. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:600: } else { On 2013/04/16 18:09:49, mmenke wrote: > Preferred style in Chrome is not to have an else here, as it's not needed. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:133: int SetMulticastTimeToLive(int time_to_live); Please see comments in udp_socket_libevent.cc On 2013/04/16 18:09:49, mmenke wrote: > Think it's worth mentioned that this is safe to call either before or after the > socket is bound. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:535: net::IPAddressNumber groupIp; On 2013/04/16 18:09:49, mmenke wrote: > nit: group_ip Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:591: if (multicast_time_to_live_ != 1) { http://ibiblio.org/pub/linux/docs/HOWTO/other-formats/html_single/Multicast-H... mentioned the default value is 1. Also the loopback is by default enabled. On 2013/04/16 19:59:42, wtc wrote: > > Can you cite a MSDN page or some Microsoft documentation > that says the default value of IP_MULTICAST_TTL is 1 on > Windows? Or is this default value specified by an RFC? https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:636: int rv = -1; On 2013/04/16 19:59:42, wtc wrote: > > Nit: initialize |rv| to SOCKET_ERROR (which has the value -1). Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:658: break; On 2013/04/16 19:59:42, wtc wrote: > > NOTE: all of my comments on JoinGroup also apply to LeaveGroup. > For brevity I won't repeat them. > > Add a default case: > default: > NOTREACHED(); > return ERR_ADDRESS_INVALID; Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:661: return MapSystemError(rv); On 2013/04/16 19:59:42, wtc wrote: > > BUG: this should be: > if (rv != 0) > return MapSystemError(WSAGetLastError()); > return OK; Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:700: return ERR_INVALID_ARGUMENT; I'm changing the behavior to make it only works before Bind/Connect to be consist with other methods. This also applies to SetMulticastLoopbackMode() On 2013/04/16 19:59:42, wtc wrote: > > This check should be moved to line 709, before we save > time_to_live in multicast_time_to_live_. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:717: uint8 loop = loopback; Thank you for pointing that out. Adding the notes to the documents about this issue. On 2013/04/16 19:59:42, wtc wrote: > > NOTE: all of my comments on SetMulticastLoopbackMode also > apply to SetMulticastTimeToLive. For brevity, I won't repeat > them. > > BUG?: Are you sure |loop| should be a uint8? Winsock > usually uses 'int' or 'BOOL' for socket options that are > boolean flags. These two MSDN pages say |loop| should be a > DWORD: > http://msdn.microsoft.com/en-us/library/windows/desktop/ms738586%28v=vs.85%29... > > http://msdn.microsoft.com/en-us/library/windows/desktop/ms738574%28v=vs.85%29... > > The second MSDN page says IPV6_MULTICAST_LOOP should be > used with IPPROTO_IPV6. > > Also note the "Note" in this MSDN page about the difference > of IP_MULTICAST_LOOP between Windows and Unix: > http://msdn.microsoft.com/en-us/library/windows/desktop/ms739161%28v=vs.85%29... https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:719: return MapSystemError(setsockopt(socket_, On 2013/04/16 19:59:42, wtc wrote: > > BUG: MapSystemError() takes a socket error code > (WSAGetLastError(), or errno for POSIX). But setsockopt() > returns a success/failure status (0 or -1/SOCKET_ERROR). Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:725: multicast_loopback_mode_ = loopback; On 2013/04/16 19:59:42, wtc wrote: > > I think this function should always store |loopback| in > multicast_loopback_mode_, like this: > > int UDPSocketWin::SetMulticastLoopbackMode(bool loopback) { > DCHECK(CalledOnValidThread()); > multicast_loopback_mode_ = loopback; > if (is_connected()) { > // call setsockopt > ... > } > return OK; > } > > Are you sure this socket option cannot be set before the > UDP socket is connected? Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:10: #include "base/hash_tables.h" On 2013/04/16 19:59:42, wtc wrote: > > I believe "base/hash_tables.h" should be removed because > I don't see any use of base::hash_* in this file. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:121: // IPv4 or IPv6 address. On 2013/04/16 19:59:42, wtc wrote: > > Nit: add "an" before "IPv4". Make the same change to line 126. > > Please document the 'int' return value of all the new functions. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:132: // Set the time-to-live option for udp packets sent to the multicast On 2013/04/16 19:59:42, wtc wrote: > > Nit: udp => UDP > > Also on line 137. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:133: // group address. Initially this value is 1. Cannot be negative or On 2013/04/16 19:59:42, wtc wrote: > Initially => By default ? > > Or we can say "The default value of this option is 1." > > Also on line 139. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:134: // more than 255. Yes. It means loopback only. On 2013/04/16 19:59:42, wtc wrote: > > Can this value be 0? https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:138: // will receive package sent to the joined group from itself. On 2013/04/16 19:59:42, wtc wrote: > > package => packets ? Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:187: int sock_addr_family_; On 2013/04/16 19:59:42, wtc wrote: > > Nit: you can omit "sock_" in this member's name because > that is implied. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:194: // Cannot be used after Bind(). On 2013/04/16 19:59:42, wtc wrote: > > "Cannot be used after Bind()" seems to contradicts the > implementation in the .cc file. Done. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.h... net/udp/udp_socket_win.h:196: bool multicast_loopback_mode_; On 2013/04/16 19:59:42, wtc wrote: > > I think multicast_loopback_mode_ should be replaced by a > bit flag in socket_options_. See lines 143-146 and 189-191. Done.
Patch set 8 LGTM. 1. I'm giving you LGTM in advance to save time. This CL still needs work. Please make the suggested changes and upload a new patch set before you check this in. 2. Some of my comments also apply to the similar code (which may be in a different file). For brevity I didn't repeat the comments. 3. I reviewed all the filed under net/, and just skimmed through the other files. https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/12684008/diff/111001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:717: uint8 loop = loopback; On 2013/04/17 17:53:03, Bei Zhang wrote: > Thank you for pointing that out. Adding the notes to the documents about this > issue. I think you should also add the notes to the udp_socket_libevent.h and udp_socket_win.h header files. https://codereview.chromium.org/12684008/diff/130004/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.h (right): https://codereview.chromium.org/12684008/diff/130004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.h:45: int GetJoinedGroups(base::hash_set<std::string>* groups) const; I think you should document these five new methods. This is outside my area, so I don't know if your team requires this. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:491: uint8 loop = 0; BUG: it seems that this variable needs to be a u_int for IPv6. See http://lists.ntp.org/pipermail/bugs/2011-May/013564.html and http://publib.boulder.ibm.com/infocenter/zos/v1r13/index.jsp?topic=%2Fcom.ibm... https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:492: int ip_family = This variable should be named 'level' or 'protocol_level' instead of 'ip_family'. See http://pubs.opengroup.org/onlinepubs/009695399/functions/setsockopt.html Make the same change to line 502. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:505: addr_family_ == AF_INET ? IP_MULTICAST_TTL: IP_MULTICAST_TTL; BUG?: I think we need to use IPV6_MULTICAST_HOPS for IPv6. Note that IPV6_MULTICAST_HOPS uses an 'int' for its option value, according to http://publib.boulder.ibm.com/infocenter/zos/v1r13/index.jsp?topic=%2Fcom.ibm... https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:134: // Should be called before Bind(). Add: Return a network error code. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:226: // Cannot be used after Bind(). "Cannot be used after Bind()": Is this still true? https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:534: const char* kGroup = "237.132.100.17"; This should be const char* const kGroup = ...; so that kGroup itself is constant (rather than the thing it points to is constant). https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:535: net::IPAddressNumber group_ip; Remove net:: because this is inside the net namespace. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:542: NetLog::Source())); Nit: it doesn't seem necessary to use a scoped_ptr: UDPSocket socket(DatagramSocket::DEFAULT_BIND, ...); should also work.
Thanks for reviewing! Please note I disabled join group test for android because most android devices does not support receiving multicast socket. https://codereview.chromium.org/12684008/diff/130004/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.h (right): https://codereview.chromium.org/12684008/diff/130004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.h:45: int GetJoinedGroups(base::hash_set<std::string>* groups) const; None of the method is documented here. I guess it's because it is just a wrapper of another class. On 2013/04/17 22:06:50, wtc wrote: > > I think you should document these five new methods. This is > outside my area, so I don't know if your team requires this. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:491: uint8 loop = 0; Done. Now we use different branches for IPv4 and IPv6 so the following problems are prevented. On 2013/04/17 22:06:50, wtc wrote: > > BUG: it seems that this variable needs to be a u_int for > IPv6. See http://lists.ntp.org/pipermail/bugs/2011-May/013564.html > and > http://publib.boulder.ibm.com/infocenter/zos/v1r13/index.jsp?topic=%252Fcom.i... https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:492: int ip_family = On 2013/04/17 22:06:50, wtc wrote: > > This variable should be named 'level' or 'protocol_level' > instead of 'ip_family'. > See http://pubs.opengroup.org/onlinepubs/009695399/functions/setsockopt.html > > Make the same change to line 502. Done. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:505: addr_family_ == AF_INET ? IP_MULTICAST_TTL: IP_MULTICAST_TTL; On 2013/04/17 22:06:50, wtc wrote: > > BUG?: I think we need to use IPV6_MULTICAST_HOPS for IPv6. > Note that IPV6_MULTICAST_HOPS uses an 'int' for its option > value, according to > http://publib.boulder.ibm.com/infocenter/zos/v1r13/index.jsp?topic=%252Fcom.i... Done. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:134: // Should be called before Bind(). On 2013/04/17 22:06:50, wtc wrote: > > Add: Return a network error code. Done. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:226: // Cannot be used after Bind(). Yes, it's true this time (it wasn't). If write to this field after bind(), it will never be applied to the socket. On 2013/04/17 22:06:50, wtc wrote: > > "Cannot be used after Bind()": Is this still true? https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:534: const char* kGroup = "237.132.100.17"; On 2013/04/17 22:06:50, wtc wrote: > > This should be > const char* const kGroup = ...; > so that kGroup itself is constant (rather than the thing it > points to is constant). Done. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:535: net::IPAddressNumber group_ip; On 2013/04/17 22:06:50, wtc wrote: > > Remove net:: because this is inside the net namespace. Done. https://codereview.chromium.org/12684008/diff/130004/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:542: NetLog::Source())); On 2013/04/17 22:06:50, wtc wrote: > > Nit: it doesn't seem necessary to use a scoped_ptr: > > UDPSocket socket(DatagramSocket::DEFAULT_BIND, > ...); > > should also work. Done.
Patch set 10 LGTM. Thanks. Most of the changes I suggest are about style nits. The most serious problem is probably a potential flaky test. I marked that comment with "IMPORTANT". https://codereview.chromium.org/12684008/diff/130004/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.h (right): https://codereview.chromium.org/12684008/diff/130004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.h:45: int GetJoinedGroups(base::hash_set<std::string>* groups) const; On 2013/04/19 19:40:14, Bei Zhang wrote: > None of the method is documented here. I guess it's because it is just a wrapper > of another class. The existing methods are all implementations of base class methods (note that they all have the OVERRIDE keyword). They don't need to be documented in a subclass. The five new methods you added should be documented. Since this file is outside src/net, I won't insist that you document the new methods. You should follow the policy of the extensions team. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:265: return net::ERR_FAILED; Please use net::ERR_UNEXPECTED here and on line 271. In general we should use the most specific error code. ERR_FAILED is very broad ("the function failed"). ERR_UNEXPECTED means the function failed because of an unexpected reason (usually a programming error). It's still not ideal, but is better than ERR_FAILED. But, why don't you just let socket_.SetMulticastTimeToLive() and socket_.SetMulticastLoopbackMode() do this checking? https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:38: EXPECT_EQ(static_cast<int>(test_message_length), count); The static_cast<int> can be removed because you declare test_message_length as an int. Similarly for line 44. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:91: const char* group, int result) { It may be better to list one parameter per line. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:94: src->Write(data, test_message_length, base::Bind(&OnSendCompleted)); Should we check the return value of src->Write()? Same question for the dest->SendTo() call on lines 103-104. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:101: FAIL() << "Failed to recveive from multicast address"; Typo: recveive => receive https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:120: // Wait for 2 seconds for the JoinGroup to take effect. This comment says "2 seconds", but you seem to wait for 4 seconds (see line 125). IMPORTANT: this can be a source of flaky tests. Is there a way to wait until JoinGroup has taken effect, rather than waiting for a fixed time interval and hoping that JoinGroup has taken effect? https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:144: // Note: the behavior of <code>SetMulticastLoopbackMode</code> is slightly Nit: <code>SetMulticastLoopbackMode</code> => |SetMulticastLoopbackMode| Make the same change to udp_socket_win.h. In src/net comments, we often use "|foo|" to refer to a function or argument named "foo". https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:530: // Some Android devices does not support multicast socket. Typo: does not => do not socket => sockets https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:531: // Ones supporting multicast need WifiManager.MulitcastLock to enable it. Typo: Ones => The ones https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:536: #endif // defined(OS_ANDROID) This is usually done as follows: #if defined(OS_ANDROID) // Comments ... #define MAYBE_JoinMulticastGroup DISABLED_JoinMulticastGroup #else #define MAYBE_JoinMulticastGroup JoinMulticastGroup #endif TEST_F(UDPSocketTest, MAYBE_JoinMulticastGroup) { https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:552: EXPECT_NE(OK, socket.JoinGroup(group_ip)); Why can't we check for a specific error code here and on lines 555 and 574? This is probably not a big deal. I'm just curious.
Thanks for the review! About the MessageQueue issue, I'm so sorry to find out it was my misuse. I called FAIL() before QuitNow(). That's why it's not quitting. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:265: return net::ERR_FAILED; net::UDPSocket should be used internally. I think we should not check it in net::UDPSocket in the Release build because we can control the way we use it. So it only has a DCHECK on it. On 2013/04/22 19:19:53, wtc wrote: > > Please use net::ERR_UNEXPECTED here and on line 271. In general > we should use the most specific error code. ERR_FAILED is > very broad ("the function failed"). ERR_UNEXPECTED means > the function failed because of an unexpected reason (usually > a programming error). It's still not ideal, but is better > than ERR_FAILED. > > But, why don't you just let socket_.SetMulticastTimeToLive() > and socket_.SetMulticastLoopbackMode() do this checking? https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:36: static void OnReadCompleted(int count, Renamed to OnMulticastReadCompleted at line 102. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:38: EXPECT_EQ(static_cast<int>(test_message_length), count); On 2013/04/22 19:19:53, wtc wrote: > > The static_cast<int> can be removed because you declare > test_message_length as an int. > > Similarly for line 44. Done. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:91: const char* group, int result) { On 2013/04/22 19:19:53, wtc wrote: > > It may be better to list one parameter per line. Done. Renamed to SendMulticastPacket at line 87. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:94: src->Write(data, test_message_length, base::Bind(&OnSendCompleted)); extensions::Socket::Write and extensions::Socket::Read does not have a return value. The error code is return in the callback which is checked on line 44. On 2013/04/22 19:19:53, wtc wrote: > > Should we check the return value of src->Write()? > > Same question for the dest->SendTo() call on lines 103-104. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:100: void Timeout(UDPSocket* dest, const char* loopback_addr, int port) { Renamed to QuitMessageLoop at line 83. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:101: FAIL() << "Failed to recveive from multicast address"; On 2013/04/22 19:19:53, wtc wrote: > > Typo: recveive => receive Done. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:120: // Wait for 2 seconds for the JoinGroup to take effect. Done. On 2013/04/22 19:19:53, wtc wrote: > > This comment says "2 seconds", but you seem to wait for 4 > seconds (see line 125). > > IMPORTANT: this can be a source of flaky tests. Is there a > way to wait until JoinGroup has taken effect, rather than > waiting for a fixed time interval and hoping that JoinGroup > has taken effect? https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.h (right): https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.h:144: // Note: the behavior of <code>SetMulticastLoopbackMode</code> is slightly On 2013/04/22 19:19:53, wtc wrote: > > Nit: <code>SetMulticastLoopbackMode</code> => |SetMulticastLoopbackMode| > > Make the same change to udp_socket_win.h. > > In src/net comments, we often use "|foo|" to refer to a > function or argument named "foo". Done. https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:530: // Some Android devices does not support multicast socket. On 2013/04/22 19:19:53, wtc wrote: > > Typo: does not => do not > socket => sockets Done. https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:531: // Ones supporting multicast need WifiManager.MulitcastLock to enable it. On 2013/04/22 19:19:53, wtc wrote: > > Typo: Ones => The ones Done. https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:536: #endif // defined(OS_ANDROID) On 2013/04/22 19:19:53, wtc wrote: > > This is usually done as follows: > > #if defined(OS_ANDROID) > // Comments ... > #define MAYBE_JoinMulticastGroup DISABLED_JoinMulticastGroup > #else > #define MAYBE_JoinMulticastGroup JoinMulticastGroup > #endif > > TEST_F(UDPSocketTest, MAYBE_JoinMulticastGroup) { Done. https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:552: EXPECT_NE(OK, socket.JoinGroup(group_ip)); The setsockopt function creates different errno on different OSes. Do you think we should map it manually to get a consistent error code? On 2013/04/22 19:19:53, wtc wrote: > > Why can't we check for a specific error code here and on > lines 555 and 574? > > This is probably not a big deal. I'm just curious.
Patch set 11 LGTM. https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/122028/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:265: return net::ERR_FAILED; On 2013/04/23 17:26:53, Bei Zhang wrote: > net::UDPSocket should be used internally. I think we should not check it in > net::UDPSocket in the Release build because we can control the way we use it. So > it only has a DCHECK on it. I see. Thank you for the explanation. net::UDPSocket can be used by multiple components of Chrome. Although net::UDPSocket is an internal class to the extensions component, other components may not use it this way. So I think it is better for net::UDPSocket to do proper error checking, rather than relying on DCHECK only. https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/122028/net/udp/udp_socket_unitt... net/udp/udp_socket_unittest.cc:552: EXPECT_NE(OK, socket.JoinGroup(group_ip)); On 2013/04/23 17:26:53, Bei Zhang wrote: > The setsockopt function creates different errno on different OSes. Do you think > we should map it manually to get a consistent error code? It is not necessary to get a consistent error code across different OSes, but if it is convenient to do so, we should do that. Do you remember what the error codes are? I am curious to know. https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:87: static void SendMulticastPacket(UDPSocket* src, This method should be documented. It is not obvious from the name "SendMulticastPacket" that you are just sending a test packet, and that you are sending it repeatedly every one second until it is received. https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:88: const char* group, The |group| argument is not used. Can we remove it? https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:95: base::TimeDelta::FromSeconds(1)); Do we need to cancel this delayed task if it is no longer necessary? https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:102: static void OnMulticastReadCompleted(bool *packet_received, |packet_received| is an output argument. Our Style Guide recommends that we list it after the input arguments. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param... https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:128: // If not received in 4 seconds quit the message loop. This should be increased to a very large number, such as 30 seconds or 60 seconds. You may want to consult the maintainer of try bots to find out how long they allow each unit test to take. Then, use a timeout equal or close to that timeout. https://codereview.chromium.org/12684008/diff/104004/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/104004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:616: DCHECK(!is_connected()); As noted elsewhere, I think these DCHECK(!is_connected()) can be replaced by proper error checking since the extensions subsystem need it. Make the same change to UDPSocketWin.
https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:87: static void SendMulticastPacket(UDPSocket* src, On 2013/04/23 18:01:07, wtc wrote: > > This method should be documented. It is not obvious from > the name "SendMulticastPacket" that you are just sending a > test packet, and that you are sending it repeatedly every > one second until it is received. Done. https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:88: const char* group, On 2013/04/23 18:01:07, wtc wrote: > > The |group| argument is not used. Can we remove it? Done. https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:95: base::TimeDelta::FromSeconds(1)); We do not need to cancel the task. It will not be invoked once we call QuitNow(). On 2013/04/23 18:01:07, wtc wrote: > > Do we need to cancel this delayed task if it is no longer > necessary? https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:102: static void OnMulticastReadCompleted(bool *packet_received, Unfortunately we cannot bind argument to the end of the arguments list of a closure. Would you like to make it a static global variable? On 2013/04/23 18:01:07, wtc wrote: > > |packet_received| is an output argument. Our Style Guide > recommends that we list it after the input arguments. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param... https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:128: // If not received in 4 seconds quit the message loop. Done. I found out that there is no per tests case time limit. So you were right the 75 second should be a system limit. On 2013/04/23 18:01:07, wtc wrote: > > This should be increased to a very large number, such as 30 > seconds or 60 seconds. You may want to consult the maintainer > of try bots to find out how long they allow each unit test to > take. Then, use a timeout equal or close to that timeout. https://codereview.chromium.org/12684008/diff/104004/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/104004/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:616: DCHECK(!is_connected()); On 2013/04/23 18:01:07, wtc wrote: > > As noted elsewhere, I think these DCHECK(!is_connected()) > can be replaced by proper error checking since the > extensions subsystem need it. > > Make the same change to UDPSocketWin. Done.
Patch set 12 LGTM. Thanks. https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/104004/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:102: static void OnMulticastReadCompleted(bool *packet_received, On 2013/04/23 20:36:39, Bei Zhang wrote: > Unfortunately we cannot bind argument to the end of the arguments list of a > closure. Would you like to make it a static global variable? Thank you for the explanation. (I am not familiar with closures.) You don't need to change this to a static global variable. https://codereview.chromium.org/12684008/diff/185001/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/185001/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:88: // Send multicast packet every second. Nit: add "a" before "multicast packet". I would also suggest saying "a test multicast packet". https://codereview.chromium.org/12684008/diff/185001/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:130: // If not received in action timeout, quit the message loop. Nit: "in action timeout" sounds a little strange. Perhaps "within the test action timeout"? https://codereview.chromium.org/12684008/diff/185001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/185001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:546: if(!is_connected()) Add a space between "if" and the opening parenthesis "(". This is recommended by the Style Guide. Make the same change to the other three if statements you added, and also to udp_socket_win.cc. https://codereview.chromium.org/12684008/diff/185001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:620: return ERR_UNEXPECTED; We should add a new error code ERR_SOCKET_IS_CONNECTED. Let's do that in a separate CL.
https://codereview.chromium.org/12684008/diff/185001/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/185001/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:88: // Send multicast packet every second. On 2013/04/23 22:15:26, wtc wrote: > > Nit: add "a" before "multicast packet". > > I would also suggest saying "a test multicast packet". Done. https://codereview.chromium.org/12684008/diff/185001/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:130: // If not received in action timeout, quit the message loop. On 2013/04/23 22:15:26, wtc wrote: > > Nit: "in action timeout" sounds a little strange. Perhaps > "within the test action timeout"? Done. https://codereview.chromium.org/12684008/diff/185001/net/udp/udp_socket_libev... File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/12684008/diff/185001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:546: if(!is_connected()) On 2013/04/23 22:15:26, wtc wrote: > > Add a space between "if" and the opening parenthesis "(". > This is recommended by the Style Guide. > > Make the same change to the other three if statements you > added, and also to udp_socket_win.cc. Done. https://codereview.chromium.org/12684008/diff/185001/net/udp/udp_socket_libev... net/udp/udp_socket_libevent.cc:620: return ERR_UNEXPECTED; Created a bug at http://crbug.com/235066 . On 2013/04/23 22:15:26, wtc wrote: > > We should add a new error code ERR_SOCKET_IS_CONNECTED. > Let's do that in a separate CL.
Patch set 13 LGTM.
https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.cc:46: const char kWildcardArress[] = "*"; I think this word is supposed to be "Address" https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.cc:47: const int kWildcardPortNumber = 0; "Port Number" is overly descriptive. Just "Port" will work in this context. https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.cc:715: SocketPermissionRequest::UDP_MUTICAST_MEMBERSHIP, Spelling: MULTICAST https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.cc:724: } else { I wouldn't put this in an else, because it's going to lead to needless nesting of the main block of code. Something more like this would make sense: if (!socket) exit if (!TYPE_UDP) exit doAllTheWork https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.cc:759: if (socket->GetSocketType() == Socket::TYPE_UDP) { Same here -- rearrange so obvious validation failures are handled immediately, then let the normal case be a top-level block in the method. https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.cc:801: if (socket->GetSocketType() == Socket::TYPE_UDP) { There's a bunch of boilerplate code here. What about a single utility method like "GetUDPSocketWithValidPermission" that does all these tests? https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.cc:889: list->AppendString(*it); Please confirm that AppendString does a copy of the string. Some Value methods take ownership of the object, which doesn't seem to be what you'd want here. https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... File chrome/browser/extensions/api/socket/socket_api.h (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/socket_api.h:354: DECLARE_EXTENSION_FUNCTION("socket.joinGroup", SOCKET_MUILTICAST_JOIN_GROUP) Spelling: MULTICAST (please replace all misspellings) https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/udp_socket.cc:267: int UDPSocket::GetJoinedGroups(base::hash_set<std::string>* groups) const { Same here -- why hash_set? https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/udp_socket.cc:271: groups->clear(); This method signature is a bit odd. Why require the caller to pass in a hash_set if you're going to clear it every time? Why not just return a new hash_set? https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... File chrome/browser/extensions/api/socket/udp_socket.h (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/udp_socket.h:71: base::hash_set<std::string> multicast_groups_; Why a hash_set and not a set? https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:91: int result) { no need to wrap https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... File chrome/browser/extensions/extension_function_histogram_value.h (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/browser/ex... chrome/browser/extensions/extension_function_histogram_value.h:527: SOCKET_MUILTICAST_GET_JOINED_GROUPS, spelling https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... File chrome/common/extensions/api/socket.idl (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:265: // Join the multicast group and start to receive diagrams from that group. "diagrams" is probably not the right word. https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:277: // It's not nessesary to leave the multicast group before destroying the spelling: necessary https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:297: // |callback| : Called when the configuretion operation is done. spelling: configuration https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:303: // Set the whether multicast diagrams sent from the host to the multicast remove the first "the" https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:307: // different between Windows and Unix-like systems. The inconsistency only happens only (not only happens) https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:308: // happens when there are more than one applications on the same host fix pluralization of "applications" https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:308: // happens when there are more than one applications on the same host there is, not there are ("there is ... one application") -- see http://english.stackexchange.com/a/35998 https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:313: // other applications on the same host. See MSDN: http://goo.gl/6vqbj Hmmmm. This is a problem. The app writer will never ever ever know whether the app is running on a specific OS. So what good is this documentation? https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:318: // |enabled| : Indicate whether to enable loop back mode. Be consistent: either "loop back" or "loopback" (preferred) https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/api/socket.idl:319: // |callback| : Called when the configuretion operation is done. spelling https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... File chrome/common/extensions/permissions/socket_permission_data.cc (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/common/ext... chrome/common/extensions/permissions/socket_permission_data.cc:61: case SocketPermissionRequest::UDP_MUTICAST_MEMBERSHIP: spelling (fix everywhere, please) https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/test/data/... File chrome/test/data/extensions/api_test/socket/api/multicast.js (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/chrome/test/data/... chrome/test/data/extensions/api_test/socket/api/multicast.js:164: chrome.test.fail("Recieved message after leaving the group"); spelling https://chromiumcodereview.appspot.com/12684008/diff/108002/content/public/co... File content/public/common/socket_permission_request.h (right): https://chromiumcodereview.appspot.com/12684008/diff/108002/content/public/co... content/public/common/socket_permission_request.h:21: UDP_MUTICAST_MEMBERSHIP spelling!
Thanks for the review! https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:46: const char kWildcardArress[] = "*"; On 2013/04/25 21:52:12, miket wrote: > I think this word is supposed to be "Address" Done. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:47: const int kWildcardPortNumber = 0; On 2013/04/25 21:52:12, miket wrote: > "Port Number" is overly descriptive. Just "Port" will work in this context. Done. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:715: SocketPermissionRequest::UDP_MUTICAST_MEMBERSHIP, On 2013/04/25 21:52:12, miket wrote: > Spelling: MULTICAST Done. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:724: } else { On 2013/04/25 21:52:12, miket wrote: > I wouldn't put this in an else, because it's going to lead to needless nesting > of the main block of code. Something more like this would make sense: > > if (!socket) > exit > if (!TYPE_UDP) > exit > doAllTheWork Done. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:759: if (socket->GetSocketType() == Socket::TYPE_UDP) { On 2013/04/25 21:52:12, miket wrote: > Same here -- rearrange so obvious validation failures are handled immediately, > then let the normal case be a top-level block in the method. Done. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:801: if (socket->GetSocketType() == Socket::TYPE_UDP) { That'll be ideal. But I think maybe we should do it in a separate CL because actually all the code in this file are already written this way. On 2013/04/25 21:52:12, miket wrote: > There's a bunch of boilerplate code here. What about a single utility method > like "GetUDPSocketWithValidPermission" that does all these tests? https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:889: list->AppendString(*it); The copy cannot be avoided by changing the ownership of the string. It will be copied anyway in the constructor of StringValue. However, we can change the interface of UDPSocket to accept ListValue* instead of hash_set<std::string> to avoid the creation of std::string. On 2013/04/25 21:52:12, miket wrote: > Please confirm that AppendString does a copy of the string. Some Value methods > take ownership of the object, which doesn't seem to be what you'd want here. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:354: DECLARE_EXTENSION_FUNCTION("socket.joinGroup", SOCKET_MUILTICAST_JOIN_GROUP) On 2013/04/25 21:52:12, miket wrote: > Spelling: MULTICAST (please replace all misspellings) Done. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:271: groups->clear(); To avoid the copy of the container. Clear would be fast if the container is empty in the first place. On 2013/04/25 21:52:12, miket wrote: > This method signature is a bit odd. Why require the caller to pass in a hash_set > if you're going to clear it every time? Why not just return a new hash_set? https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.h (right): https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.h:71: base::hash_set<std::string> multicast_groups_; hash set is faster for strings. A vector would be faster if we are storing numbers. On 2013/04/25 21:52:12, miket wrote: > Why a hash_set and not a set? https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right): https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket_unittest.cc:91: int result) { On 2013/04/25 21:52:12, miket wrote: > no need to wrap Done. https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... File chrome/browser/extensions/extension_function_histogram_value.h (right): https://codereview.chromium.org/12684008/diff/108002/chrome/browser/extension... chrome/browser/extensions/extension_function_histogram_value.h:527: SOCKET_MUILTICAST_GET_JOINED_GROUPS, On 2013/04/25 21:52:12, miket wrote: > spelling Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:265: // Join the multicast group and start to receive diagrams from that group. On 2013/04/25 21:52:12, miket wrote: > "diagrams" is probably not the right word. Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:277: // It's not nessesary to leave the multicast group before destroying the On 2013/04/25 21:52:12, miket wrote: > spelling: necessary Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:297: // |callback| : Called when the configuretion operation is done. On 2013/04/25 21:52:12, miket wrote: > spelling: configuration Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:297: // |callback| : Called when the configuretion operation is done. On 2013/04/25 21:52:12, miket wrote: > spelling: configuration Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:303: // Set the whether multicast diagrams sent from the host to the multicast On 2013/04/25 21:52:12, miket wrote: > remove the first "the" Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:307: // different between Windows and Unix-like systems. The inconsistency only On 2013/04/25 21:52:12, miket wrote: > happens only (not only happens) Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:308: // happens when there are more than one applications on the same host On 2013/04/25 21:52:12, miket wrote: > fix pluralization of "applications" Wow thanks! this is really helpful! Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:308: // happens when there are more than one applications on the same host On 2013/04/25 21:52:12, miket wrote: > fix pluralization of "applications" Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:313: // other applications on the same host. See MSDN: http://goo.gl/6vqbj wtc concerned that we may not be able to make this behavior consistent ever. So instead of making developers pull their hairs off finding bugs, at least we should give them a hint. If this really makes a problem, they should start to consider a way to workaround, for example, filter the packets manually. On 2013/04/25 21:52:12, miket wrote: > Hmmmm. This is a problem. The app writer will never ever ever know whether the > app is running on a specific OS. So what good is this documentation? https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:318: // |enabled| : Indicate whether to enable loop back mode. On 2013/04/25 21:52:12, miket wrote: > Be consistent: either "loop back" or "loopback" (preferred) Done. https://codereview.chromium.org/12684008/diff/108002/chrome/common/extensions... chrome/common/extensions/api/socket.idl:319: // |callback| : Called when the configuretion operation is done. On 2013/04/25 21:52:12, miket wrote: > spelling Done. https://codereview.chromium.org/12684008/diff/108002/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/socket/api/multicast.js (right): https://codereview.chromium.org/12684008/diff/108002/chrome/test/data/extensi... chrome/test/data/extensions/api_test/socket/api/multicast.js:164: chrome.test.fail("Recieved message after leaving the group"); On 2013/04/25 21:52:12, miket wrote: > spelling Done. https://codereview.chromium.org/12684008/diff/108002/content/public/common/so... File content/public/common/socket_permission_request.h (right): https://codereview.chromium.org/12684008/diff/108002/content/public/common/so... content/public/common/socket_permission_request.h:21: UDP_MUTICAST_MEMBERSHIP On 2013/04/25 21:52:12, miket wrote: > spelling! Done.
LGTM but please see comments below. > That'll be ideal. But I think maybe we should do it in a separate CL because > actually all the code in this file are already written this way. Please add a TODO for yourself to remember to clean up this code. > base::hash_set<std::string> multicast_groups_; > hash set is faster for strings. A vector would be faster if we are storing > numbers. Hmmm. I don't think speed is an issue here given that this isn't a method that will be called frequently and given that the number of elements in this set is likely to be very small, but I suppose it's not a big deal if this remains as a private implementation detail (contrast new comments about passing it in the method signature). https://codereview.chromium.org/12684008/diff/222003/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/222003/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:255: multicast_groups_.erase(find_result); You don't need to do the find/erase here. You can simply erase(normalized_address) and return the error if erase() returns 0. https://codereview.chromium.org/12684008/diff/222003/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:267: int UDPSocket::GetJoinedGroups(base::hash_set<std::string>* groups) const { The hash_set really doesn't make any sense here. In every use case you're just copying a list and never doing any lookups. Use a vector.
Thanks for the review! https://codereview.chromium.org/12684008/diff/222003/chrome/browser/extension... File chrome/browser/extensions/api/socket/udp_socket.cc (right): https://codereview.chromium.org/12684008/diff/222003/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:255: multicast_groups_.erase(find_result); We are changing to a vector. Have to lookup the iterator this time. On 2013/04/26 01:20:01, miket wrote: > You don't need to do the find/erase here. You can simply > erase(normalized_address) and return the error if erase() returns 0. https://codereview.chromium.org/12684008/diff/222003/chrome/browser/extension... chrome/browser/extensions/api/socket/udp_socket.cc:267: int UDPSocket::GetJoinedGroups(base::hash_set<std::string>* groups) const { I Agree. For our case, I think vector is our best choice. On 2013/04/26 01:20:01, miket wrote: > The hash_set really doesn't make any sense here. In every use case you're just > copying a list and never doing any lookups. Use a vector.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/12684008/238001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/12684008/240003
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ikarienator@chromium.org/12684008/240003
Message was sent while issue was closed.
Change committed as 197192 |
