Convert FileRefImpl and URLRequestInfo to shared_impl.

Convert FileRefImpl and URLRequestInfo to shared_impl.

One nice side effect of this change is that there are no longer any IPCs
associated with the request info. Instead, we just send the parameters to
the host when we open a url loader.

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

[brettw, 2011-08-22 22:44:28]

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

http://codereview.chromium.org/7706021/diff/1/ppapi/tests/test_url_loader.cc#...
ppapi/tests/test_url_loader.cc:323: int32_t rv = loader.Open(request, callback);
This test changed because we now report header errors when the request is issued
rather than ignoring them like before.

[bbudge, 2011-08-23 20:35:58]

Darin and I have discussed moving the request validation logic down into Webkit
and I'm working on the patches. I'm adding a 'untrustedHttp" field to
WebURLLoaderOptions. Setting this 'true' will cause AssociatedURLLoader to run
the validation checks using the same code as XMLHttpRequest (after I pull it out
into some static methods.) This will change the behavior from
URLRequestInfo::SetProperty returning 'false' to URLLoader::Open returning an
error asynchronously. We will probably want to remove all method/header
validation code from PPAPI in the near future.

I made some comments about things I saw. A lot of this shared_impl and thunk
code is new to me so I had some questions as well.

http://codereview.chromium.org/7706021/diff/2002/ppapi/proxy/ppapi_param_trai...
File ppapi/proxy/ppapi_param_traits.h (right):

http://codereview.chromium.org/7706021/diff/2002/ppapi/proxy/ppapi_param_trai...
ppapi/proxy/ppapi_param_traits.h:16: #include
"ppapi/shared_impl/file_ref_impl.h"
I don't understand why you need this include here.

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
File ppapi/shared_impl/url_request_info_impl.cc (right):

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.cc:20: 
Just a heads up that Darin and I think this logic should be moved down into the
AssociatedURLLoader, where we can reuse the XMLHttpRequest code. This will
change the behavior of 'SetProperty', which currently returns 'false' when the
value isn't allowed. Instead, the loader will return (asynchronously) an error.

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.cc:101: headers(),
Do you really have to have these default constructor calls in the initializer
list? There are 3 others below too.

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.cc:121: const PPB_URLRequestInfo_Data&
data)
Alignment with prev param.

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.cc:127: const PPB_URLRequestInfo_Data&
data)
Alignment.

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
File ppapi/shared_impl/url_request_info_impl.h (right):

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.h:35: // Is is_file is set, these
variables are set. Note that the resoruce
sp. resource

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.h:42: // wire.
I'm having a hard time understanding this comment.

http://codereview.chromium.org/7706021/diff/2002/ppapi/thunk/ppb_file_ref_api.h
File ppapi/thunk/ppb_file_ref_api.h (right):

http://codereview.chromium.org/7706021/diff/2002/ppapi/thunk/ppb_file_ref_api...
ppapi/thunk/ppb_file_ref_api.h:22: virtual PP_Var GetPath() = 0;
Couldn't we keep these 'const' by using the 'mutable' keyword on member fields
of the implementation class?

http://codereview.chromium.org/7706021/diff/2002/webkit/plugins/ppapi/ppb_fil...
File webkit/plugins/ppapi/ppb_file_ref_impl.h (right):

http://codereview.chromium.org/7706021/diff/2002/webkit/plugins/ppapi/ppb_fil...
webkit/plugins/ppapi/ppb_file_ref_impl.h:66: FilePath system_path_;
Perhaps this should be named 'external_file_system_path_'.

[brettw, 2011-08-24 23:41:09]

Thanks, new snap up.

http://codereview.chromium.org/7706021/diff/2002/ppapi/proxy/ppapi_param_trai...
File ppapi/proxy/ppapi_param_traits.h (right):

http://codereview.chromium.org/7706021/diff/2002/ppapi/proxy/ppapi_param_trai...
ppapi/proxy/ppapi_param_traits.h:16: #include
"ppapi/shared_impl/file_ref_impl.h"
It defines FileRef_CreateInfo for which there's a template below that actually
needs the definition. I was able to remove the forward-declare below, however.

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
File ppapi/shared_impl/url_request_info_impl.cc (right):

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.cc:101: headers(),
On 2011/08/23 20:35:59, bbudge1 wrote:
> Do you really have to have these default constructor calls in the initializer
> list? There are 3 others below too.

No, but there were so many initializers I found it hard to make sure I had them
all and that they were in the right order when I didn't have the empty ones.

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
File ppapi/shared_impl/url_request_info_impl.h (right):

http://codereview.chromium.org/7706021/diff/2002/ppapi/shared_impl/url_reques...
ppapi/shared_impl/url_request_info_impl.h:42: // wire.
On 2011/08/23 20:35:59, bbudge1 wrote:
> I'm having a hard time understanding this comment.

I simplified this a bit.

[brettw, 2011-08-25 16:18:29]

New new snap up. This makes the test_shell_tests compile, which I forgot to
update. This revealed I messed up the default value for follow_redirects, so
thanks for that test!

I changed the url_request_info test from ASSERT to EXPECT since this was easier
for me to fix the problem (they're the same except EXPECT allows the test to
continue, so you can see multiple errors at once).

[bbudge, 2011-08-25 19:50:37]

LGTM with nits. Thanks for fixing the test - ASSERT->EXPECT.

http://codereview.chromium.org/7706021/diff/14001/ppapi/shared_impl/url_reque...
File ppapi/shared_impl/url_request_info_impl.h (right):

http://codereview.chromium.org/7706021/diff/14001/ppapi/shared_impl/url_reque...
ppapi/shared_impl/url_request_info_impl.h:13: #include
"ppapi/shared_impl/resource.h"
#include ordering

http://codereview.chromium.org/7706021/diff/14001/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

http://codereview.chromium.org/7706021/diff/14001/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:116: file_system->pp_instance());
This fits on a single line.

http://codereview.chromium.org/7706021/diff/14001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/7706021/diff/14001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:133: data().url)));
Single line?

http://codereview.chromium.org/7706021/diff/14001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:141:
frame->document().firstPartyForCookies());
Single line?