Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8555002:
Added NaCl proxy for TCP/UDP (Closed)
|
Created:
9 years, 1 month ago by ygorshenin Modified:
9 years ago CC:
chromium-reviews, darin-cc_chromium.org, Dmitry Polukhin Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPPB_TCPSocket_Private/PPB_UDPSocket_Private are exposed to Renderer process.
BUG=105859
TEST=build, tests are in next CL with render implementation
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112426
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #
Total comments: 44
Patch Set 4 : Fixed issues. #
Total comments: 2
Patch Set 5 : Fixed problem in GetLocalAddress and GetRemoteAddress methods. #
Total comments: 4
Patch Set 6 : Added rpc_server suffix. #
Total comments: 2
Patch Set 7 : Added PPAPI NaCl tests. #
Total comments: 179
Patch Set 8 : Fixed issues. #
Total comments: 3
Patch Set 9 : Fixed TCP test. #Patch Set 10 : Deleted PPAPI NaCl tests. #Messages
Total messages: 18 (0 generated)
There are no tests in the CL. How do you know your code works? :) We are now trying to migrate to ppapi_tests (ppapi/tests/, ppapi/ppapi_tests.gyp:ppapi_nacl_tests) running under NaCl. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc:420: static const PPB_TCPSocket_Private *ppb = consistency: PPB_TCPSocket_Private* ppb http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc:427: static const PPB_UDPSocket_Private *ppb = ditto http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_globals.h (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.h:120: const PPB_TCPSocket_Private* PPBTCPSocketPrivateInterface(); Not quite related to this CL... Private historically meant - not use in untrusted plugins. Then we added PPB_PDF, which is a special purpose interface, but does not have "private" in it. Why did these get renamed to have "private"? http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:3: // found in the LICENSE file. quick header comment? http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:44: int32_t* is_tcp_socket_private) { s/private// http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:62: nacl_abi_size_t host_bytes, char* host, can't you just use "char*" (string in .srpc)? http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:100: CHECK(addr_bytes == sizeof(PP_NetAddress_Private)); we usually just return in such cases, I think http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:112: reinterpret_cast<const struct PP_NetAddress_Private*>(addr), "const" is not necessary here, is it? http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:143: static_cast<nacl_abi_size_t>(sizeof(PP_NetAddress_Private)); Shouldn't this be a check in the beginning of the function? http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:167: static_cast<nacl_abi_size_t>(sizeof(PP_NetAddress_Private)); same comment as above http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:176: nacl_abi_size_t server_name_bytes, char* server_name, same comment as above about string .srpc type http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:216: static_cast<nacl_abi_size_t>(std::numeric_limits<int32_t>::max())); chrome implementation should be validating the number of bytes, no? http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:3: // found in the LICENSE file. please apply the comments from the other file to this one - they have a lot in common http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc:45: int32_t is_tcp_socket_private; s/private// (match the function name) http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc:63: if (callback.func == 0) // Just like Chrome, for now disallow blocking calls. You should be checking for NULL or even do this check after AddCallback for the id, in case AddCallback returns 0 to signal other error cases. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc:92: if (callback.func == 0) // Just like Chrome, for now disallow blocking calls. same comment as above http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:47: int32_t is_udp_socket_private; s/private// http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:9: 'name': 'Ppb_TCPSocketPrivate', our convention is no _ after Ppb and RpcServer at the end. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/tests/pp... File ppapi/native_client/tests/ppapi_test_lib/get_browser_interface.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/tests/pp... ppapi/native_client/tests/ppapi_test_lib/get_browser_interface.cc:129: GetBrowserInterfaceSafe(PPB_TCPSOCKET_PRIVATE_INTERFACE)); Won't there be some kind of switch for these interfaces? I.e. we won't expect them to be always there?
To really test these code we need to implement renderer part - it is in progress right now. But this CL looks almost ready and we would like to start codereview as early as possible taking into account that we have just several days before M17 branch point. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_globals.h (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.h:37: #include "ppapi/c/private/ppb_tcp_socket_private.h" Please sort it. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.h:120: const PPB_TCPSocket_Private* PPBTCPSocketPrivateInterface(); On 2011/11/18 09:52:10, polina wrote: > Not quite related to this CL... Private historically meant - not use in > untrusted plugins. Then we added PPB_PDF, which is a special purpose interface, > but does not have "private" in it. Why did these get renamed to have "private"? I think because we would like to let people know that they shouldn't use these interface except for some special circumstances. Chrome does the same for special extension permissions. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/tests/pp... File ppapi/native_client/tests/ppapi_test_lib/get_browser_interface.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/tests/pp... ppapi/native_client/tests/ppapi_test_lib/get_browser_interface.cc:129: GetBrowserInterfaceSafe(PPB_TCPSOCKET_PRIVATE_INTERFACE)); On 2011/11/18 09:52:10, polina wrote: > Won't there be some kind of switch for these interfaces? I.e. we won't expect > them to be always there? I think interfaces will be always available. But CreateSocket will fail for non-approved apps. The check should happen in Browser process.
http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc:420: static const PPB_TCPSocket_Private *ppb = On 2011/11/18 09:52:10, polina wrote: > consistency: PPB_TCPSocket_Private* ppb Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc:427: static const PPB_UDPSocket_Private *ppb = On 2011/11/18 09:52:10, polina wrote: > ditto Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_globals.h (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_globals.h:37: #include "ppapi/c/private/ppb_tcp_socket_private.h" On 2011/11/18 10:20:43, Dmitry Polukhin wrote: > Please sort it. Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:3: // found in the LICENSE file. On 2011/11/18 09:52:10, polina wrote: > quick header comment? Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:44: int32_t* is_tcp_socket_private) { On 2011/11/18 09:52:10, polina wrote: > s/private// Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:62: nacl_abi_size_t host_bytes, char* host, On 2011/11/18 09:52:10, polina wrote: > can't you just use "char*" (string in .srpc)? Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:100: CHECK(addr_bytes == sizeof(PP_NetAddress_Private)); On 2011/11/18 09:52:10, polina wrote: > we usually just return in such cases, I think Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:112: reinterpret_cast<const struct PP_NetAddress_Private*>(addr), On 2011/11/18 09:52:10, polina wrote: > "const" is not necessary here, is it? Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:143: static_cast<nacl_abi_size_t>(sizeof(PP_NetAddress_Private)); On 2011/11/18 09:52:10, polina wrote: > Shouldn't this be a check in the beginning of the function? Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:167: static_cast<nacl_abi_size_t>(sizeof(PP_NetAddress_Private)); On 2011/11/18 09:52:10, polina wrote: > same comment as above Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:176: nacl_abi_size_t server_name_bytes, char* server_name, On 2011/11/18 09:52:10, polina wrote: > same comment as above about string .srpc type Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:216: static_cast<nacl_abi_size_t>(std::numeric_limits<int32_t>::max())); On 2011/11/18 09:52:10, polina wrote: > chrome implementation should be validating the number of bytes, no? Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:3: // found in the LICENSE file. On 2011/11/18 09:52:10, polina wrote: > please apply the comments from the other file to this one - they have a lot in > common Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc:45: int32_t is_tcp_socket_private; On 2011/11/18 09:52:10, polina wrote: > s/private// (match the function name) Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc:63: if (callback.func == 0) // Just like Chrome, for now disallow blocking calls. On 2011/11/18 09:52:10, polina wrote: > You should be checking for NULL or even do this check after AddCallback for the > id, in case AddCallback returns 0 to signal other error cases. Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_tcp_socket_private.cc:92: if (callback.func == 0) // Just like Chrome, for now disallow blocking calls. On 2011/11/18 09:52:10, polina wrote: > same comment as above Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:47: int32_t is_udp_socket_private; On 2011/11/18 09:52:10, polina wrote: > s/private// Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:9: 'name': 'Ppb_TCPSocketPrivate', On 2011/11/18 09:52:10, polina wrote: > our convention is no _ after Ppb and RpcServer at the end. Done.
PTAL
http://codereview.chromium.org/8555002/diff/8002/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/8002/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc:5: // SRPC-abstraction wrappers around PPB_TCPSocket_Private functions. The original file name with "rpc_server" at the end was correct. Please rename things back. http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc:5: // SRPC-abstraction wrappers around PPB_TCPSocket_Private functions. Previous file name browser_..._rpc_server.cc was correct and followed our convention. The new name does not. please rename back. This will also make it easier for me to view the diff of the changes to the file. http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:160: char * const raw_addr = char*
PTAL http://codereview.chromium.org/8555002/diff/8002/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/8002/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc:5: // SRPC-abstraction wrappers around PPB_TCPSocket_Private functions. On 2011/11/23 08:09:29, polina wrote: > The original file name with "rpc_server" at the end was correct. Please rename > things back. Done. http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private.cc:5: // SRPC-abstraction wrappers around PPB_TCPSocket_Private functions. On 2011/11/23 08:09:29, polina wrote: > Previous file name browser_..._rpc_server.cc was correct and followed our > convention. The new name does not. please rename back. This will also make it > easier for me to view the diff of the changes to the file. Done. http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc (right): http://codereview.chromium.org/8555002/diff/21001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_udp_socket_private.cc:160: char * const raw_addr = On 2011/11/23 08:09:29, polina wrote: > char* Done.
What's the status of the chrome CL that should provide a ppapi/test that we can quickly convert into a nexe for testing the proxy? http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:100: CHECK(addr_bytes == sizeof(PP_NetAddress_Private)); On 2011/11/21 14:13:33, ygorshenin wrote: > On 2011/11/18 09:52:10, polina wrote: > > we usually just return in such cases, I think > > Done. You missed several other such CHECKs in this file. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:3: // found in the LICENSE file. On 2011/11/21 14:13:33, ygorshenin wrote: > On 2011/11/18 09:52:10, polina wrote: > > please apply the comments from the other file to this one - they have a lot in > > common > > Done. This must be the wrong snapshot. I don't see any changes to CHECKs, remote callback validation, _private, etc. Don't see the changes for removing "_private", http://codereview.chromium.org/8555002/diff/26012/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/26012/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:104: rpc->result = NACL_SRPC_RESULT_APP_ERROR; This should be set before the return. (Actually this is the reason why we start all functions with these standard lines, even if the don't have intermediate returns. If those ever get added, you don't risk such omissions.)
PTAL http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:100: CHECK(addr_bytes == sizeof(PP_NetAddress_Private)); On 2011/11/28 07:55:45, polina wrote: > On 2011/11/21 14:13:33, ygorshenin wrote: > > On 2011/11/18 09:52:10, polina wrote: > > > we usually just return in such cases, I think > > > > Done. > > You missed several other such CHECKs in this file. Done. http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/4001/ppapi/native_client/src/shar... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:3: // found in the LICENSE file. On 2011/11/28 07:55:45, polina wrote: > On 2011/11/21 14:13:33, ygorshenin wrote: > > On 2011/11/18 09:52:10, polina wrote: > > > please apply the comments from the other file to this one - they have a lot > in > > > common > > > > Done. > > This must be the wrong snapshot. I don't see any changes to CHECKs, remote > callback validation, _private, etc. > Don't see the changes for removing "_private", Done. http://codereview.chromium.org/8555002/diff/26012/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/26012/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:104: rpc->result = NACL_SRPC_RESULT_APP_ERROR; On 2011/11/28 07:55:45, polina wrote: > This should be set before the return. > (Actually this is the reason why we start all functions with these standard > lines, even if the don't have intermediate returns. If those ever get added, you > don't risk such omissions.) Done.
I really hoped not to have to repeat duplicate comments over and over again by having you apply each unique comment to all occurrences of the issue in the CL, but it does not seem to be working :( Since you are on a tight timeline and I am going on vacation on Wed, I took the time to point out explicitly as many duplicates as possible. I might have missed a couple of spots, especially at the end when I was running out of steam, so please do take advantage of global search&replace. Also, please do take a look at other ppapi/tests. Yours looks like a direct port of a Nacl test, but the NaCl tests follow different conventions. You need a bit more work to make this a proper ppapi_test. They also often use the C++ interface instead of the C interface, so you might get that comment from other reviewers. I personally think there is value in testing the C interface to isolate the bugs in the C++ one from the bugs in other layers. http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest.cc File chrome/test/ui/ppapi_uitest.cc (right): http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... chrome/test/ui/ppapi_uitest.cc:202: launch_arguments_.AppendArg("--allow-nacl-socket-api=127.0.0.1"); A comment explaining what this flag is for would be helpful. http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... chrome/test/ui/ppapi_uitest.cc:202: launch_arguments_.AppendArg("--allow-nacl-socket-api=127.0.0.1"); Is there no switches file where this flag is defined? http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... chrome/test/ui/ppapi_uitest.cc:295: TEST_PPAPI_NACL_VIA_HTTP(TCPSocketPrivateShared) A ppapi test should occur as IN, OUT and NACL. This way you can separate trusted from unstrusted and isolate failure coming from Chrome implementation from the failures coming from the NaCl implemenataion. The the Chrome IPC proxy does not even overlap with NaCl test. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:24: void PpbTCPSocketPrivateServer::PPB_TCPSocket_Private_Create( As mentioned earlier this should be PpbTCPSocketPrivateRpcServer. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:139: rpc->result = NACL_SRPC_RESULT_OK; it's more consistent with the rest of our code and safer (in case more returns are added later) to move this to the end http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:165: rpc->result = NACL_SRPC_RESULT_OK; same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:278: static_cast<nacl_abi_size_t>(std::numeric_limits<int32_t>::max()))) same comment as for the unnecessary check in Read http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:24: void PpbUDPSocketPrivateServer::PPB_UDPSocket_Private_Create( As mentioned earlier, as per our convention, the name should be PpbFooRpcServer, not PpbFooServer. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:46: int32_t* is_udp_socket_private) { As mentioned earlier, "private" should be removed. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:81: reinterpret_cast<const struct PP_NetAddress_Private*>(addr), As mentioned earlier, you should not need const, should you? http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:101: char* buffer, move to the previous line? http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:107: static_cast<nacl_abi_size_t>(std::numeric_limits<int32_t>::max()))) As mentioned earlier, it should be ok to rely on the size checks done by the ppapi implementation. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:169: *addr_bytes = static_cast<nacl_abi_size_t>(sizeof(PP_NetAddress_Private)); As mentioned earlier, this should be an equality equality check in the beginning. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:184: no blank http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:188: UNREFERENCED_PARAMETER(addr_bytes); aren't you missing a sizeof check for this? http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:192: return; same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:206: (const struct PP_NetAddress_Private*) addr, same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:206: (const struct PP_NetAddress_Private*) addr, c-style casts are against the style guide http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:9: 'name': 'PpbTCPSocketPrivate', PpbTCPSocketPrivateRpc http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:20: 'outputs': [['is_tcp_socket_private', 'int32_t'], # PP_Bool s/private// http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:25: ['host', 'string'], # const char* http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:26: ['port', 'int32_t'], # uint16_t http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:27: ['callback_id', 'int32_t'], # PP_CompletionCallback http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:35: ['callback_id', 'int32_t'], same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:56: ['server_name', 'string'], same as above, here and below http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:66: ['callback_id', 'int32_t'], same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:68: 'outputs': [['buffer', 'char[]'], # char* http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:74: ['buffer', 'char[]'], # const char* http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:76: ['callback_id', 'int32_t'] same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:9: 'name': 'PpbUDPSocketPrivate', PpbUDPSocketPrivateRpc http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:26: ['callback_id', 'int32_t'], same as in the other file http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:34: ['callback_id', 'int32_t'] same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:36: 'outputs': [['buffer', 'char[]'], # char* http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:49: ['buffer', 'char[]'], # char* http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:52: ['callback_id', 'int32_t'] same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi File ppapi/ppapi_tests.gypi (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi#newc... ppapi/ppapi_tests.gypi:133: 'tests/test_url_loader.h', new tests should also show up here to run as trusted http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi#newc... ppapi/ppapi_tests.gypi:512: 'tests/test_tcp_socket_private_shared.cc', why do these have "shared" suffix? http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... File ppapi/tests/test_tcp_socket_private_shared.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:8: #include "ppapi/tests/test_tcp_socket_private_shared.h" this should be the first include (see style guide) http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:24: return tcp_socket_private_interface_ && InitTestingInterface(); It would be helpful to set the error string, at least for the missing interface case, so the person running the test has a better idea why it is failing. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:31: RUN_TEST(Reconnect, filter); For tests running callbacks, you might want to test with force_async behavior and without. See test_urlloader for example. RUN_TEST_FORCEASYNC_AND_NOT. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:41: return true; Why does this return a bool if it always succeeds? http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:47: ASSERT_TRUE(tcp_socket_private_interface_->IsTCPSocket(*socket)); For diagnostics purposes, it would be more helpful if you used ReportError() and PASS() (see other tests for examples). In addition to being run via ui_tests, these tests can also be run manually, in which case the string errors get reported on the web page. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:54: TestCompletionCallback callback(instance_->pp_instance(), true); or use force_async_setting instead of true so you can test both modes http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:55: pp::CompletionCallback cb = (pp::CompletionCallback) callback; must not use c-style casts (see style guide) and you can actually use callback directly, without casting (the class defines a special operator for this) http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:64: std::string TestTCPSocketPrivateShared::SyncConnectWithNetAddress( same comments for this function as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:75: std::string TestTCPSocketPrivateShared::SyncSSLHandshake(PP_Resource socket, same comments for this function as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:87: std::string TestTCPSocketPrivateShared::SyncRead(PP_Resource socket, same comments for this function as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:96: *bytes_read = callback.WaitForResult(); no bytes_read validation? http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:100: std::string TestTCPSocketPrivateShared::SyncWrite(PP_Resource socket, same comments for this function as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:109: *bytes_wrote = callback.WaitForResult(); no bytes_wrote validation? http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:113: std::string TestTCPSocketPrivateShared::CheckHTTPResponse( same comments for this function as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:119: if (error_message != kSuccess) error.empty() http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:125: char response_buffer[kResponseBufferSize]; use a small buffer, so there are multiple reads http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:128: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:132: ASSERT_EQ(0, strncmp(response, response_buffer, ppapi tests usually take advantage of std::string functionality and use == instead of strncmp, etc. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:139: PP_Resource socket; please combine lines 139 and 141 http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:145: if (error_message != kSuccess) just return CreateSocket(...) http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:155: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:159: if (error_message != kSuccess) ditto http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:167: socket, &remote_address)); same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:179: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:182: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:185: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:189: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:201: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:204: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:207: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:211: ASSERT_EQ(PP_TRUE, same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:216: error_message = CreateSocket(&socket); same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:220: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:223: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:227: if (error_message != kSuccess) same as above http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... File ppapi/tests/test_tcp_socket_private_shared.h (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.h:5: #ifndef PPAPI_TESTS_TEST_TCP_SOCKET_PRIVATE_SHARED_H_ I am pretty sure the reviewers of the other CL would want to see this test and the gyp changes to enabled it as trusted. You will also need their approval as I am not in the ppapi/OWNERS. I recommend moving it to the chrome CL. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.h:12: class TestTCPSocketPrivateShared : public TestCase { Why "Shared" at the end? http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.h:15: : TestCase(instance) { you should initialize member variables here (in .cc) http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... File ppapi/tests/test_udp_socket_private_shared.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:25: return tcp_socket_private_interface_ && udp_socket_private_interface_ && same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:31: RUN_TEST(Connect, filter); same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:40: static const char* const kServerHost = "www.google.com"; would make more sense to move these to the header this way you will also avoid hardcoding the same values in the comment there http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:44: ASSERT_NE(0, *socket); same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:47: pp::CompletionCallback cb = (pp::CompletionCallback) callback; same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:58: std::string TestUDPSocketPrivateShared::CreateAndBindUDPSocket( same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:74: std::string TestUDPSocketPrivateShared::TestCreate() { same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:93: if (error_message != kSuccess) same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:96: if (error_message != kSuccess) same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:101: if (error_message != kSuccess) same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:104: if (error_message != kSuccess) same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:120: ASSERT_EQ(PP_OK_COMPLETIONPENDING, write_rv); same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... File ppapi/tests/test_udp_socket_private_shared.h (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.h:12: class TestUDPSocketPrivateShared : public TestCase { same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.h:15: : TestCase(instance) { same as in other file
http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest.cc File chrome/test/ui/ppapi_uitest.cc (right): http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... chrome/test/ui/ppapi_uitest.cc:202: launch_arguments_.AppendArg("--allow-nacl-socket-api=127.0.0.1"); On 2011/11/29 05:20:47, polina wrote: > Is there no switches file where this flag is defined? Yes, this flag is defined in CL http://codereview.chromium.org/8688002/, so, this CL can't be commited and need to be merged with it. But in this case we'll lost comments and history. http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... chrome/test/ui/ppapi_uitest.cc:295: TEST_PPAPI_NACL_VIA_HTTP(TCPSocketPrivateShared) On 2011/11/29 05:20:47, polina wrote: > A ppapi test should occur as IN, OUT and NACL. This way you can separate trusted > from unstrusted and isolate failure coming from Chrome implementation from the > failures coming from the NaCl implemenataion. The the Chrome IPC proxy does not > even overlap with NaCl test. Added TEST_PPAPI_OUT_OF_PROCESS(...). TEST_PPAPI_IN_PROCESS doesn't work and in future will be investigated. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:24: void PpbTCPSocketPrivateServer::PPB_TCPSocket_Private_Create( On 2011/11/29 05:20:47, polina wrote: > As mentioned earlier this should be PpbTCPSocketPrivateRpcServer. Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:139: rpc->result = NACL_SRPC_RESULT_OK; On 2011/11/29 05:20:47, polina wrote: > it's more consistent with the rest of our code and safer (in case more returns > are added later) to move this to the end Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:165: rpc->result = NACL_SRPC_RESULT_OK; On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_tcp_socket_private_rpc_server.cc:278: static_cast<nacl_abi_size_t>(std::numeric_limits<int32_t>::max()))) On 2011/11/29 05:20:47, polina wrote: > same comment as for the unnecessary check in Read Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:24: void PpbUDPSocketPrivateServer::PPB_UDPSocket_Private_Create( On 2011/11/29 05:20:47, polina wrote: > As mentioned earlier, as per our convention, the name should be PpbFooRpcServer, > not PpbFooServer. Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:46: int32_t* is_udp_socket_private) { On 2011/11/29 05:20:47, polina wrote: > As mentioned earlier, "private" should be removed. Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:81: reinterpret_cast<const struct PP_NetAddress_Private*>(addr), On 2011/11/29 05:20:47, polina wrote: > As mentioned earlier, you should not need const, should you? Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:101: char* buffer, On 2011/11/29 05:20:47, polina wrote: > move to the previous line? Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:107: static_cast<nacl_abi_size_t>(std::numeric_limits<int32_t>::max()))) On 2011/11/29 05:20:47, polina wrote: > As mentioned earlier, it should be ok to rely on the size checks done by the > ppapi implementation. Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:169: *addr_bytes = static_cast<nacl_abi_size_t>(sizeof(PP_NetAddress_Private)); On 2011/11/29 05:20:47, polina wrote: > As mentioned earlier, this should be an equality equality check in the > beginning. Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:184: On 2011/11/29 05:20:47, polina wrote: > no blank Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:188: UNREFERENCED_PARAMETER(addr_bytes); On 2011/11/29 05:20:47, polina wrote: > aren't you missing a sizeof check for this? Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:192: return; On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_udp_socket_private_rpc_server.cc:206: (const struct PP_NetAddress_Private*) addr, On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:9: 'name': 'PpbTCPSocketPrivate', On 2011/11/29 05:20:47, polina wrote: > PpbTCPSocketPrivateRpc Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:20: 'outputs': [['is_tcp_socket_private', 'int32_t'], # PP_Bool On 2011/11/29 05:20:47, polina wrote: > s/private// Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:25: ['host', 'string'], On 2011/11/29 05:20:47, polina wrote: > # const char* Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:26: ['port', 'int32_t'], On 2011/11/29 05:20:47, polina wrote: > # uint16_t Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:27: ['callback_id', 'int32_t'], On 2011/11/29 05:20:47, polina wrote: > # PP_CompletionCallback Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:35: ['callback_id', 'int32_t'], On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:56: ['server_name', 'string'], On 2011/11/29 05:20:47, polina wrote: > same as above, here and below Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:66: ['callback_id', 'int32_t'], On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:68: 'outputs': [['buffer', 'char[]'], On 2011/11/29 05:20:47, polina wrote: > # char* Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:74: ['buffer', 'char[]'], On 2011/11/29 05:20:47, polina wrote: > # const char* Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_tcp_socket_private.srpc:76: ['callback_id', 'int32_t'] On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... File ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:9: 'name': 'PpbUDPSocketPrivate', On 2011/11/29 05:20:47, polina wrote: > PpbUDPSocketPrivateRpc Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:26: ['callback_id', 'int32_t'], On 2011/11/29 05:20:47, polina wrote: > same as in the other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:34: ['callback_id', 'int32_t'] On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:36: 'outputs': [['buffer', 'char[]'], On 2011/11/29 05:20:47, polina wrote: > # char* Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:49: ['buffer', 'char[]'], On 2011/11/29 05:20:47, polina wrote: > # char* Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/native_client/src/sha... ppapi/native_client/src/shared/ppapi_proxy/ppb_udp_socket_private.srpc:52: ['callback_id', 'int32_t'] On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi File ppapi/ppapi_tests.gypi (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi#newc... ppapi/ppapi_tests.gypi:133: 'tests/test_url_loader.h', On 2011/11/29 05:20:47, polina wrote: > new tests should also show up here to run as trusted Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi#newc... ppapi/ppapi_tests.gypi:512: 'tests/test_tcp_socket_private_shared.cc', On 2011/11/29 05:20:47, polina wrote: > why do these have "shared" suffix? Because of there already exists test_tcp_socket_private.cc file. This test doesn't work in NaCl and under our investigation. If you can suggest more appropriate suffix, you're welcome. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... File ppapi/tests/test_tcp_socket_private_shared.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:8: #include "ppapi/tests/test_tcp_socket_private_shared.h" On 2011/11/29 05:20:47, polina wrote: > this should be the first include (see style guide) Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:24: return tcp_socket_private_interface_ && InitTestingInterface(); On 2011/11/29 05:20:47, polina wrote: > It would be helpful to set the error string, at least for the missing interface > case, so the person running the test has a better idea why it is failing. Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:31: RUN_TEST(Reconnect, filter); On 2011/11/29 05:20:47, polina wrote: > For tests running callbacks, you might want to test with force_async behavior > and without. See test_urlloader for example. RUN_TEST_FORCEASYNC_AND_NOT. Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:41: return true; On 2011/11/29 05:20:47, polina wrote: > Why does this return a bool if it always succeeds? Fields initialization moved to constructor. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:47: ASSERT_TRUE(tcp_socket_private_interface_->IsTCPSocket(*socket)); On 2011/11/29 05:20:47, polina wrote: > For diagnostics purposes, it would be more helpful if you used ReportError() and > PASS() (see other tests for examples). In addition to being run via ui_tests, > these tests can also be run manually, in which case the string errors get > reported on the web page. Good idea. But in this particular method ReportError will be useless --- there are no error codes. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:54: TestCompletionCallback callback(instance_->pp_instance(), true); On 2011/11/29 05:20:47, polina wrote: > or use force_async_setting instead of true so you can test both modes Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:55: pp::CompletionCallback cb = (pp::CompletionCallback) callback; On 2011/11/29 05:20:47, polina wrote: > must not use c-style casts (see style guide) > and you can actually use callback directly, without casting (the class defines a > special operator for this) Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:64: std::string TestTCPSocketPrivateShared::SyncConnectWithNetAddress( On 2011/11/29 05:20:47, polina wrote: > same comments for this function as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:75: std::string TestTCPSocketPrivateShared::SyncSSLHandshake(PP_Resource socket, On 2011/11/29 05:20:47, polina wrote: > same comments for this function as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:87: std::string TestTCPSocketPrivateShared::SyncRead(PP_Resource socket, On 2011/11/29 05:20:47, polina wrote: > same comments for this function as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:96: *bytes_read = callback.WaitForResult(); On 2011/11/29 05:20:47, polina wrote: > no bytes_read validation? Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:100: std::string TestTCPSocketPrivateShared::SyncWrite(PP_Resource socket, On 2011/11/29 05:20:47, polina wrote: > same comments for this function as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:109: *bytes_wrote = callback.WaitForResult(); On 2011/11/29 05:20:47, polina wrote: > no bytes_wrote validation? Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:113: std::string TestTCPSocketPrivateShared::CheckHTTPResponse( On 2011/11/29 05:20:47, polina wrote: > same comments for this function as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:119: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > error.empty() Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:125: char response_buffer[kResponseBufferSize]; On 2011/11/29 05:20:47, polina wrote: > use a small buffer, so there are multiple reads Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:128: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:132: ASSERT_EQ(0, strncmp(response, response_buffer, On 2011/11/29 05:20:47, polina wrote: > ppapi tests usually take advantage of std::string functionality and use == > instead of strncmp, etc. > Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:139: PP_Resource socket; On 2011/11/29 05:20:47, polina wrote: > please combine lines 139 and 141 Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:145: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > just return CreateSocket(...) Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:155: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:159: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > ditto Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:167: socket, &remote_address)); On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:179: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:182: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:185: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:189: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:201: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:204: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:207: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:211: ASSERT_EQ(PP_TRUE, On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:216: error_message = CreateSocket(&socket); On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:220: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:223: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:227: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as above Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... File ppapi/tests/test_tcp_socket_private_shared.h (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.h:5: #ifndef PPAPI_TESTS_TEST_TCP_SOCKET_PRIVATE_SHARED_H_ On 2011/11/29 05:20:47, polina wrote: > I am pretty sure the reviewers of the other CL would want to see this test and > the gyp changes to enabled it as trusted. You will also need their approval as I > am not in the ppapi/OWNERS. I recommend moving it to the chrome CL. After moving it to the chrome CL we'll lost all history. I think, both CLs must be merged after your approval. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.h:12: class TestTCPSocketPrivateShared : public TestCase { On 2011/11/29 05:20:47, polina wrote: > Why "Shared" at the end? Because of there already exists class with name "TCPSocketPrivate". Suggest better suffix. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.h:15: : TestCase(instance) { On 2011/11/29 05:20:47, polina wrote: > you should initialize member variables here (in .cc) Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... File ppapi/tests/test_udp_socket_private_shared.cc (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:25: return tcp_socket_private_interface_ && udp_socket_private_interface_ && On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:31: RUN_TEST(Connect, filter); On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:40: static const char* const kServerHost = "www.google.com"; On 2011/11/29 05:20:47, polina wrote: > would make more sense to move these to the header > this way you will also avoid hardcoding the same values in the comment there Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:44: ASSERT_NE(0, *socket); On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:47: pp::CompletionCallback cb = (pp::CompletionCallback) callback; On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:58: std::string TestUDPSocketPrivateShared::CreateAndBindUDPSocket( On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:74: std::string TestUDPSocketPrivateShared::TestCreate() { On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:93: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:96: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:101: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:104: if (error_message != kSuccess) On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.cc:120: ASSERT_EQ(PP_OK_COMPLETIONPENDING, write_rv); On 2011/11/29 05:20:47, polina wrote: > same as in other file Done. http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... File ppapi/tests/test_udp_socket_private_shared.h (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.h:12: class TestUDPSocketPrivateShared : public TestCase { On 2011/11/29 05:20:47, polina wrote: > same as in other file same as in other file http://codereview.chromium.org/8555002/diff/37001/ppapi/tests/test_udp_socket... ppapi/tests/test_udp_socket_private_shared.h:15: : TestCase(instance) { On 2011/11/29 05:20:47, polina wrote: > same as in other file Done.
http://codereview.chromium.org/8555002/diff/43003/ppapi/tests/test_tcp_socket... File ppapi/tests/test_tcp_socket_private_shared.cc (right): http://codereview.chromium.org/8555002/diff/43003/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:18: const char* const TestTCPSocketPrivateShared::kHost = "www.google.com"; Is it possible to make the tests (both TCP and UDP tests) not rely on external website?
http://codereview.chromium.org/8555002/diff/43003/ppapi/tests/test_tcp_socket... File ppapi/tests/test_tcp_socket_private_shared.cc (right): http://codereview.chromium.org/8555002/diff/43003/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:18: const char* const TestTCPSocketPrivateShared::kHost = "www.google.com"; On 2011/11/29 18:26:33, yzshen1 wrote: > Is it possible to make the tests (both TCP and UDP tests) not rely on external > website? Yeah, unfortunately we didn't find how to make it work with internal test server for NaCl (it looks like testing HTTP server has problem with requests that comes during testing - need more investigations). In addition with local server we won't be able to test SSL handshake. It looks like over tests use www.google.com/robots.txt too, like GetURL. I propose to add TODO to replace it with internal server in next CL that should be much smaller.
http://codereview.chromium.org/8555002/diff/43003/ppapi/tests/test_tcp_socket... File ppapi/tests/test_tcp_socket_private_shared.cc (right): http://codereview.chromium.org/8555002/diff/43003/ppapi/tests/test_tcp_socket... ppapi/tests/test_tcp_socket_private_shared.cc:18: const char* const TestTCPSocketPrivateShared::kHost = "www.google.com"; On 2011/11/29 20:09:38, Dmitry Polukhin wrote: > On 2011/11/29 18:26:33, yzshen1 wrote: > > Is it possible to make the tests (both TCP and UDP tests) not rely on external > > website? > > Yeah, unfortunately we didn't find how to make it work with internal test server > for NaCl (it looks like testing HTTP server has problem with requests that comes > during testing - need more investigations). In addition with local server we > won't be able to test SSL handshake. It looks like over tests use > http://www.google.com/robots.txt too, like GetURL. I propose to add TODO to replace it > with internal server in next CL that should be much smaller. Okay. Thanks for explaining.
LGTM on the NaCl proxy stuff (after all dependencies are committed/merged and trybots are happy). You should get a separate LGTM on the ppapi test from the people reviewing the chrome side of things. And if you end up committing this separately, please update the Issue title and description to reflect that this CL adds the NaCl proxy. http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest.cc File chrome/test/ui/ppapi_uitest.cc (right): http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... chrome/test/ui/ppapi_uitest.cc:202: launch_arguments_.AppendArg("--allow-nacl-socket-api=127.0.0.1"); On 2011/11/29 13:25:18, ygorshenin wrote: > On 2011/11/29 05:20:47, polina wrote: > > Is there no switches file where this flag is defined? > > Yes, this flag is defined in CL http://codereview.chromium.org/8688002/, so, > this CL can't be commited and need to be merged with it. But in this case we'll > lost comments and history. Don't merge the CLs. Just commit them in the right order. This will make it easier to isolate failures, track commit history, etc. And if you do end up merging or moving things around, please don't forget to add references to CLs with original comments to the new CLs' descriptions. http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... chrome/test/ui/ppapi_uitest.cc:295: TEST_PPAPI_NACL_VIA_HTTP(TCPSocketPrivateShared) On 2011/11/29 13:25:18, ygorshenin wrote: > On 2011/11/29 05:20:47, polina wrote: > > A ppapi test should occur as IN, OUT and NACL. This way you can separate > trusted > > from unstrusted and isolate failure coming from Chrome implementation from the > > failures coming from the NaCl implemenataion. The the Chrome IPC proxy does > not > > even overlap with NaCl test. > > Added TEST_PPAPI_OUT_OF_PROCESS(...). TEST_PPAPI_IN_PROCESS doesn't work and in > future will be investigated. TEST_PPAPI_IN_PROCESS tests your chrome implementation (the other CL) with an in-process plugin. It is very alarming that the test does not pass. Please at least add a TODO here with a bug id for this. You might also let the reviewers of the other CL know about this. Technically this test change belongs in the other CL. http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi File ppapi/ppapi_tests.gypi (right): http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi#newc... ppapi/ppapi_tests.gypi:512: 'tests/test_tcp_socket_private_shared.cc', On 2011/11/29 13:25:18, ygorshenin wrote: > On 2011/11/29 05:20:47, polina wrote: > > why do these have "shared" suffix? > > Because of there already exists test_tcp_socket_private.cc file. This test > doesn't work in NaCl and under our investigation. If you can suggest more > appropriate suffix, you're welcome. I would name the udp one what it should be (no shared) and the other one something simply like tcp_socket_private_shared2. Then file a bug to combine the two tests. There should not be multiple tests for the same interface.
No LGTM from valid reviewers yet.
On 2011/11/30 01:26:21, polina wrote: > LGTM on the NaCl proxy stuff (after all dependencies are committed/merged and > trybots are happy). > > You should get a separate LGTM on the ppapi test from the people reviewing the > chrome side of things. > > And if you end up committing this separately, please update the Issue title and > description to reflect that this CL adds the NaCl proxy. Changed issue title and description. > > http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest.cc > File chrome/test/ui/ppapi_uitest.cc (right): > > http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... > chrome/test/ui/ppapi_uitest.cc:202: > launch_arguments_.AppendArg("--allow-nacl-socket-api=127.0.0.1"); > On 2011/11/29 13:25:18, ygorshenin wrote: > > On 2011/11/29 05:20:47, polina wrote: > > > Is there no switches file where this flag is defined? > > > > Yes, this flag is defined in CL http://codereview.chromium.org/8688002/, so, > > this CL can't be commited and need to be merged with it. But in this case > we'll > > lost comments and history. > > Don't merge the CLs. Just commit them in the right order. This will make it > easier to isolate failures, track commit history, etc. And if you do end up > merging or moving things around, please don't forget to add references to CLs > with original comments to the new CLs' descriptions. > > http://codereview.chromium.org/8555002/diff/37001/chrome/test/ui/ppapi_uitest... > chrome/test/ui/ppapi_uitest.cc:295: > TEST_PPAPI_NACL_VIA_HTTP(TCPSocketPrivateShared) > On 2011/11/29 13:25:18, ygorshenin wrote: > > On 2011/11/29 05:20:47, polina wrote: > > > A ppapi test should occur as IN, OUT and NACL. This way you can separate > > trusted > > > from unstrusted and isolate failure coming from Chrome implementation from > the > > > failures coming from the NaCl implemenataion. The the Chrome IPC proxy does > > not > > > even overlap with NaCl test. > > > > Added TEST_PPAPI_OUT_OF_PROCESS(...). TEST_PPAPI_IN_PROCESS doesn't work and > in > > future will be investigated. > > TEST_PPAPI_IN_PROCESS tests your chrome implementation (the other CL) with an > in-process plugin. It is very alarming that the test does not pass. > > Please at least add a TODO here with a bug id for this. You might also let the > reviewers of the other CL know about this. Technically this test change belongs > in the other CL. Added TODO comments about failed TEST_PPAPI_IN_PROCESS > > http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi > File ppapi/ppapi_tests.gypi (right): > > http://codereview.chromium.org/8555002/diff/37001/ppapi/ppapi_tests.gypi#newc... > ppapi/ppapi_tests.gypi:512: 'tests/test_tcp_socket_private_shared.cc', > On 2011/11/29 13:25:18, ygorshenin wrote: > > On 2011/11/29 05:20:47, polina wrote: > > > why do these have "shared" suffix? > > > > Because of there already exists test_tcp_socket_private.cc file. This test > > doesn't work in NaCl and under our investigation. If you can suggest more > > appropriate suffix, you're welcome. > > I would name the udp one what it should be (no shared) and the other one > something simply like tcp_socket_private_shared2. Then file a bug to combine the > two tests. There should not be multiple tests for the same interface. Tests moved to http://codereview.chromium.org/8688002/
No LGTM from valid reviewers yet. |
