GetDocumentURL is added to PPB_Testing_Dev.

GetDocumentURL is added to PPB_Testing_Dev.
Fixed Test{TCP|UDP}SocketPrivateShared.

BUG=105863
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114637

[ygorshenin, 2011-12-13 13:46:31]

bbudge: your trick to get local available url doesn't work in OutOfProcess mode
--- URLLoader doesn't open pages in this mode. Also, we need some way to parse
URL, but GURL doesn't compile under NaCl. So, we decided to expose
GetDocumentURL.

[yzshen1, 2011-12-13 18:49:53]

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc
(right):

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc:108:
instance, reinterpret_cast<struct PP_URLComponents_Dev*>(components));
Is there any potential alignment problem with reinterpret_cast<> here?

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc:112:
rpc->result = NACL_SRPC_RESULT_OK;
I would suggest to follow other methods and add DebugPrintf.

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_testing.cc (right):

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_testing.cc:113:
std::vector<char> url_bytes(url_size);
It seems better to use things like
nacl::scoped_array<char> url_bytes(new char[length]);

http://codereview.chromium.org/8840007/diff/1/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (right):

http://codereview.chromium.org/8840007/diff/1/ppapi/tests/test_utils.cc#newco...
ppapi/tests/test_utils.cc:62: if (components.port.len > 0) {
You could reverse the condition and return early on failure.

http://codereview.chromium.org/8840007/diff/1/ppapi/tests/test_utils.cc#newco...
ppapi/tests/test_utils.cc:63: int i = atoi(url.substr(components.port.begin,
Please add include for atoi.

http://codereview.chromium.org/8840007/diff/1/webkit/plugins/ppapi/plugin_mod...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8840007/diff/1/webkit/plugins/ppapi/plugin_mod...
webkit/plugins/ppapi/plugin_module.cc:349: strcmp(name,
PPB_TESTING_DEV_INTERFACE_0_8)) {
strcmp(...) == 0

[bbudge, 2011-12-13 18:55:29]

LGTM. My trick seemed to work for me out-of-process (it's in the URLLoader tests
right now) but I like this better.

http://codereview.chromium.org/8840007/diff/1/ppapi/api/dev/ppb_testing_dev.idl
File ppapi/api/dev/ppb_testing_dev.idl (right):

http://codereview.chromium.org/8840007/diff/1/ppapi/api/dev/ppb_testing_dev.i...
ppapi/api/dev/ppb_testing_dev.idl:112: * to specify that no component
information is necessary.
A little awkward. How about:
If the canonicalized URL is valid, the method will parse the
URL and fill in the components structure. This pointer may be NULL to specify
that no component information is necessary.

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_testing.cc (right):

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_testing.cc:141: NULL,
OK, but only because I will follow up immediately with another CL to add
ExecuteScript, and the proxy for SimulateInputEvent.

[ygorshenin, 2011-12-14 17:41:10]

Thanks, PTAL

http://codereview.chromium.org/8840007/diff/1/ppapi/api/dev/ppb_testing_dev.idl
File ppapi/api/dev/ppb_testing_dev.idl (right):

http://codereview.chromium.org/8840007/diff/1/ppapi/api/dev/ppb_testing_dev.i...
ppapi/api/dev/ppb_testing_dev.idl:112: * to specify that no component
information is necessary.
On 2011/12/13 18:55:29, bbudge1 wrote:
> A little awkward. How about:
> If the canonicalized URL is valid, the method will parse the
> URL and fill in the components structure. This pointer may be NULL to specify
> that no component information is necessary.

Done.

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc
(right):

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc:108:
instance, reinterpret_cast<struct PP_URLComponents_Dev*>(components));
On 2011/12/13 18:49:54, yzshen1 wrote:
> Is there any potential alignment problem with reinterpret_cast<> here?

Look at PpbTestingRpcServer::PPB_Testing_ReadImageData. I think, there is no
problem with alignment.

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc:112:
rpc->result = NACL_SRPC_RESULT_OK;
On 2011/12/13 18:49:54, yzshen1 wrote:
> I would suggest to follow other methods and add DebugPrintf.

Done.

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_testing.cc (right):

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_testing.cc:113:
std::vector<char> url_bytes(url_size);
On 2011/12/13 18:49:54, yzshen1 wrote:
> It seems better to use things like
> nacl::scoped_array<char> url_bytes(new char[length]);

Done.

http://codereview.chromium.org/8840007/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_testing.cc:141: NULL,
On 2011/12/13 18:55:29, bbudge1 wrote:
> OK, but only because I will follow up immediately with another CL to add
> ExecuteScript, and the proxy for SimulateInputEvent.

Done.

http://codereview.chromium.org/8840007/diff/1/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (right):

http://codereview.chromium.org/8840007/diff/1/ppapi/tests/test_utils.cc#newco...
ppapi/tests/test_utils.cc:62: if (components.port.len > 0) {
On 2011/12/13 18:49:54, yzshen1 wrote:
> You could reverse the condition and return early on failure.

Done.

http://codereview.chromium.org/8840007/diff/1/ppapi/tests/test_utils.cc#newco...
ppapi/tests/test_utils.cc:63: int i = atoi(url.substr(components.port.begin,
On 2011/12/13 18:49:54, yzshen1 wrote:
> Please add include for atoi.

Done.

http://codereview.chromium.org/8840007/diff/1/webkit/plugins/ppapi/plugin_mod...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8840007/diff/1/webkit/plugins/ppapi/plugin_mod...
webkit/plugins/ppapi/plugin_module.cc:349: strcmp(name,
PPB_TESTING_DEV_INTERFACE_0_8)) {
On 2011/12/13 18:49:54, yzshen1 wrote:
> strcmp(...) == 0

Done.

[yzshen1, 2011-12-14 19:16:53]

lgtm

http://codereview.chromium.org/8840007/diff/4004/ppapi/native_client/src/shar...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc
(right):

http://codereview.chromium.org/8840007/diff/4004/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc:113:
DebugPrintf("PPB_Testing_Dev::GetDocumentURL\n");
nit: I think it is better to also output relevant information, such as the
resulting url.

http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (right):

http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc#ne...
ppapi/tests/test_utils.cc:65: else {
nit: no need to use 'else'.

if (...)
  return false;

int i = atoi(...);
...

[bbudge, 2011-12-14 19:37:47]

On 2011/12/14 19:16:53, yzshen1 wrote:
> lgtm
> 
>
http://codereview.chromium.org/8840007/diff/4004/ppapi/native_client/src/shar...
> File
> ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc
> (right):
> 
>
http://codereview.chromium.org/8840007/diff/4004/ppapi/native_client/src/shar...
>
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc:113:
> DebugPrintf("PPB_Testing_Dev::GetDocumentURL\n");
> nit: I think it is better to also output relevant information, such as the
> resulting url.
> 
> http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc
> File ppapi/tests/test_utils.cc (right):
> 
>
http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc#ne...
> ppapi/tests/test_utils.cc:65: else {
> nit: no need to use 'else'.
> 
> if (...)
>   return false;
> 
> int i = atoi(...);
> ...

Still LGTM. You'll need one for webkit/plugins/ppapi too. Probably brettw.

brettw@chromium.org
piman@chromium.org
viettrungluu@chromium.org

[ygorshenin, 2011-12-14 19:46:23]

+brettw

[brettw, 2011-12-14 22:36:47]

LGTM with yzshen's comment fixed.

http://codereview.chromium.org/8840007/diff/4004/ppapi/api/dev/ppb_url_util_d...
File ppapi/api/dev/ppb_url_util_dev.idl (right):

http://codereview.chromium.org/8840007/diff/4004/ppapi/api/dev/ppb_url_util_d...
ppapi/api/dev/ppb_url_util_dev.idl:7: * This file defines the
<code>PPB_URLUtil_Dev</code> interface.
Thanks for writing this.

http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (right):

http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc#ne...
ppapi/tests/test_utils.cc:65: else {
Yeah, we generally frown on using else after return. Note that if you kept this,
you should use {} for the if as well (all arms should either use {} or not).

[ygorshenin, 2011-12-15 11:48:22]

Thanks, yzshen's comments are fixed.

http://codereview.chromium.org/8840007/diff/4004/ppapi/api/dev/ppb_url_util_d...
File ppapi/api/dev/ppb_url_util_dev.idl (right):

http://codereview.chromium.org/8840007/diff/4004/ppapi/api/dev/ppb_url_util_d...
ppapi/api/dev/ppb_url_util_dev.idl:7: * This file defines the
<code>PPB_URLUtil_Dev</code> interface.
On 2011/12/14 22:36:47, brettw wrote:
> Thanks for writing this.

Done.

http://codereview.chromium.org/8840007/diff/4004/ppapi/native_client/src/shar...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc
(right):

http://codereview.chromium.org/8840007/diff/4004/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_testing_rpc_server.cc:113:
DebugPrintf("PPB_Testing_Dev::GetDocumentURL\n");
On 2011/12/14 19:16:54, yzshen1 wrote:
> nit: I think it is better to also output relevant information, such as the
> resulting url.

Done.

http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (right):

http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc#ne...
ppapi/tests/test_utils.cc:65: else {
On 2011/12/14 19:16:54, yzshen1 wrote:
> nit: no need to use 'else'.
> 
> if (...)
>   return false;
> 
> int i = atoi(...);
> ...

Done.

http://codereview.chromium.org/8840007/diff/4004/ppapi/tests/test_utils.cc#ne...
ppapi/tests/test_utils.cc:65: else {
On 2011/12/14 22:36:47, brettw wrote:
> Yeah, we generally frown on using else after return. Note that if you kept
this,
> you should use {} for the if as well (all arms should either use {} or not).

Done.

[commit-bot: I haz the power, 2011-12-15 14:18:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@google.com/8840007/23001

[commit-bot: I haz the power, 2011-12-15 15:29:22]

Change committed as 114637