Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10916052:
Fixed UDP tests for MacOS X 10.7 (Closed)
|
Created:
8 years, 3 months ago by ygorshenin1 Modified:
8 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Dmitry Polukhin Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixed UDP unittests for MacOS X 10.7
Fixed Connect and VerifyConnectBindsAddr unittests.
As broadcast option is blocked for regular users on OSX, added check for UID to Broadcast unittest.
Also, disabled browser_test:UDPSocketPrivate_Broadcast as it's compiled via NaCl toolchain, so we can't recognize current OS being inside that test.
BUG=145487
TEST=net_unittests:UDPSocketTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157399
Patch Set 1 #Patch Set 2 : Sync, fix. #Patch Set 3 : Fix. #Patch Set 4 : Fix #Patch Set 5 : Disabled UDPSocketPrivate::TestBroadcast on Mac. #Patch Set 6 : Fix. #
Total comments: 4
Patch Set 7 : Fix. #
Total comments: 6
Patch Set 8 : Added comments. #
Total comments: 2
Patch Set 9 : Fixed comments. #
Total comments: 5
Patch Set 10 : UDPSocketTest.Broadcast is fully disabled on OSX #Patch Set 11 : Sync. #
Messages
Total messages: 26 (0 generated)
I'm not sufficiently familiar with UDP to review this, and the fact that you're adding calls to server->AllowAddressReuse(), which results in a call to setsockopt, which apparently requires admin privileges on 10.7, confuses me. I suggest you find a better reviewer. You'll also need a chrome/test/ppapi OWNER.
+ Sergey
The description of this CL is confusing. The problem is not that setsockopt() requires root permissions, it's just that broadcast is blocked for regular users. LGTM once description is updated and my other comments are addressed. http://codereview.chromium.org/10916052/diff/13006/net/udp/udp_socket_unittes... File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/13006/net/udp/udp_socket_unittes... net/udp/udp_socket_unittest.cc:207: // TODO (ygorshenin): crbug.com/145487 It's better to keep the test but just skip this test when not running as root, e.g. #if !defined(OS_MACOSX) if (getuid() != 0) { LOG(ERROR) << "Skipping Broadcast test because it requires root permissions on OSX"; return; } #endif // !defined (OS_MAXOSX) http://codereview.chromium.org/10916052/diff/13006/net/udp/udp_socket_unittes... net/udp/udp_socket_unittest.cc:247: #endif add // !defined (OS_MAXOSX)
PTAL http://codereview.chromium.org/10916052/diff/13006/net/udp/udp_socket_unittes... File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/13006/net/udp/udp_socket_unittes... net/udp/udp_socket_unittest.cc:207: // TODO (ygorshenin): crbug.com/145487 On 2012/09/04 21:27:32, sergeyu wrote: > It's better to keep the test but just skip this test when not running as root, > e.g. > #if !defined(OS_MACOSX) > if (getuid() != 0) { > LOG(ERROR) << "Skipping Broadcast test because it requires root permissions > on OSX"; > return; > } > #endif // !defined (OS_MAXOSX) Done. http://codereview.chromium.org/10916052/diff/13006/net/udp/udp_socket_unittes... net/udp/udp_socket_unittest.cc:247: #endif On 2012/09/04 21:27:32, sergeyu wrote: > add // !defined (OS_MAXOSX) Done.
couple more style nits. LGTM otherwise. http://codereview.chromium.org/10916052/diff/4010/chrome/test/ppapi/ppapi_bro... File chrome/test/ppapi/ppapi_browsertest.cc (right): http://codereview.chromium.org/10916052/diff/4010/chrome/test/ppapi/ppapi_bro... chrome/test/ppapi/ppapi_browsertest.cc:293: #if !defined(OS_MACOSX) Please add comment to explain why this test is disabled on Mac. http://codereview.chromium.org/10916052/diff/4010/chrome/test/ppapi/ppapi_bro... chrome/test/ppapi/ppapi_browsertest.cc:295: #endif nit: // defined(OS_MACOSX) http://codereview.chromium.org/10916052/diff/4010/net/udp/udp_socket_unittest.cc File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/4010/net/udp/udp_socket_unittest... net/udp/udp_socket_unittest.cc:6: #include <unistd.h> You need to move this after udp_server_socket.h and before base/basictypes.h include below.
Thanks, Sergey! + Trung for chrome/test/ppapi* Matt, could you please give an OWNER's LGTM for net/*? http://codereview.chromium.org/10916052/diff/4010/chrome/test/ppapi/ppapi_bro... File chrome/test/ppapi/ppapi_browsertest.cc (right): http://codereview.chromium.org/10916052/diff/4010/chrome/test/ppapi/ppapi_bro... chrome/test/ppapi/ppapi_browsertest.cc:293: #if !defined(OS_MACOSX) On 2012/09/05 22:23:56, sergeyu wrote: > Please add comment to explain why this test is disabled on Mac. Done. http://codereview.chromium.org/10916052/diff/4010/chrome/test/ppapi/ppapi_bro... chrome/test/ppapi/ppapi_browsertest.cc:295: #endif On 2012/09/05 22:23:56, sergeyu wrote: > nit: // defined(OS_MACOSX) Done. http://codereview.chromium.org/10916052/diff/4010/net/udp/udp_socket_unittest.cc File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/4010/net/udp/udp_socket_unittest... net/udp/udp_socket_unittest.cc:6: #include <unistd.h> On 2012/09/05 22:23:56, sergeyu wrote: > You need to move this after udp_server_socket.h and before base/basictypes.h > include below. Done.
net/ LGTM http://codereview.chromium.org/10916052/diff/5004/net/udp/udp_socket_unittest.cc File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/5004/net/udp/udp_socket_unittest... net/udp/udp_socket_unittest.cc:213: "Skipping Broadcast test because it requires root permissions on OSX"; nit: Think "Skipping test, as UDP broadcast requires root permissions on OSX 10.8+." would be clearer. May also want to update your comment in the other file along the same lines.
Thanks, PTAL. http://codereview.chromium.org/10916052/diff/5004/net/udp/udp_socket_unittest.cc File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/5004/net/udp/udp_socket_unittest... net/udp/udp_socket_unittest.cc:213: "Skipping Broadcast test because it requires root permissions on OSX"; On 2012/09/06 14:29:07, Matt Menke wrote: > nit: Think "Skipping test, as UDP broadcast requires root permissions on OSX > 10.8+." would be clearer. May also want to update your comment in the other > file along the same lines. Done.
LGTM
Trung, could you please give an OWNER's LGTM for chrome/test/ppapi?
http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... File chrome/test/ppapi/ppapi_browsertest.cc (right): http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... chrome/test/ppapi/ppapi_browsertest.cc:291: // UDPSocketPrivate_Broadcast is disabled for OSX because it requires Maybe it's better to just remove the broadcast test from TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of course mean never testing broadcast in NaCl -- do we really need broadcast in NaCl? Especially since it's apparently no longer a platform-independent capability?) (What you've done here is not only rather messy, but there's also high probability that anyone adding new UDP tests will forget to add them here; keeping the list of tests in sync is a pain.) http://codereview.chromium.org/10916052/diff/16003/net/udp/udp_socket_unittes... File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/16003/net/udp/udp_socket_unittes... net/udp/udp_socket_unittest.cc:210: #if defined(OS_MACOSX) Do you really expect that we'll ever run this test as root? If not, maybe we should just disable this test (via conditionally marking it as "DISABLED_Broadcast") on Mac?
http://codereview.chromium.org/10916052/diff/16003/net/udp/udp_socket_unittes... File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/16003/net/udp/udp_socket_unittes... net/udp/udp_socket_unittest.cc:210: #if defined(OS_MACOSX) On 2012/09/10 17:51:10, viettrungluu wrote: > Do you really expect that we'll ever run this test as root? > > If not, maybe we should just disable this test (via conditionally marking it as > "DISABLED_Broadcast") on Mac? I agree that this is better practice.
PTAL http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... File chrome/test/ppapi/ppapi_browsertest.cc (right): http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... chrome/test/ppapi/ppapi_browsertest.cc:291: // UDPSocketPrivate_Broadcast is disabled for OSX because it requires From NaCl broadcasting is not a capability --- it's a special "feature" which is handled individually for Mac, Win and Linux. Take a look at http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_libeven... and http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_win.cc&.... On 2012/09/10 17:51:10, viettrungluu wrote: > Maybe it's better to just remove the broadcast test from > TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of course mean > never testing broadcast in NaCl -- do we really need broadcast in NaCl? > Especially since it's apparently no longer a platform-independent capability?) > > (What you've done here is not only rather messy, but there's also high > probability that anyone adding new UDP tests will forget to add them here; > keeping the list of tests in sync is a pain.)
PTAL http://codereview.chromium.org/10916052/diff/16003/net/udp/udp_socket_unittes... File net/udp/udp_socket_unittest.cc (right): http://codereview.chromium.org/10916052/diff/16003/net/udp/udp_socket_unittes... net/udp/udp_socket_unittest.cc:210: #if defined(OS_MACOSX) On 2012/09/10 17:53:29, Matt Menke wrote: > On 2012/09/10 17:51:10, viettrungluu wrote: > > Do you really expect that we'll ever run this test as root? > > > > If not, maybe we should just disable this test (via conditionally marking it > as > > "DISABLED_Broadcast") on Mac? > > I agree that this is better practice. Done.
On 2012/09/10 18:05:22, ygorshenin1 wrote: > PTAL > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > File chrome/test/ppapi/ppapi_browsertest.cc (right): > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > chrome/test/ppapi/ppapi_browsertest.cc:291: // UDPSocketPrivate_Broadcast is > disabled for OSX because it requires > From NaCl broadcasting is not a capability --- it's a special "feature" which is > handled individually for Mac, Win and Linux. Take a look at > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_libeven... > and > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_win.cc&.... I'm not quite sure I follow. Is broadcasting supported from a NaCl plugin or not? (If it's not going to be supported on Mac, then should we support it at all? And if we don't support it, we probably shouldn't test it; probably we should disable it for all platforms.) > > On 2012/09/10 17:51:10, viettrungluu wrote: > > Maybe it's better to just remove the broadcast test from > > TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of course mean > > never testing broadcast in NaCl -- do we really need broadcast in NaCl? > > Especially since it's apparently no longer a platform-independent capability?) > > > > (What you've done here is not only rather messy, but there's also high > > probability that anyone adding new UDP tests will forget to add them here; > > keeping the list of tests in sync is a pain.)
Broadcasting is supported from a NaCl plugin. It can be tested on OSX 10.6, but it requires admin privileges on OSX 10.7. As you can see from discussion, it was decided to simply disable corresponding net_unittests/browser tests for OSX. But this doesn't mean that we should switch off broadcasting feature, because we need this one for several NaCl applications. On 2012/09/11 17:18:43, viettrungluu wrote: > On 2012/09/10 18:05:22, ygorshenin1 wrote: > > PTAL > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > File chrome/test/ppapi/ppapi_browsertest.cc (right): > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > chrome/test/ppapi/ppapi_browsertest.cc:291: // UDPSocketPrivate_Broadcast is > > disabled for OSX because it requires > > From NaCl broadcasting is not a capability --- it's a special "feature" which > is > > handled individually for Mac, Win and Linux. Take a look at > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_libeven... > > and > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_win.cc&.... > > I'm not quite sure I follow. Is broadcasting supported from a NaCl plugin or > not? (If it's not going to be supported on Mac, then should we support it at > all? And if we don't support it, we probably shouldn't test it; probably we > should disable it for all platforms.) > > > > > On 2012/09/10 17:51:10, viettrungluu wrote: > > > Maybe it's better to just remove the broadcast test from > > > TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of course mean > > > never testing broadcast in NaCl -- do we really need broadcast in NaCl? > > > Especially since it's apparently no longer a platform-independent > capability?) > > > > > > (What you've done here is not only rather messy, but there's also high > > > probability that anyone adding new UDP tests will forget to add them here; > > > keeping the list of tests in sync is a pain.)
Trung, friendly ping. On 2012/09/12 11:30:55, ygorshenin1 wrote: > Broadcasting is supported from a NaCl plugin. It can be tested on OSX 10.6, but > it requires admin privileges on OSX 10.7. As you can see from discussion, it was > decided to simply disable corresponding net_unittests/browser tests for OSX. But > this doesn't mean that we should switch off broadcasting feature, because we > need this one for several NaCl applications. > > On 2012/09/11 17:18:43, viettrungluu wrote: > > On 2012/09/10 18:05:22, ygorshenin1 wrote: > > > PTAL > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > File chrome/test/ppapi/ppapi_browsertest.cc (right): > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > chrome/test/ppapi/ppapi_browsertest.cc:291: // UDPSocketPrivate_Broadcast is > > > disabled for OSX because it requires > > > From NaCl broadcasting is not a capability --- it's a special "feature" > which > > is > > > handled individually for Mac, Win and Linux. Take a look at > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_libeven... > > > and > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_win.cc&.... > > > > I'm not quite sure I follow. Is broadcasting supported from a NaCl plugin or > > not? (If it's not going to be supported on Mac, then should we support it at > > all? And if we don't support it, we probably shouldn't test it; probably we > > should disable it for all platforms.) > > > > > > > > On 2012/09/10 17:51:10, viettrungluu wrote: > > > > Maybe it's better to just remove the broadcast test from > > > > TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of course > mean > > > > never testing broadcast in NaCl -- do we really need broadcast in NaCl? > > > > Especially since it's apparently no longer a platform-independent > > capability?) > > > > > > > > (What you've done here is not only rather messy, but there's also high > > > > probability that anyone adding new UDP tests will forget to add them here; > > > > keeping the list of tests in sync is a pain.)
On 2012/09/18 11:26:50, ygorshenin1 wrote: > Trung, friendly ping. > > On 2012/09/12 11:30:55, ygorshenin1 wrote: > > Broadcasting is supported from a NaCl plugin. It can be tested on OSX 10.6, > but > > it requires admin privileges on OSX 10.7. As you can see from discussion, it > was > > decided to simply disable corresponding net_unittests/browser tests for OSX. > But > > this doesn't mean that we should switch off broadcasting feature, because we > > need this one for several NaCl applications. LGTM, I guess, though I wonder how we're supposedly going to support UDP broadcast in NaCl if it won't work on Mac OS 10.7+.... > > > > On 2012/09/11 17:18:43, viettrungluu wrote: > > > On 2012/09/10 18:05:22, ygorshenin1 wrote: > > > > PTAL > > > > > > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > > File chrome/test/ppapi/ppapi_browsertest.cc (right): > > > > > > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > > chrome/test/ppapi/ppapi_browsertest.cc:291: // UDPSocketPrivate_Broadcast > is > > > > disabled for OSX because it requires > > > > From NaCl broadcasting is not a capability --- it's a special "feature" > > which > > > is > > > > handled individually for Mac, Win and Linux. Take a look at > > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_libeven... > > > > and > > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_win.cc&.... > > > > > > I'm not quite sure I follow. Is broadcasting supported from a NaCl plugin or > > > not? (If it's not going to be supported on Mac, then should we support it at > > > all? And if we don't support it, we probably shouldn't test it; probably we > > > should disable it for all platforms.) > > > > > > > > > > > On 2012/09/10 17:51:10, viettrungluu wrote: > > > > > Maybe it's better to just remove the broadcast test from > > > > > TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of course > > mean > > > > > never testing broadcast in NaCl -- do we really need broadcast in NaCl? > > > > > Especially since it's apparently no longer a platform-independent > > > capability?) > > > > > > > > > > (What you've done here is not only rather messy, but there's also high > > > > > probability that anyone adding new UDP tests will forget to add them > here; > > > > > keeping the list of tests in sync is a pain.)
Broadcasting *works* on Mac. But it requires admin privileges. So if mac user will run nacl plugin which sends some broadcasting messages, the system will prompt for root password. On 2012/09/18 15:36:28, viettrungluu wrote: > On 2012/09/18 11:26:50, ygorshenin1 wrote: > > Trung, friendly ping. > > > > On 2012/09/12 11:30:55, ygorshenin1 wrote: > > > Broadcasting is supported from a NaCl plugin. It can be tested on OSX 10.6, > > but > > > it requires admin privileges on OSX 10.7. As you can see from discussion, it > > was > > > decided to simply disable corresponding net_unittests/browser tests for OSX. > > But > > > this doesn't mean that we should switch off broadcasting feature, because we > > > need this one for several NaCl applications. > > LGTM, I guess, though I wonder how we're supposedly going to support UDP > broadcast in NaCl if it won't work on Mac OS 10.7+.... > > > > > > > On 2012/09/11 17:18:43, viettrungluu wrote: > > > > On 2012/09/10 18:05:22, ygorshenin1 wrote: > > > > > PTAL > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > > > File chrome/test/ppapi/ppapi_browsertest.cc (right): > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > > > chrome/test/ppapi/ppapi_browsertest.cc:291: // > UDPSocketPrivate_Broadcast > > is > > > > > disabled for OSX because it requires > > > > > From NaCl broadcasting is not a capability --- it's a special "feature" > > > which > > > > is > > > > > handled individually for Mac, Win and Linux. Take a look at > > > > > > > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_libeven... > > > > > and > > > > > > > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_win.cc&.... > > > > > > > > I'm not quite sure I follow. Is broadcasting supported from a NaCl plugin > or > > > > not? (If it's not going to be supported on Mac, then should we support it > at > > > > all? And if we don't support it, we probably shouldn't test it; probably > we > > > > should disable it for all platforms.) > > > > > > > > > > > > > > On 2012/09/10 17:51:10, viettrungluu wrote: > > > > > > Maybe it's better to just remove the broadcast test from > > > > > > TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of > course > > > mean > > > > > > never testing broadcast in NaCl -- do we really need broadcast in > NaCl? > > > > > > Especially since it's apparently no longer a platform-independent > > > > capability?) > > > > > > > > > > > > (What you've done here is not only rather messy, but there's also high > > > > > > probability that anyone adding new UDP tests will forget to add them > > here; > > > > > > keeping the list of tests in sync is a pain.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/10916052/5007
Failed to apply patch for net/udp/udp_socket_unittest.cc: While running patch -p1 --forward --force; patching file net/udp/udp_socket_unittest.cc Hunk #1 succeeded at 131 with fuzz 2. Hunk #3 FAILED at 342. 1 out of 3 hunks FAILED -- saving rejects to file net/udp/udp_socket_unittest.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/10916052/19002
Change committed as 157399
On 2012/09/18 17:00:04, ygorshenin1 wrote: > Broadcasting *works* on Mac. But it requires admin privileges. So if mac user > will run nacl plugin which sends some broadcasting messages, the system will > prompt for root password. Chrome has never allowed content to trigger a system elevation prompt, even at install. Ignoring the severe security implications allowing that, it's also a UX mess. I'm sorry if that means this can't be supported on Mac 10.7+, but I don't see any other alternative. > > On 2012/09/18 15:36:28, viettrungluu wrote: > > On 2012/09/18 11:26:50, ygorshenin1 wrote: > > > Trung, friendly ping. > > > > > > On 2012/09/12 11:30:55, ygorshenin1 wrote: > > > > Broadcasting is supported from a NaCl plugin. It can be tested on OSX > 10.6, > > > but > > > > it requires admin privileges on OSX 10.7. As you can see from discussion, > it > > > was > > > > decided to simply disable corresponding net_unittests/browser tests for > OSX. > > > But > > > > this doesn't mean that we should switch off broadcasting feature, because > we > > > > need this one for several NaCl applications. > > > > LGTM, I guess, though I wonder how we're supposedly going to support UDP > > broadcast in NaCl if it won't work on Mac OS 10.7+.... > > > > > > > > > > On 2012/09/11 17:18:43, viettrungluu wrote: > > > > > On 2012/09/10 18:05:22, ygorshenin1 wrote: > > > > > > PTAL > > > > > > > > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > > > > File chrome/test/ppapi/ppapi_browsertest.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/10916052/diff/16003/chrome/test/ppapi/ppapi_br... > > > > > > chrome/test/ppapi/ppapi_browsertest.cc:291: // > > UDPSocketPrivate_Broadcast > > > is > > > > > > disabled for OSX because it requires > > > > > > From NaCl broadcasting is not a capability --- it's a special > "feature" > > > > which > > > > > is > > > > > > handled individually for Mac, Win and Linux. Take a look at > > > > > > > > > > > > > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_libeven... > > > > > > and > > > > > > > > > > > > > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/net/udp/udp_socket_win.cc&.... > > > > > > > > > > I'm not quite sure I follow. Is broadcasting supported from a NaCl > plugin > > or > > > > > not? (If it's not going to be supported on Mac, then should we support > it > > at > > > > > all? And if we don't support it, we probably shouldn't test it; probably > > we > > > > > should disable it for all platforms.) > > > > > > > > > > > > > > > > > On 2012/09/10 17:51:10, viettrungluu wrote: > > > > > > > Maybe it's better to just remove the broadcast test from > > > > > > > TestUDPSocketPrivate::RunTests() on Mac and NaCl? (This would of > > course > > > > mean > > > > > > > never testing broadcast in NaCl -- do we really need broadcast in > > NaCl? > > > > > > > Especially since it's apparently no longer a platform-independent > > > > > capability?) > > > > > > > > > > > > > > (What you've done here is not only rather messy, but there's also > high > > > > > > > probability that anyone adding new UDP tests will forget to add them > > > here; > > > > > > > keeping the list of tests in sync is a pain.)
On 2012/09/19 20:42:21, Justin Schuh wrote: > On 2012/09/18 17:00:04, ygorshenin1 wrote: > > Broadcasting *works* on Mac. But it requires admin privileges. So if mac user > > will run nacl plugin which sends some broadcasting messages, the system will > > prompt for root password. > > Chrome has never allowed content to trigger a system elevation prompt, even at > install. Ignoring the severe security implications allowing that, it's also a UX > mess. I'm sorry if that means this can't be supported on Mac 10.7+, but I don't > see any other alternative. I agree with this. Unless there's a strong reason to keep it on other platforms, it's probably best to disable NaCl UDP broadcast all together, though not my area of expertise. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
