Pepper: Move FileRef to the "new" resource proxy.

Pepper: Move FileRef to the "new" resource proxy.

This change moves the FileRef implementation from the previous one in the "old"
resource model (ppb_file_ref_impl.cc) to the "new" resource model
(pepper_file_ref_host.cc), and from the renderer to the browser.

As many as possible of the supporting changes were split off to other changes
to minimize the size of this change. Unfortunately, a lot of changes for
URLLoader had to be rolled into this change.

The data structures for CreateInfo have changed, and all users of FileRef have
to be moved over, which is what causes this change to be so large.

TBR=dmichael@chromium.org, jschuh@chromium.org, yzshen@chromium.org
BUG=225441
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216744

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218305

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219911

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221284

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223963

[teravest, 2013-08-05 17:39:46]

+jschuh for ppapi/proxy/ppapi_messages.h

This change removes the old PPB_FileRef_CreateInfo structure and replaces it
with the new FileRef_CreateInfo structure in some new messages. It also removes
messages so support the "old" FileRef implementation.

The only real change between the structures is how the idea of a "host resource"
is represented.

[jschuh, 2013-08-06 23:00:55]

Looks like the surface is basically the same (or smaller). So ipc security lgtm
(but this is a big one, so please correct me if i'm mistaken).

[teravest, 2013-08-06 23:05:08]

That's correct, the surface is basically the same.
Thanks!

On Tue, Aug 6, 2013 at 5:00 PM,  <jschuh@chromium.org> wrote:
> Looks like the surface is basically the same (or smaller). So ipc security
> lgtm
> (but this is a big one, so please correct me if i'm mistaken).
>
> https://codereview.chromium.org/21966004/

[dmichael (off chromium), 2013-08-07 22:19:13]

https://chromiumcodereview.appspot.com/21966004/diff/79001/content/renderer/p...
File content/renderer/pepper/url_request_info_util.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/79001/content/renderer/p...
content/renderer/pepper/url_request_info_util.cc:49: PP_Resource resource,
s/resouce/file_ref_resource

https://chromiumcodereview.appspot.com/21966004/diff/89017/chrome/renderer/pe...
File chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/chrome/renderer/pe...
chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc:65:
base::Bind(&PepperFlashDRMRendererHost::DidCreateResourceHost,
DidCreateFileRefHost might be a clearer name, since this class is also a
resource host.

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:108:
PpapiHostMsg_FileRef_CreateExternal(file_path);
Is there any slicing problem with doing this? It seems like you could just
create it in the function parameter list below.

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:182:
info.pending_host_resource_id = id;
Suggestion: What do you think of adding a couple of constructors or static
"named" constructors, so for example you could do:
FileRef_CreateInfo::CreateExternal(display_name, id);

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host_unittest.cc:69: //
communication.
Does it still compile? If it still compiles, please prefix it with DISABLED_
instead. And either way, you should probably file a bug and reference it here.

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_io_host.cc:155: plugin_instance_ =
HostGlobals::Get()->GetInstance(instance);
nit: Why not do this in the initializer list?

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/url_request_info_util.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/url_request_info_util.cc:48: ppapi::FileRef_DetailedInfo
file_info,
nit: The underscore looks a little weird. I think straight camel-case is
probably fine. (Same for FileRefCreateInfo).

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/url_request_info_util.cc:126: if (!data->body.empty()) {
this if seems unnecessary...  the loop condition should suffice

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/url_response_info_util.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/url_response_info_util.cc:50:
DataFromWebURLResponseCallback callback,
teeny nit: const&, for consistency with below (and arguably teeny performance
benefit)

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource_unittest.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource_unittest.cc:102: create_info.file_system_type
= PP_FILESYSTEMTYPE_LOCALTEMPORARY;
I'm not sure I understand why you need to do this...  you're synthesizing the
message, so it seems like you're totally bypassing any checks about creating
external FileRefs from the plugin...

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_i...
File ppapi/proxy/file_io_resource.cc (left):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.cc:19: using ppapi::thunk::PPB_FileRef_API;
Why did you need to do this? Seems like it would be better to stay consistent
with the other API types used here.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_lo...
File ppapi/proxy/url_loader_resource.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_lo...
ppapi/proxy/url_loader_resource.cc:371: if
(data.body_as_file_ref.file_system_type != PP_FILESYSTEMTYPE_INVALID) {
you check this a lot. How about a convenience function on FileRefCreateInfo... 
IsValid or something.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_re...
File ppapi/proxy/url_response_info_resource.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_re...
ppapi/proxy/url_response_info_resource.cc:70: body_as_file_ref_ =
ScopedPPResource(r);
(you could assign directly, though I'm okay with making it clear you're
converting to ScopedPPResource)

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
File ppapi/shared_impl/url_request_info_data.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
ppapi/shared_impl/url_request_info_data.cc:52: file_ref_pp_resource);
nit/suggestion: can you use ScopedPPResource instead?

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
File ppapi/shared_impl/url_response_info_data.h (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
ppapi/shared_impl/url_response_info_data.h:26: // Nonzero when streaming to a
file.
nit: comment could be clearer...  "Valid when streaming to a file" or something

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/thunk/ppb_fi...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/thunk/ppb_fi...
ppapi/thunk/ppb_file_ref_thunk.cc:40: info.file_system_plugin_resource =
file_system;
seems like a FileRefCreateInfo constructor would be nice?

[teravest, 2013-08-08 00:50:06]

https://chromiumcodereview.appspot.com/21966004/diff/89017/chrome/renderer/pe...
File chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/chrome/renderer/pe...
chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc:65:
base::Bind(&PepperFlashDRMRendererHost::DidCreateResourceHost,
On 2013/08/07 22:19:13, dmichael wrote:
> DidCreateFileRefHost might be a clearer name, since this class is also a
> resource host.

Done.

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:108:
PpapiHostMsg_FileRef_CreateExternal(file_path);
On 2013/08/07 22:19:13, dmichael wrote:
> Is there any slicing problem with doing this? It seems like you could just
> create it in the function parameter list below.

What do you mean? I don't understand.

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:182:
info.pending_host_resource_id = id;
On 2013/08/07 22:19:13, dmichael wrote:
> Suggestion: What do you think of adding a couple of constructors or static
> "named" constructors, so for example you could do:
> FileRef_CreateInfo::CreateExternal(display_name, id);

Done. (I had a function that was pretty close already. I just modified it to
take a display_name parameter also)

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host_unittest.cc:69: //
communication.
On 2013/08/07 22:19:13, dmichael wrote:
> Does it still compile? If it still compiles, please prefix it with DISABLED_
> instead. And either way, you should probably file a bug and reference it here.

No, since I changed the CreateInfo struct. I could make it compile and let it
limp along if you'd like, but it won't work...

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_io_host.cc:155: plugin_instance_ =
HostGlobals::Get()->GetInstance(instance);
On 2013/08/07 22:19:13, dmichael wrote:
> nit: Why not do this in the initializer list?

Done.

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/url_request_info_util.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/url_request_info_util.cc:126: if (!data->body.empty()) {
On 2013/08/07 22:19:13, dmichael wrote:
> this if seems unnecessary...  the loop condition should suffice

Done.

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/url_response_info_util.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/url_response_info_util.cc:50:
DataFromWebURLResponseCallback callback,
On 2013/08/07 22:19:13, dmichael wrote:
> teeny nit: const&, for consistency with below (and arguably teeny performance
> benefit)

Done.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource_unittest.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource_unittest.cc:102: create_info.file_system_type
= PP_FILESYSTEMTYPE_LOCALTEMPORARY;
On 2013/08/07 22:19:13, dmichael wrote:
> I'm not sure I understand why you need to do this...  you're synthesizing the
> message, so it seems like you're totally bypassing any checks about creating
> external FileRefs from the plugin...

The problem is that an external FileRef always needs a pending_resource_id to
attach to, which we don't have in this case. We could try to make that work, but
I figured it would be more complicated than using a LOCALTEMPORARY file in the
test instead.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_i...
File ppapi/proxy/file_io_resource.cc (left):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.cc:19: using ppapi::thunk::PPB_FileRef_API;
On 2013/08/07 22:19:13, dmichael wrote:
> Why did you need to do this? Seems like it would be better to stay consistent
> with the other API types used here.

Done.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_lo...
File ppapi/proxy/url_loader_resource.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_lo...
ppapi/proxy/url_loader_resource.cc:371: if
(data.body_as_file_ref.file_system_type != PP_FILESYSTEMTYPE_INVALID) {
On 2013/08/07 22:19:13, dmichael wrote:
> you check this a lot. How about a convenience function on FileRefCreateInfo...

> IsValid or something.

Done.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_re...
File ppapi/proxy/url_response_info_resource.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/url_re...
ppapi/proxy/url_response_info_resource.cc:70: body_as_file_ref_ =
ScopedPPResource(r);
On 2013/08/07 22:19:13, dmichael wrote:
> (you could assign directly, though I'm okay with making it clear you're
> converting to ScopedPPResource)

Changed to assign directly instead.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
File ppapi/shared_impl/url_request_info_data.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
ppapi/shared_impl/url_request_info_data.cc:52: file_ref_pp_resource);
On 2013/08/07 22:19:13, dmichael wrote:
> nit/suggestion: can you use ScopedPPResource instead?

It makes serializing the object a lot harder/more annoying. If you feel
strongly, I can look into it harder.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
File ppapi/shared_impl/url_response_info_data.h (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/shared_impl/...
ppapi/shared_impl/url_response_info_data.h:26: // Nonzero when streaming to a
file.
On 2013/08/07 22:19:13, dmichael wrote:
> nit: comment could be clearer...  "Valid when streaming to a file" or
something

Done.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/thunk/ppb_fi...
File ppapi/thunk/ppb_file_ref_thunk.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/thunk/ppb_fi...
ppapi/thunk/ppb_file_ref_thunk.cc:40: info.file_system_plugin_resource =
file_system;
On 2013/08/07 22:19:13, dmichael wrote:
> seems like a FileRefCreateInfo constructor would be nice?

This is the only place where we have to mess with it much. If you want, I can
add one, though.

[dmichael (off chromium), 2013-08-08 01:58:41]

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:108:
PpapiHostMsg_FileRef_CreateExternal(file_path);
On 2013/08/08 00:50:06, teravest wrote:
> On 2013/08/07 22:19:13, dmichael wrote:
> > Is there any slicing problem with doing this? It seems like you could just
> > create it in the function parameter list below.
> 
> What do you mean? I don't understand.
"Slicing" is when you assign an object to a type that's its base class. It's
possible in those cases to lose data, and you certainly lose the dynamic type.
I.e., create_msg really *should* be a PpapiHostMsg_FileRef_CreateExternal, not
its base, IPC::Message. I think it probably works okay in practice here, since
all the data is really in the Pickle base class. But it still looks funny to me,
and *may* have some subtle problems.

I was suggesting just putting:
 PpapiHostMsg_FileRef_CreateExternal(file_path),
right in the parameter list on line 112 instead of using create_msg.

[teravest, 2013-08-08 15:53:02]

On 2013/08/08 01:58:41, dmichael wrote:
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
> File content/renderer/pepper/pepper_file_chooser_host.cc (right):
> 
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
> content/renderer/pepper/pepper_file_chooser_host.cc:108:
> PpapiHostMsg_FileRef_CreateExternal(file_path);
> On 2013/08/08 00:50:06, teravest wrote:
> > On 2013/08/07 22:19:13, dmichael wrote:
> > > Is there any slicing problem with doing this? It seems like you could just
> > > create it in the function parameter list below.
> > 
> > What do you mean? I don't understand.
> "Slicing" is when you assign an object to a type that's its base class. It's
> possible in those cases to lose data, and you certainly lose the dynamic type.
> I.e., create_msg really *should* be a PpapiHostMsg_FileRef_CreateExternal, not
> its base, IPC::Message. I think it probably works okay in practice here, since
> all the data is really in the Pickle base class. But it still looks funny to
me,
> and *may* have some subtle problems.
> 
> I was suggesting just putting:
>  PpapiHostMsg_FileRef_CreateExternal(file_path),
> right in the parameter list on line 112 instead of using create_msg.

Sounds good. The code is shorter this way, too.

[teravest, 2013-08-08 18:05:20]

Please take another look.

[teravest, 2013-08-08 20:42:19]

+jam for
content/content_renderer.gypi
content/public/renderer/pepper_plugin_instance.h
content/public/renderer/renderer_ppapi_host.h

[dmichael (off chromium), 2013-08-08 21:24:03]

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host.cc:182:
info.pending_host_resource_id = id;
On 2013/08/08 00:50:06, teravest wrote:
> On 2013/08/07 22:19:13, dmichael wrote:
> > Suggestion: What do you think of adding a couple of constructors or static
> > "named" constructors, so for example you could do:
> > FileRef_CreateInfo::CreateExternal(display_name, id);
> 
> Done. (I had a function that was pretty close already. I just modified it to
> take a display_name parameter also)
Any reason you aren't using it here?

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
File content/renderer/pepper/pepper_file_chooser_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
content/renderer/pepper/pepper_file_chooser_host_unittest.cc:69: //
communication.
On 2013/08/08 00:50:06, teravest wrote:
> On 2013/08/07 22:19:13, dmichael wrote:
> > Does it still compile? If it still compiles, please prefix it with DISABLED_
> > instead. And either way, you should probably file a bug and reference it
here.
> 
> No, since I changed the CreateInfo struct. I could make it compile and let it
> limp along if you'd like, but it won't work...
I might slightly prefer that you make it compile, so we at least will maintain
it to that extent. Failing that, we might as well delete it. If we do that,
please verify this is covered by a browser test or file a bug.

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource_unittest.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource_unittest.cc:102: create_info.file_system_type
= PP_FILESYSTEMTYPE_LOCALTEMPORARY;
On 2013/08/08 00:50:06, teravest wrote:
> On 2013/08/07 22:19:13, dmichael wrote:
> > I'm not sure I understand why you need to do this...  you're synthesizing
the
> > message, so it seems like you're totally bypassing any checks about creating
> > external FileRefs from the plugin...
> 
> The problem is that an external FileRef always needs a pending_resource_id to
> attach to, which we don't have in this case. We could try to make that work,
but
> I figured it would be more complicated than using a LOCALTEMPORARY file in the
> test instead.
Hmm. It feels quite weird to unit test a file type that FileChooser will never
actually get. Like...  I could imagine us having a DCHECK somewhere in
FileChooserResource that the file system is external.

I don't know that I would want to hold your CL up on this detail. But maybe you
can stick in a TODO or file a bug to do it with the right kind of file. Maybe
even just leave the test mostly alone (aside from changing CreateInfo types) and
disable it with a filed bug.

It seems this is likely to come up elsewhere, and we might want to figure out
how to test it. Maybe just stick a fake pending resource id in, and perhaps look
to see if we send an appropriate AttachToPendingResource message (assuming the
Send goes in to a TestSink).

https://chromiumcodereview.appspot.com/21966004/diff/142001/chrome/renderer/p...
File chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/142001/chrome/renderer/p...
chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc:75: "",
std::string()?

https://chromiumcodereview.appspot.com/21966004/diff/142001/content/public/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/21966004/diff/142001/content/public/re...
content/public/renderer/renderer_ppapi_host.h:54: // will be returned.
Is that true always, or only after we switch the proxy on?
(I.e., I think maybe we switch from in-process to the "untrusted plugin"
somewhere in all this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
)

https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
content/renderer/pepper/pepper_file_io_host.cc:250: // necessary in the case
where we're a host for a resource in a trusted
s/"a trusted plugin"/"the trusted NaCl plugin"
i.e., this doesn't apply to just any trusted plugin, right?

https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
content/renderer/pepper/pepper_file_io_host.cc:254: child_process_id = 0;
optional suggestion: If you have to do this in very many places, you could make
a convenience function somewhere (the HostImpl?) that does this:
void RendererPpapiHostImpl::AdjustChildProcessId(&child_process_id) {
  if (IsRunningInProcess())
    *child_process_id = 0;
}
...the main purpose of which would be to have 1 place to stick the big fat
comment.

https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
File content/renderer/pepper/url_response_info_util.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
content/renderer/pepper/url_response_info_util.cc:54: external_path, "",
pending_resource_id);
nit: s/""/std::string()

https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/shared_impl...
File ppapi/shared_impl/file_ref_create_info.cc (right):

https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/shared_impl...
ppapi/shared_impl/file_ref_create_info.cc:41: if (display_name != "")
!display_name.empty()?

https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/thunk/ppb_f...
File ppapi/thunk/ppb_file_ref_api.h (right):

https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/thunk/ppb_f...
ppapi/thunk/ppb_file_ref_api.h:18: struct PPB_FileRefCreateInfo;
I think this needs to be FileRefCreateInfo (you probably got it for free from
one of your includes)

[teravest, 2013-08-08 21:53:35]

On Thu, Aug 8, 2013 at 3:24 PM,  <dmichael@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
> File content/renderer/pepper/pepper_file_chooser_host.cc (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
> content/renderer/pepper/pepper_file_chooser_host.cc:182:
> info.pending_host_resource_id = id;
> On 2013/08/08 00:50:06, teravest wrote:
>>
>> On 2013/08/07 22:19:13, dmichael wrote:
>> > Suggestion: What do you think of adding a couple of constructors or
>
> static
>>
>> > "named" constructors, so for example you could do:
>> > FileRef_CreateInfo::CreateExternal(display_name, id);
>
>
>> Done. (I had a function that was pretty close already. I just modified
>
> it to
>>
>> take a display_name parameter also)
>
> Any reason you aren't using it here?

Done.

>
>
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
> File content/renderer/pepper/pepper_file_chooser_host_unittest.cc
> (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/content/renderer/p...
> content/renderer/pepper/pepper_file_chooser_host_unittest.cc:69: //
> communication.
> On 2013/08/08 00:50:06, teravest wrote:
>>
>> On 2013/08/07 22:19:13, dmichael wrote:
>> > Does it still compile? If it still compiles, please prefix it with
>
> DISABLED_
>>
>> > instead. And either way, you should probably file a bug and
>
> reference it here.
>
>> No, since I changed the CreateInfo struct. I could make it compile and
>
> let it
>>
>> limp along if you'd like, but it won't work...
>
> I might slightly prefer that you make it compile, so we at least will
> maintain it to that extent. Failing that, we might as well delete it. If
> we do that, please verify this is covered by a browser test or file a
> bug.

I don't think we'll be able to maintain this much as-is to make it work.
I'd started down the path of making this test work, but it required
setting up a lot of browser-renderer communication that wasn't
working. We'll probably have to rethink how this whole integration
test works.
Sadly, we don't have much coverage for FileChooser.


>
>
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
> File ppapi/proxy/file_chooser_resource_unittest.cc (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/89017/ppapi/proxy/file_c...
> ppapi/proxy/file_chooser_resource_unittest.cc:102:
> create_info.file_system_type = PP_FILESYSTEMTYPE_LOCALTEMPORARY;
> On 2013/08/08 00:50:06, teravest wrote:
>>
>> On 2013/08/07 22:19:13, dmichael wrote:
>> > I'm not sure I understand why you need to do this...  you're
>
> synthesizing the
>>
>> > message, so it seems like you're totally bypassing any checks about
>
> creating
>>
>> > external FileRefs from the plugin...
>
>
>> The problem is that an external FileRef always needs a
>
> pending_resource_id to
>>
>> attach to, which we don't have in this case. We could try to make that
>
> work, but
>>
>> I figured it would be more complicated than using a LOCALTEMPORARY
>
> file in the
>>
>> test instead.
>
> Hmm. It feels quite weird to unit test a file type that FileChooser will
> never actually get. Like...  I could imagine us having a DCHECK
> somewhere in FileChooserResource that the file system is external.
>
> I don't know that I would want to hold your CL up on this detail. But
> maybe you can stick in a TODO or file a bug to do it with the right kind
> of file. Maybe even just leave the test mostly alone (aside from
> changing CreateInfo types) and disable it with a filed bug.
>
> It seems this is likely to come up elsewhere, and we might want to
> figure out how to test it. Maybe just stick a fake pending resource id
> in, and perhaps look to see if we send an appropriate
> AttachToPendingResource message (assuming the Send goes in to a
> TestSink).

Changed back to an external file.

>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/chrome/renderer/p...
> File chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/chrome/renderer/p...
> chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc:75: "",
> std::string()?

Done.

>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/content/public/re...
> File content/public/renderer/renderer_ppapi_host.h (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/content/public/re...
> content/public/renderer/renderer_ppapi_host.h:54: // will be returned.
> Is that true always, or only after we switch the proxy on?
> (I.e., I think maybe we switch from in-process to the "untrusted plugin"
> somewhere in all this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

Changed to be more explicit.

> )
>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
> File content/renderer/pepper/pepper_file_io_host.cc (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
> content/renderer/pepper/pepper_file_io_host.cc:250: // necessary in the
> case where we're a host for a resource in a trusted
> s/"a trusted plugin"/"the trusted NaCl plugin"
> i.e., this doesn't apply to just any trusted plugin, right?

Yep. Done.

>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
> content/renderer/pepper/pepper_file_io_host.cc:254: child_process_id =
> 0;
> optional suggestion: If you have to do this in very many places, you
> could make a convenience function somewhere (the HostImpl?) that does
> this:
> void RendererPpapiHostImpl::AdjustChildProcessId(&child_process_id) {
>   if (IsRunningInProcess())
>     *child_process_id = 0;
> }
> ...the main purpose of which would be to have 1 place to stick the big
> fat comment.

It doesn't show up in too many places. I'd like to find a better way
to make sure this is always handled properly, but I haven't thought of
anything yet.

>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
> File content/renderer/pepper/url_response_info_util.cc (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/content/renderer/...
> content/renderer/pepper/url_response_info_util.cc:54: external_path, "",
> pending_resource_id);
> nit: s/""/std::string()

Done.

>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/shared_impl...
> File ppapi/shared_impl/file_ref_create_info.cc (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/shared_impl...
> ppapi/shared_impl/file_ref_create_info.cc:41: if (display_name != "")
> !display_name.empty()?

Done.

>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/thunk/ppb_f...
> File ppapi/thunk/ppb_file_ref_api.h (right):
>
>
https://chromiumcodereview.appspot.com/21966004/diff/142001/ppapi/thunk/ppb_f...
> ppapi/thunk/ppb_file_ref_api.h:18: struct PPB_FileRefCreateInfo;
> I think this needs to be FileRefCreateInfo (you probably got it for free
> from one of your includes)

Thanks. Done.

>
> https://chromiumcodereview.appspot.com/21966004/

[dmichael (off chromium), 2013-08-08 23:02:53]

lgtm with deleting the broken unit test and solemnly swearing to manually test
it for now and write an integration test in a follow-up.

Thanks for slogging through this task.

https://codereview.chromium.org/21966004/diff/172002/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_chooser_host_unittest.cc (right):

https://codereview.chromium.org/21966004/diff/172002/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host_unittest.cc:126: const
std::vector<ppapi::PPB_FileRefCreateInfo>& chooser_results =
s/PPB_/

Have you tried uncommenting this? If we don't actually think it's fixable,
though, we should delete it. And please follow up with an integration test
(ideally file a bug so it doesn't get lost)

[yzshen1, 2013-08-08 23:16:21]

Wow, this is a very big CL. :)

https://codereview.chromium.org/21966004/diff/142001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc (right):

https://codereview.chromium.org/21966004/diff/142001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc:70:
ppapi::host::ReplyMessageContext reply_context,
const &, please.

https://codereview.chromium.org/21966004/diff/142001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_renderer_connection.cc:7: #include
"base/debug/stack_trace.h"
Please remove it.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host.cc:83: weak_factory_(this) {
please init num_pending_file_resources_.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host.cc:116: if (files.size() == 0)
{
- Please use files.empty().
- Does it make sense to share this code with DidCreateResourceHost?

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_io_host.cc:280: PP_FileSystemType type =
infos[0].file_system_type;
Maybe directly assign to file_system_type_?

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_io_host.cc:283: const std::string&
file_system_url_spec = infos[0].file_system_url_spec;
nit: Maybe merge line 283 and 284?

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_in_process_resource_creation.cc:90: PP_Instance
instance,
wrong indent.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:48: ppapi::FileRefDetailedInfo
file_info,
const &?

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:138: 0, child_process_id,
resources, &infos));
What does rounting ID 0 mean here?

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:151: return false;
- What if line 136 failed and |infos| is an empty vector?
- (I haven't checked, just to make sure you have considered that.) Is it okay
that we return false but change the contents of |dest|?

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.h (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.h:28: ::ppapi::URLRequestInfoData*
data,
nit: you don't need '::' at the beginning.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/url_response_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_response_info_util.cc:49: void
DidCreateResourceHost(ppapi::URLResponseInfoData data,
const &, please.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_response_info_util.cc:55:
base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback, data));
Why we have to post again? Couldn't we run the callback directly?

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/file_chooser...
File ppapi/proxy/file_chooser_resource_unittest.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/file_chooser...
ppapi/proxy/file_chooser_resource_unittest.cc:95: // host resource value set or
deserialization will fail. Since there isn't
Please update the comment accordingly.

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_loader_r...
File ppapi/proxy/url_loader_resource.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_loader_r...
ppapi/proxy/url_loader_resource.cc:7: #include "base/debug/stack_trace.h"
It seems we don't need it?

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_response...
File ppapi/proxy/url_response_info_resource.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_response...
ppapi/proxy/url_response_info_resource.cc:66: body_as_file_ref_ =
FileRefResource::CreateFileRef(
What if the response is not streamed to a file?

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_create_info.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.cc:7: #include "base/files/file_path.h"
It is already included in .h.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.cc:41: if (display_name != "")
please use empty().

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_create_info.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:21:
file_system_type(PP_FILESYSTEMTYPE_INVALID),
wrong indent.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:25: PPAPI_SHARED_EXPORT bool IsValid()
const;
Out of curiosity: why putting PPAPI_SHARED_EXPORT here instead of on the struct
itself?

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:34: // Since FileRef needs to hold a
FileSystem reference, we need to pass the
Please comment on whether this struct holds 1 ref to the resource.
(I guess it doesn't, right?)

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:42: MakeExternalFileRefCreateInfo(const
base::FilePath& external_path,
Maybe it is better to make it a static function of FileRefCreateInfo?

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_detailed_info.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_detailed_info.h:17: // the Browser to the renderer
'b'rowser and trailing '.', please.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_detailed_info.h:19: PP_Resource resource;
Just to be sure: I think this doesn't carry 1 ref, right?

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
File ppapi/shared_impl/url_request_info_data.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
ppapi/shared_impl/url_request_info_data.cc:7: #include <stdio.h>
nit: an empty line below it, please.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
File ppapi/shared_impl/url_request_info_data.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
ppapi/shared_impl/url_request_info_data.h:42: // ref is releated when BodyItem
is destroyed.
- releated -> released
- it is more clear to use ScopedPPResource to indicate that a ref is held by
this object.

https://codereview.chromium.org/21966004/diff/142001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_api.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_api.h:18: struct PPB_FileRefCreateInfo;
Is it FileRefCreateInfo (no PPB_)?

[teravest, 2013-08-09 02:00:07]

https://codereview.chromium.org/21966004/diff/142001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc (right):

https://codereview.chromium.org/21966004/diff/142001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc:70:
ppapi::host::ReplyMessageContext reply_context,
On 2013/08/08 23:16:21, yzshen1 wrote:
> const &, please.

Done.

https://codereview.chromium.org/21966004/diff/142001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_flash_drm_renderer_host.cc:75: "",
On 2013/08/08 21:24:03, dmichael wrote:
> std::string()?

Done.

https://codereview.chromium.org/21966004/diff/142001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_renderer_connection.cc:7: #include
"base/debug/stack_trace.h"
On 2013/08/08 23:16:21, yzshen1 wrote:
> Please remove it.

Done.

https://codereview.chromium.org/21966004/diff/142001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/21966004/diff/142001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:54: // will be returned.
On 2013/08/08 21:24:03, dmichael wrote:
> Is that true always, or only after we switch the proxy on?
> (I.e., I think maybe we switch from in-process to the "untrusted plugin"
> somewhere in all this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> )

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host.cc:83: weak_factory_(this) {
On 2013/08/08 23:16:21, yzshen1 wrote:
> please init num_pending_file_resources_.

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host.cc:116: if (files.size() == 0)
{
On 2013/08/08 23:16:21, yzshen1 wrote:
> - Please use files.empty().
> - Does it make sense to share this code with DidCreateResourceHost?

Done. There are a few differences in the similar code; it's not very much to
repeat, I think.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_io_host.cc:250: // necessary in the case
where we're a host for a resource in a trusted
On 2013/08/08 21:24:03, dmichael wrote:
> s/"a trusted plugin"/"the trusted NaCl plugin"
> i.e., this doesn't apply to just any trusted plugin, right?

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_io_host.cc:280: PP_FileSystemType type =
infos[0].file_system_type;
On 2013/08/08 23:16:21, yzshen1 wrote:
> Maybe directly assign to file_system_type_?

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_io_host.cc:283: const std::string&
file_system_url_spec = infos[0].file_system_url_spec;
On 2013/08/08 23:16:21, yzshen1 wrote:
> nit: Maybe merge line 283 and 284?

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/pepper_in_process_resource_creation.cc:90: PP_Instance
instance,
On 2013/08/08 23:16:21, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:48: ppapi::FileRefDetailedInfo
file_info,
On 2013/08/08 23:16:21, yzshen1 wrote:
> const &?

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:138: 0, child_process_id,
resources, &infos));
On 2013/08/08 23:16:21, yzshen1 wrote:
> What does rounting ID 0 mean here?

Added a comment.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:151: return false;
On 2013/08/08 23:16:21, yzshen1 wrote:
> - What if line 136 failed and |infos| is an empty vector?
> - (I haven't checked, just to make sure you have considered that.) Is it okay
> that we return false but change the contents of |dest|?

Thanks, now I check that the sizes of infos and resources match above.
The behavior for |dest| is unchanged from before effectively, so I assume it's
ok.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.h (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.h:28: ::ppapi::URLRequestInfoData*
data,
On 2013/08/08 23:16:21, yzshen1 wrote:
> nit: you don't need '::' at the beginning.

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
File content/renderer/pepper/url_response_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_response_info_util.cc:49: void
DidCreateResourceHost(ppapi::URLResponseInfoData data,
On 2013/08/08 23:16:21, yzshen1 wrote:
> const &, please.

Done.

https://codereview.chromium.org/21966004/diff/142001/content/renderer/pepper/...
content/renderer/pepper/url_response_info_util.cc:55:
base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback, data));
On 2013/08/08 23:16:21, yzshen1 wrote:
> Why we have to post again? Couldn't we run the callback directly?

Changed to run directly.

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/file_chooser...
File ppapi/proxy/file_chooser_resource_unittest.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/file_chooser...
ppapi/proxy/file_chooser_resource_unittest.cc:95: // host resource value set or
deserialization will fail. Since there isn't
On 2013/08/08 23:16:21, yzshen1 wrote:
> Please update the comment accordingly.

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_loader_r...
File ppapi/proxy/url_loader_resource.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_loader_r...
ppapi/proxy/url_loader_resource.cc:7: #include "base/debug/stack_trace.h"
On 2013/08/08 23:16:21, yzshen1 wrote:
> It seems we don't need it?

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_response...
File ppapi/proxy/url_response_info_resource.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/proxy/url_response...
ppapi/proxy/url_response_info_resource.cc:66: body_as_file_ref_ =
FileRefResource::CreateFileRef(
On 2013/08/08 23:16:21, yzshen1 wrote:
> What if the response is not streamed to a file?

I don't know what I was thinking here. Changed to return 0 in this case. Thanks.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_create_info.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.cc:7: #include "base/files/file_path.h"
On 2013/08/08 23:16:21, yzshen1 wrote:
> It is already included in .h.

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.cc:41: if (display_name != "")
On 2013/08/08 21:24:03, dmichael wrote:
> !display_name.empty()?

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_create_info.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:21:
file_system_type(PP_FILESYSTEMTYPE_INVALID),
On 2013/08/08 23:16:21, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:25: PPAPI_SHARED_EXPORT bool IsValid()
const;
On 2013/08/08 23:16:21, yzshen1 wrote:
> Out of curiosity: why putting PPAPI_SHARED_EXPORT here instead of on the
struct
> itself?

I figured I'd put it minimally per-method; that was the style I've seen
elsewhere.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:34: // Since FileRef needs to hold a
FileSystem reference, we need to pass the
On 2013/08/08 23:16:21, yzshen1 wrote:
> Please comment on whether this struct holds 1 ref to the resource.
> (I guess it doesn't, right?)

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:42: MakeExternalFileRefCreateInfo(const
base::FilePath& external_path,
On 2013/08/08 23:16:21, yzshen1 wrote:
> Maybe it is better to make it a static function of FileRefCreateInfo?

What would the benefit be?

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_detailed_info.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_detailed_info.h:17: // the Browser to the renderer
On 2013/08/08 23:16:21, yzshen1 wrote:
> 'b'rowser and trailing '.', please.

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_detailed_info.h:19: PP_Resource resource;
On 2013/08/08 23:16:21, yzshen1 wrote:
> Just to be sure: I think this doesn't carry 1 ref, right?

Done.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
File ppapi/shared_impl/url_request_info_data.cc (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
ppapi/shared_impl/url_request_info_data.cc:7: #include <stdio.h>
On 2013/08/08 23:16:21, yzshen1 wrote:
> nit: an empty line below it, please.
Removed.

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
File ppapi/shared_impl/url_request_info_data.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
ppapi/shared_impl/url_request_info_data.h:42: // ref is releated when BodyItem
is destroyed.
On 2013/08/08 23:16:21, yzshen1 wrote:
> - releated -> released
> - it is more clear to use ScopedPPResource to indicate that a ref is held by
> this object.

I don't use ScopedPPResource here because we send this over IPC.

https://codereview.chromium.org/21966004/diff/142001/ppapi/thunk/ppb_file_ref...
File ppapi/thunk/ppb_file_ref_api.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/thunk/ppb_file_ref...
ppapi/thunk/ppb_file_ref_api.h:18: struct PPB_FileRefCreateInfo;
On 2013/08/08 23:16:21, yzshen1 wrote:
> Is it FileRefCreateInfo (no PPB_)?

Done.

[commit-bot: I haz the power, 2013-08-09 15:12:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/205001

[yzshen1, 2013-08-09 17:11:23]

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_create_info.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:42: MakeExternalFileRefCreateInfo(const
base::FilePath& external_path,
(I don't think it is very important, though.)

The struct/class provides a better namespace, we usually do:
---------------------------------
class A {
...
  static A* Create(...);
};
---------------------------------
Instead of:
---------------------------------
class A {
};

A* CreateA(...)
---------------------------------

On 2013/08/09 02:00:08, teravest wrote:
> On 2013/08/08 23:16:21, yzshen1 wrote:
> > Maybe it is better to make it a static function of FileRefCreateInfo?
> 
> What would the benefit be?

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
File ppapi/shared_impl/url_request_info_data.h (right):

https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
ppapi/shared_impl/url_request_info_data.h:42: // ref is releated when BodyItem
is destroyed.
It seems wrong if you need to send it over IPC:
the file_ref_pp_resource can only be added/released ref in one process (in
out-of-process case it is the plugin process, in in-process case it is the
renderer).
It doesn't make sense if this struct can showed up in multiple processes.

Have I misunderstood anything?
On 2013/08/09 02:00:08, teravest wrote:
> On 2013/08/08 23:16:21, yzshen1 wrote:
> > - releated -> released
> > - it is more clear to use ScopedPPResource to indicate that a ref is held by
> > this object.
> 
> I don't use ScopedPPResource here because we send this over IPC.

[teravest, 2013-08-09 17:40:34]

On Fri, Aug 9, 2013 at 11:11 AM,  <yzshen@chromium.org> wrote:
>
>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
> File ppapi/shared_impl/file_ref_create_info.h (right):
>
>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
> ppapi/shared_impl/file_ref_create_info.h:42:
> MakeExternalFileRefCreateInfo(const base::FilePath& external_path,
> (I don't think it is very important, though.)
>
> The struct/class provides a better namespace, we usually do:
> ---------------------------------
> class A {
> ...
>   static A* Create(...);
> };
> ---------------------------------
> Instead of:
> ---------------------------------
> class A {
> };
>
> A* CreateA(...)
> ---------------------------------
>
>
> On 2013/08/09 02:00:08, teravest wrote:
>>
>> On 2013/08/08 23:16:21, yzshen1 wrote:
>> > Maybe it is better to make it a static function of
>
> FileRefCreateInfo?
>
>> What would the benefit be?
>
>
>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
> File ppapi/shared_impl/url_request_info_data.h (right):
>
>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
> ppapi/shared_impl/url_request_info_data.h:42: // ref is releated when
> BodyItem is destroyed.
> It seems wrong if you need to send it over IPC:
> the file_ref_pp_resource can only be added/released ref in one process
> (in out-of-process case it is the plugin process, in in-process case it
> is the renderer).
> It doesn't make sense if this struct can showed up in multiple
> processes.
>
> Have I misunderstood anything?

Thanks for catching this, now the code only takes/releases the refs on
the plugin side.

>
> On 2013/08/09 02:00:08, teravest wrote:
>>
>> On 2013/08/08 23:16:21, yzshen1 wrote:
>> > - releated -> released
>> > - it is more clear to use ScopedPPResource to indicate that a ref is
>
> held by
>>
>> > this object.
>
>
>> I don't use ScopedPPResource here because we send this over IPC.
>
>
> https://codereview.chromium.org/21966004/

[teravest1, 2013-08-09 18:33:35]

Okay, the ref management should be way better in BodyItem now (it
looks a little simpler to read, too).
I tested locally and saw no calls to AddRefResource or ReleaseResource
which weren't able to look up the target PP_Resource.

On Fri, Aug 9, 2013 at 11:40 AM, Justin TerAvest <teravest@chromium.org> wrote:
> On Fri, Aug 9, 2013 at 11:11 AM,  <yzshen@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
>> File ppapi/shared_impl/file_ref_create_info.h (right):
>>
>>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/file_r...
>> ppapi/shared_impl/file_ref_create_info.h:42:
>> MakeExternalFileRefCreateInfo(const base::FilePath& external_path,
>> (I don't think it is very important, though.)
>>
>> The struct/class provides a better namespace, we usually do:
>> ---------------------------------
>> class A {
>> ...
>>   static A* Create(...);
>> };
>> ---------------------------------
>> Instead of:
>> ---------------------------------
>> class A {
>> };
>>
>> A* CreateA(...)
>> ---------------------------------
>>
>>
>> On 2013/08/09 02:00:08, teravest wrote:
>>>
>>> On 2013/08/08 23:16:21, yzshen1 wrote:
>>> > Maybe it is better to make it a static function of
>>
>> FileRefCreateInfo?
>>
>>> What would the benefit be?
>>
>>
>>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
>> File ppapi/shared_impl/url_request_info_data.h (right):
>>
>>
https://codereview.chromium.org/21966004/diff/142001/ppapi/shared_impl/url_re...
>> ppapi/shared_impl/url_request_info_data.h:42: // ref is releated when
>> BodyItem is destroyed.
>> It seems wrong if you need to send it over IPC:
>> the file_ref_pp_resource can only be added/released ref in one process
>> (in out-of-process case it is the plugin process, in in-process case it
>> is the renderer).
>> It doesn't make sense if this struct can showed up in multiple
>> processes.
>>
>> Have I misunderstood anything?
>
> Thanks for catching this, now the code only takes/releases the refs on
> the plugin side.
>
>>
>> On 2013/08/09 02:00:08, teravest wrote:
>>>
>>> On 2013/08/08 23:16:21, yzshen1 wrote:
>>> > - releated -> released
>>> > - it is more clear to use ScopedPPResource to indicate that a ref is
>>
>> held by
>>>
>>> > this object.
>>
>>
>>> I don't use ScopedPPResource here because we send this over IPC.
>>
>>
>> https://codereview.chromium.org/21966004/

[yzshen1, 2013-08-09 18:38:30]

LGTM

Thanks!

https://codereview.chromium.org/21966004/diff/211003/ppapi/shared_impl/url_re...
File ppapi/shared_impl/url_request_info_data.h (right):

https://codereview.chromium.org/21966004/diff/211003/ppapi/shared_impl/url_re...
ppapi/shared_impl/url_request_info_data.h:40: PP_Resource file_ref_pp_resource;
nit: please add ref that the struct doesn't hold ref to it.
and I think it is good to say that some fields are only valid when |is_file| is
set.

[teravest1, 2013-08-09 19:09:32]

On Fri, Aug 9, 2013 at 12:38 PM,  <yzshen@chromium.org> wrote:
> LGTM
>
> Thanks!
>
>
>
https://codereview.chromium.org/21966004/diff/211003/ppapi/shared_impl/url_re...
> File ppapi/shared_impl/url_request_info_data.h (right):
>
>
https://codereview.chromium.org/21966004/diff/211003/ppapi/shared_impl/url_re...
> ppapi/shared_impl/url_request_info_data.h:40: PP_Resource
> file_ref_pp_resource;
> nit: please add ref that the struct doesn't hold ref to it.
> and I think it is good to say that some fields are only valid when
> |is_file| is set.

Done.

>
> https://codereview.chromium.org/21966004/

[commit-bot: I haz the power, 2013-08-09 19:10:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/255001

[commit-bot: I haz the power, 2013-08-09 19:10:55]

Failed to apply patch for content/renderer/pepper/resource_creation_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/pepper/resource_creation_impl.cc
  Hunk #2 FAILED at 71.
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/pepper/resource_creation_impl.cc.rej

Patch:       content/renderer/pepper/resource_creation_impl.cc
Index: content/renderer/pepper/resource_creation_impl.cc
diff --git a/content/renderer/pepper/resource_creation_impl.cc
b/content/renderer/pepper/resource_creation_impl.cc
index
d2ddd306fe2fcc7805c845a95d73436b7f76826c..9cb647953e0f3e0fcbe4ed7b784984d8f440ba85
100644
--- a/content/renderer/pepper/resource_creation_impl.cc
+++ b/content/renderer/pepper/resource_creation_impl.cc
@@ -8,7 +8,6 @@
 #include "content/renderer/pepper/ppb_audio_impl.h"
 #include "content/renderer/pepper/ppb_broker_impl.h"
 #include "content/renderer/pepper/ppb_buffer_impl.h"
-#include "content/renderer/pepper/ppb_file_ref_impl.h"
 #include "content/renderer/pepper/ppb_flash_message_loop_impl.h"
 #include "content/renderer/pepper/ppb_graphics_3d_impl.h"
 #include "content/renderer/pepper/ppb_image_data_impl.h"
@@ -72,22 +71,6 @@ PP_Resource ResourceCreationImpl::CreateBuffer(PP_Instance
instance,
   return PPB_Buffer_Impl::Create(instance, size);
 }
 
-PP_Resource ResourceCreationImpl::CreateFileRef(
-    PP_Instance instance,
-    PP_Resource file_system,
-    const char* path) {
-  PPB_FileRef_Impl* res = PPB_FileRef_Impl::CreateInternal(
-      instance, file_system, path);
-  return res ? res->GetReference() : 0;
-}
-
-PP_Resource ResourceCreationImpl::CreateFileRef(
-    const ::ppapi::PPB_FileRef_CreateInfo& serialized) {
-  // When we're in-process, the host resource in the create info *is* the
-  // resource, so we don't need to do anything.
-  return serialized.resource.host_resource();
-}
-
 PP_Resource ResourceCreationImpl::CreateFlashDRM(PP_Instance instance) {
   return 0;  // Not supported in-process.
 }

[commit-bot: I haz the power, 2013-08-09 19:32:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/203017

[commit-bot: I haz the power, 2013-08-09 20:19:38]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-09 20:19:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/258001

[teravest, 2013-08-09 20:27:36]

Committed patchset #37 manually as r216744 (presubmit successful).

[elijahtaylor1, 2013-08-10 07:00:49]

On 2013/08/09 20:27:36, teravest wrote:
> Committed patchset #37 manually as r216744 (presubmit successful).

FYI This CL appears to have broken the private CRX FS pepper interface:
crbug.com/271126

[commit-bot: I haz the power, 2013-08-15 19:04:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/269001

[commit-bot: I haz the power, 2013-08-15 20:06:24]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[teravest, 2013-08-15 20:26:59]

I tested locally and resolved the problem for isolated filesystems trivially
(the code was hitting NOTREACHED() where it shouldn't have for
PP_FILESYSTEMTYPE_ISOLATED).
This fixed the issue with a manually run test of the Ext_CrxFileSystem_Private
API.

[yzshen1, 2013-08-15 23:54:25]

On 2013/08/15 20:26:59, teravest wrote:
> I tested locally and resolved the problem for isolated filesystems trivially
> (the code was hitting NOTREACHED() where it shouldn't have for
> PP_FILESYSTEMTYPE_ISOLATED).
> This fixed the issue with a manually run test of the Ext_CrxFileSystem_Private
> API.

Please let me know which patchset I should diff against to see your new change.
Thanks!

[yzshen1, 2013-08-15 23:55:34]

On 2013/08/15 23:54:25, yzshen1 wrote:
> On 2013/08/15 20:26:59, teravest wrote:
> > I tested locally and resolved the problem for isolated filesystems trivially
> > (the code was hitting NOTREACHED() where it shouldn't have for
> > PP_FILESYSTEMTYPE_ISOLATED).
> > This fixed the issue with a manually run test of the
Ext_CrxFileSystem_Private
> > API.
> 
> Please let me know which patchset I should diff against to see your new
change.
> Thanks!

Besides, Bill mentioned that this CL also broke some other NaCl apps, it is good
to test those before landing it.

[teravest, 2013-08-16 01:18:14]

On Thu, Aug 15, 2013 at 5:55 PM,  <yzshen@chromium.org> wrote:
> On 2013/08/15 23:54:25, yzshen1 wrote:
>>
>> On 2013/08/15 20:26:59, teravest wrote:
>> > I tested locally and resolved the problem for isolated filesystems
>> > trivially
>> > (the code was hitting NOTREACHED() where it shouldn't have for
>> > PP_FILESYSTEMTYPE_ISOLATED).
>> > This fixed the issue with a manually run test of the
>
> Ext_CrxFileSystem_Private
>>
>> > API.
>
>
>> Please let me know which patchset I should diff against to see your new
>
> change.
>>
>> Thanks!
>
>
> Besides, Bill mentioned that this CL also broke some other NaCl apps, it is
> good
> to test those before landing it.

Good point. I'll look at that tomorrow and make sure that's good
before landing it.

What do you mean about patchsets you should diff against? The one
previously LGTMed was #37. #38 fixes the isolated filesystem bug, and
#39 brings back the FileChooser test.

>
> https://codereview.chromium.org/21966004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2013-08-16 18:16:51]

I was able to fix the issues with Bastion this morning by modifying the FileIO
PluginResource to take a reference on the FileRef while doing Open(). With the
fix, "From Dust" also seemed to work fine.

I tried some attempts at reproducing this failure in a test, but no luck yet.

[yzshen1, 2013-08-16 18:20:34]

On 2013/08/16 01:18:14, teravest wrote:
> On Thu, Aug 15, 2013 at 5:55 PM,  <mailto:yzshen@chromium.org> wrote:
> > On 2013/08/15 23:54:25, yzshen1 wrote:
> >>
> >> On 2013/08/15 20:26:59, teravest wrote:
> >> > I tested locally and resolved the problem for isolated filesystems
> >> > trivially
> >> > (the code was hitting NOTREACHED() where it shouldn't have for
> >> > PP_FILESYSTEMTYPE_ISOLATED).
> >> > This fixed the issue with a manually run test of the
> >
> > Ext_CrxFileSystem_Private
> >>
> >> > API.
> >
> >
> >> Please let me know which patchset I should diff against to see your new
> >
> > change.
> >>
> >> Thanks!
> >
> >
> > Besides, Bill mentioned that this CL also broke some other NaCl apps, it is
> > good
> > to test those before landing it.
> 
> Good point. I'll look at that tomorrow and make sure that's good
> before landing it.
> 
> What do you mean about patchsets you should diff against? The one
> previously LGTMed was #37. #38 fixes the isolated filesystem bug, and
> #39 brings back the FileChooser test.

I mean, it seems a good idea to ask for another round of code review for your
new changes. What do you think?

> 
> >
> > https://codereview.chromium.org/21966004/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2013-08-16 18:20:34]

On 2013/08/16 01:18:14, teravest wrote:
> On Thu, Aug 15, 2013 at 5:55 PM,  <mailto:yzshen@chromium.org> wrote:
> > On 2013/08/15 23:54:25, yzshen1 wrote:
> >>
> >> On 2013/08/15 20:26:59, teravest wrote:
> >> > I tested locally and resolved the problem for isolated filesystems
> >> > trivially
> >> > (the code was hitting NOTREACHED() where it shouldn't have for
> >> > PP_FILESYSTEMTYPE_ISOLATED).
> >> > This fixed the issue with a manually run test of the
> >
> > Ext_CrxFileSystem_Private
> >>
> >> > API.
> >
> >
> >> Please let me know which patchset I should diff against to see your new
> >
> > change.
> >>
> >> Thanks!
> >
> >
> > Besides, Bill mentioned that this CL also broke some other NaCl apps, it is
> > good
> > to test those before landing it.
> 
> Good point. I'll look at that tomorrow and make sure that's good
> before landing it.
> 
> What do you mean about patchsets you should diff against? The one
> previously LGTMed was #37. #38 fixes the isolated filesystem bug, and
> #39 brings back the FileChooser test.

I mean, it seems a good idea to ask for another round of code review for your
new changes. What do you think?

> 
> >
> > https://codereview.chromium.org/21966004/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[teravest, 2013-08-16 18:42:45]

On Fri, Aug 16, 2013 at 12:20 PM,  <yzshen@chromium.org> wrote:
> On 2013/08/16 01:18:14, teravest wrote:
>
>> On Thu, Aug 15, 2013 at 5:55 PM,  <mailto:yzshen@chromium.org> wrote:
>> > On 2013/08/15 23:54:25, yzshen1 wrote:
>> >>
>> >> On 2013/08/15 20:26:59, teravest wrote:
>> >> > I tested locally and resolved the problem for isolated filesystems
>> >> > trivially
>> >> > (the code was hitting NOTREACHED() where it shouldn't have for
>> >> > PP_FILESYSTEMTYPE_ISOLATED).
>> >> > This fixed the issue with a manually run test of the
>> >
>> > Ext_CrxFileSystem_Private
>> >>
>> >> > API.
>> >
>> >
>> >> Please let me know which patchset I should diff against to see your new
>> >
>> > change.
>> >>
>> >> Thanks!
>> >
>> >
>> > Besides, Bill mentioned that this CL also broke some other NaCl apps, it
>> > is
>> > good
>> > to test those before landing it.
>
>
>> Good point. I'll look at that tomorrow and make sure that's good
>> before landing it.
>
>
>> What do you mean about patchsets you should diff against? The one
>> previously LGTMed was #37. #38 fixes the isolated filesystem bug, and
>> #39 brings back the FileChooser test.
>
>
> I mean, it seems a good idea to ask for another round of code review for
> your
> new changes. What do you think?

Sure, though dmichael is out, and I don't think it needs a security
review again, so I think it would just be you. :)

Is there something explicit I need to do to ask for more rounds of
code review? Or can you just look at the current patch set?

>
>
>> >
>> > https://codereview.chromium.org/21966004/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/21966004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2013-08-16 19:03:53]

LGTM

[teravest, 2013-08-16 22:39:50]

There's a crash in the nacl_integration test after rebasing on Raymes'
change to make CreateResourceHostFromHost create work for N resource
hosts instead of 1.

It's blowing up inside Pickle::size; I'm not sure what's going on yet.

On Fri, Aug 16, 2013 at 1:03 PM,  <yzshen@chromium.org> wrote:
> LGTM
>
> https://codereview.chromium.org/21966004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-08-19 16:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/329001

[commit-bot: I haz the power, 2013-08-19 20:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/329001

[commit-bot: I haz the power, 2013-08-19 20:54:51]

Change committed as 218305

[commit-bot: I haz the power, 2013-08-27 16:54:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/362001

[commit-bot: I haz the power, 2013-08-27 17:09:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/362001

[commit-bot: I haz the power, 2013-08-27 21:18:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/370001

[commit-bot: I haz the power, 2013-08-27 21:35:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/370001

[commit-bot: I haz the power, 2013-08-28 00:29:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/370001

[commit-bot: I haz the power, 2013-08-28 04:20:39]

Change committed as 219911

[teravest, 2013-09-03 21:52:01]

David and Yuzhu, can you take another look?

Patch 48 is the big change to have two ResourceHosts. Patch 49 is a small build
fix I overlooked.
This appears to pass the stress test that demonstrated the message ordering
issue.

[dmichael (off chromium), 2013-09-03 22:34:49]

Some comments before I go home, but I'm not done yet.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:83: message,
&file_system, &internal_path)) {
nit: should this be indented another 4?

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host.cc:193:
scoped_ptr<ppapi::host::ResourceHost>(renderer_host));
I don't think I have a better suggestion, but it feels unfortunate to have to do
this stuff any place in the renderer you know a browser FileRef was created.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:59:
net::EscapePath(internal_path_.substr(1))).spec();
the subtsr(1) looks slightly magical. I guess you're peeling off the leading
'/', and we've already made sure there is one, so it's safe to call substr(1)?
It may be good to have a CHECK and a comment.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:74: return
PP_ERROR_FAILED;
NOTREACHED() also? To catch mistakes in debug builds?

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.h (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.h:26: public
base::SupportsWeakPtr<PepperFileRefRendererHost> {
I don't see you using SupportsWeakPtr for this class...  did I miss it? If you
don't need weak ptrs, please remove. If you do need weak ptrs, but can get by
with WeakPtrFactory, please use that instead.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.h:46: virtual int32_t
OnResourceMessageReceived(
nit: this and IsFileRefHost could be private

[teravest, 2013-09-04 14:46:24]

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:83: message,
&file_system, &internal_path)) {
On 2013/09/03 22:34:50, dmichael wrote:
> nit: should this be indented another 4?
I don't think so.
However, I'm able to barely fit the arguments on the right side, so I changed to
do that.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host.cc:193:
scoped_ptr<ppapi::host::ResourceHost>(renderer_host));
On 2013/09/03 22:34:50, dmichael wrote:
> I don't think I have a better suggestion, but it feels unfortunate to have to
do
> this stuff any place in the renderer you know a browser FileRef was created.

I don't think it's unfortunate. Now that URLLoader and FileIO get information
from the FileRef ResourceHost instead of sending a message to the browser, we
have to create a ResourceHost in the renderer.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:59:
net::EscapePath(internal_path_.substr(1))).spec();
On 2013/09/03 22:34:50, dmichael wrote:
> the subtsr(1) looks slightly magical. I guess you're peeling off the leading
> '/', and we've already made sure there is one, so it's safe to call substr(1)?
> It may be good to have a CHECK and a comment.

Added a check and a comment. Also added checks for ppapi::IsValidInternalPath()
and ppapi::IsValidExternalPath() in the constructors.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:74: return
PP_ERROR_FAILED;
On 2013/09/03 22:34:50, dmichael wrote:
> NOTREACHED() also? To catch mistakes in debug builds?

Done.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.h (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.h:26: public
base::SupportsWeakPtr<PepperFileRefRendererHost> {
On 2013/09/03 22:34:50, dmichael wrote:
> I don't see you using SupportsWeakPtr for this class...  did I miss it? If you
> don't need weak ptrs, please remove. If you do need weak ptrs, but can get by
> with WeakPtrFactory, please use that instead.

Removed.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.h:46: virtual int32_t
OnResourceMessageReceived(
On 2013/09/03 22:34:50, dmichael wrote:
> nit: this and IsFileRefHost could be private

I guess it's legal C++, but it seems weird; why bother making these private?

Regardless, this matches the style of other resource hosts. If you'd like, I can
make another patch to make "OnResourceMessageReceived" and "Is...Host" private
in derived classes throughout, though I don't see much benefit.

[dmichael (off chromium), 2013-09-04 17:23:54]

looks pretty good to me. I think the one thing I care about is that the
PepperFileRefResourceHost can be made to read invalid memory right now.

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:83: message,
&file_system, &internal_path)) {
On 2013/09/04 14:46:25, teravest wrote:
> On 2013/09/03 22:34:50, dmichael wrote:
> > nit: should this be indented another 4?
> I don't think so.
> However, I'm able to barely fit the arguments on the right side, so I changed
to
> do that.
That works for me. (But if that hadn't worked, I think function params are
supposed to be 4-space indented if they don't fit aligned with the paren... I'm
generalizing a bit from the last example in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
)

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_chooser_host.cc (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_chooser_host.cc:193:
scoped_ptr<ppapi::host::ResourceHost>(renderer_host));
On 2013/09/04 14:46:25, teravest wrote:
> On 2013/09/03 22:34:50, dmichael wrote:
> > I don't think I have a better suggestion, but it feels unfortunate to have
to
> do
> > this stuff any place in the renderer you know a browser FileRef was created.
> 
> I don't think it's unfortunate. Now that URLLoader and FileIO get information
> from the FileRef ResourceHost instead of sending a message to the browser, we
> have to create a ResourceHost in the renderer.
I understand it has to be done. What I mean is it's unfortunate that creating
the hosts from the renderer is not encapsulated. Any place else in the future
that we have to create a FileRef from the renderer, we have to do this dance of
sending an appropriate message to the browser, waiting for the response,
creating the PepperFileRefRendererHost, and doing AddPendingResourceHost. It may
not be obvious that you have to do all that when you get the response from the
browser, and it seems kind of messy to do it in more than one place.

I don't know if there's a good way to pull this off, but it would be nice to
have a way to say: "create the necessary FileRef hosts, and call me back when
you're all done".

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.h (right):

https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.h:46: virtual int32_t
OnResourceMessageReceived(
On 2013/09/04 14:46:25, teravest wrote:
> On 2013/09/03 22:34:50, dmichael wrote:
> > nit: this and IsFileRefHost could be private
> 
> I guess it's legal C++, but it seems weird; why bother making these private?
> 
> Regardless, this matches the style of other resource hosts. If you'd like, I
can
> make another patch to make "OnResourceMessageReceived" and "Is...Host" private
> in derived classes throughout, though I don't see much benefit.
Okay, if this matches our other Resource hosts, that's fine. I make almost
everything private I can...  I subscribe to the philosophy of darin in this
thread:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl...
But I don't think it's consistent across the project.

https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):

https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:63:
CHECK(internal_path_[0] == '/');
I guess I meant a CHECK that the size is >= 1. Right now, if somebody misuses
this class, they can create one with an empty string for internal_path_, and
this would be a bad memory read.

Doing:
CHECK(!internal_path_.empty());
would help. Or you could consider hiding the constructors and using static
Create methods so you can fail more gracefully and not have an invalid object.

https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:65:
file_ref_host->GetFileSystemType());
^^^ Leftover debugging? Please remove.

https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:71: // FIXME
^^ Another FIXME

https://codereview.chromium.org/21966004/diff/437001/ppapi/shared_impl/file_r...
File ppapi/shared_impl/file_ref_create_info.h (right):

https://codereview.chromium.org/21966004/diff/437001/ppapi/shared_impl/file_r...
ppapi/shared_impl/file_ref_create_info.h:47: int
renderer_pending_host_resource_id);
completely optional suggestion: It could be easy to mix up browser and renderer,
since they're the same type. You could make a silly little type to combine them
in one parameter:
class PendingHostIds {
 public:
  PendingHostIds();
  PendingHostIds& set_browser(int browser);
  PendingHostIds& set_renderer(int renderer);
 private:
  int browser;
  int renderer;
};
Just to keep this API self-documenting and safer (if a little more verbose).
Used like this:
MakeExternalFileRefCreateInfo(external_path, display_name,
PendingHostIds().set_browser(browser).set_renderer(renderer));

... or whatever. Lots of ways to do this, some simpler.

[teravest, 2013-09-04 17:30:27]

On Wed, Sep 4, 2013 at 11:23 AM,  <dmichael@chromium.org> wrote:
> looks pretty good to me. I think the one thing I care about is that the
> PepperFileRefResourceHost can be made to read invalid memory right now.
>
>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> File content/renderer/pepper/content_renderer_pepper_host_factory.cc
> (right):
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> content/renderer/pepper/content_renderer_pepper_host_factory.cc:83:
> message, &file_system, &internal_path)) {
> On 2013/09/04 14:46:25, teravest wrote:
>>
>> On 2013/09/03 22:34:50, dmichael wrote:
>> > nit: should this be indented another 4?
>> I don't think so.
>> However, I'm able to barely fit the arguments on the right side, so I
>
> changed to
>>
>> do that.
>
> That works for me. (But if that hadn't worked, I think function params
> are supposed to be 4-space indented if they don't fit aligned with the
> paren... I'm generalizing a bit from the last example in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
>
> )

My point is that they already were 4-space indented from the previous line.

>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_file_chooser_host.cc (right):
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> content/renderer/pepper/pepper_file_chooser_host.cc:193:
> scoped_ptr<ppapi::host::ResourceHost>(renderer_host));
> On 2013/09/04 14:46:25, teravest wrote:
>>
>> On 2013/09/03 22:34:50, dmichael wrote:
>> > I don't think I have a better suggestion, but it feels unfortunate
>
> to have to
>>
>> do
>> > this stuff any place in the renderer you know a browser FileRef was
>
> created.
>
>> I don't think it's unfortunate. Now that URLLoader and FileIO get
>
> information
>>
>> from the FileRef ResourceHost instead of sending a message to the
>
> browser, we
>>
>> have to create a ResourceHost in the renderer.
>
> I understand it has to be done. What I mean is it's unfortunate that
> creating the hosts from the renderer is not encapsulated. Any place else
> in the future that we have to create a FileRef from the renderer, we
> have to do this dance of sending an appropriate message to the browser,
> waiting for the response, creating the PepperFileRefRendererHost, and
> doing AddPendingResourceHost. It may not be obvious that you have to do
> all that when you get the response from the browser, and it seems kind
> of messy to do it in more than one place.
>
> I don't know if there's a good way to pull this off, but it would be
> nice to have a way to say: "create the necessary FileRef hosts, and call
> me back when you're all done".

Oh, hmm, yeah. I can try to think of a cleaner way to do this, but I
don't want to block this change on that.

>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_file_ref_renderer_host.h (right):
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> content/renderer/pepper/pepper_file_ref_renderer_host.h:46: virtual
> int32_t OnResourceMessageReceived(
> On 2013/09/04 14:46:25, teravest wrote:
>>
>> On 2013/09/03 22:34:50, dmichael wrote:
>> > nit: this and IsFileRefHost could be private
>
>
>> I guess it's legal C++, but it seems weird; why bother making these
>
> private?
>
>> Regardless, this matches the style of other resource hosts. If you'd
>
> like, I can
>>
>> make another patch to make "OnResourceMessageReceived" and "Is...Host"
>
> private
>>
>> in derived classes throughout, though I don't see much benefit.
>
> Okay, if this matches our other Resource hosts, that's fine. I make
> almost everything private I can...  I subscribe to the philosophy of
> darin in this thread:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl...
> But I don't think it's consistent across the project.
>
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):
>
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
> content/renderer/pepper/pepper_file_ref_renderer_host.cc:63:
> CHECK(internal_path_[0] == '/');
> I guess I meant a CHECK that the size is >= 1. Right now, if somebody
> misuses this class, they can create one with an empty string for
> internal_path_, and this would be a bad memory read.
>
> Doing:
> CHECK(!internal_path_.empty());
> would help. Or you could consider hiding the constructors and using
> static Create methods so you can fail more gracefully and not have an
> invalid object.

Changed to add a check that it's not empty. This is actually safe
anyway due to the call do ppapi::IsValidInternalPath, I think.

>
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
> File content/renderer/pepper/url_request_info_util.cc (right):
>
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
> content/renderer/pepper/url_request_info_util.cc:65:
> file_ref_host->GetFileSystemType());
> ^^^ Leftover debugging? Please remove.

Removed.

>
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
> content/renderer/pepper/url_request_info_util.cc:71: // FIXME
> ^^ Another FIXME

Removed.

>
>
https://codereview.chromium.org/21966004/diff/437001/ppapi/shared_impl/file_r...
> File ppapi/shared_impl/file_ref_create_info.h (right):
>
>
https://codereview.chromium.org/21966004/diff/437001/ppapi/shared_impl/file_r...
> ppapi/shared_impl/file_ref_create_info.h:47: int
> renderer_pending_host_resource_id);
> completely optional suggestion: It could be easy to mix up browser and
> renderer, since they're the same type. You could make a silly little
> type to combine them in one parameter:
> class PendingHostIds {
>  public:
>   PendingHostIds();
>   PendingHostIds& set_browser(int browser);
>   PendingHostIds& set_renderer(int renderer);
>  private:
>   int browser;
>   int renderer;
> };
> Just to keep this API self-documenting and safer (if a little more
> verbose). Used like this:
> MakeExternalFileRefCreateInfo(external_path, display_name,
> PendingHostIds().set_browser(browser).set_renderer(renderer));
>
> ... or whatever. Lots of ways to do this, some simpler.
>
> https://codereview.chromium.org/21966004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2013-09-04 17:40:00]

On 2013/09/04 17:30:27, teravest wrote:
> On Wed, Sep 4, 2013 at 11:23 AM,  <mailto:dmichael@chromium.org> wrote:
> > looks pretty good to me. I think the one thing I care about is that the
> > PepperFileRefResourceHost can be made to read invalid memory right now.
> >
> >
> >
> >
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> > File content/renderer/pepper/content_renderer_pepper_host_factory.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> > content/renderer/pepper/content_renderer_pepper_host_factory.cc:83:
> > message, &file_system, &internal_path)) {
> > On 2013/09/04 14:46:25, teravest wrote:
> >>
> >> On 2013/09/03 22:34:50, dmichael wrote:
> >> > nit: should this be indented another 4?
> >> I don't think so.
> >> However, I'm able to barely fit the arguments on the right side, so I
> >
> > changed to
> >>
> >> do that.
> >
> > That works for me. (But if that hadn't worked, I think function params
> > are supposed to be 4-space indented if they don't fit aligned with the
> > paren... I'm generalizing a bit from the last example in
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> >
> > )
> 
> My point is that they already were 4-space indented from the previous line.
> 
> >
> >
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> > File content/renderer/pepper/pepper_file_chooser_host.cc (right):
> >
> >
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> > content/renderer/pepper/pepper_file_chooser_host.cc:193:
> > scoped_ptr<ppapi::host::ResourceHost>(renderer_host));
> > On 2013/09/04 14:46:25, teravest wrote:
> >>
> >> On 2013/09/03 22:34:50, dmichael wrote:
> >> > I don't think I have a better suggestion, but it feels unfortunate
> >
> > to have to
> >>
> >> do
> >> > this stuff any place in the renderer you know a browser FileRef was
> >
> > created.
> >
> >> I don't think it's unfortunate. Now that URLLoader and FileIO get
> >
> > information
> >>
> >> from the FileRef ResourceHost instead of sending a message to the
> >
> > browser, we
> >>
> >> have to create a ResourceHost in the renderer.
> >
> > I understand it has to be done. What I mean is it's unfortunate that
> > creating the hosts from the renderer is not encapsulated. Any place else
> > in the future that we have to create a FileRef from the renderer, we
> > have to do this dance of sending an appropriate message to the browser,
> > waiting for the response, creating the PepperFileRefRendererHost, and
> > doing AddPendingResourceHost. It may not be obvious that you have to do
> > all that when you get the response from the browser, and it seems kind
> > of messy to do it in more than one place.
> >
> > I don't know if there's a good way to pull this off, but it would be
> > nice to have a way to say: "create the necessary FileRef hosts, and call
> > me back when you're all done".
> 
> Oh, hmm, yeah. I can try to think of a cleaner way to do this, but I
> don't want to block this change on that.
> 
> >
> >
> >
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> > File content/renderer/pepper/pepper_file_ref_renderer_host.h (right):
> >
> >
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
> > content/renderer/pepper/pepper_file_ref_renderer_host.h:46: virtual
> > int32_t OnResourceMessageReceived(
> > On 2013/09/04 14:46:25, teravest wrote:
> >>
> >> On 2013/09/03 22:34:50, dmichael wrote:
> >> > nit: this and IsFileRefHost could be private
> >
> >
> >> I guess it's legal C++, but it seems weird; why bother making these
> >
> > private?
> >
> >> Regardless, this matches the style of other resource hosts. If you'd
> >
> > like, I can
> >>
> >> make another patch to make "OnResourceMessageReceived" and "Is...Host"
> >
> > private
> >>
> >> in derived classes throughout, though I don't see much benefit.
> >
> > Okay, if this matches our other Resource hosts, that's fine. I make
> > almost everything private I can...  I subscribe to the philosophy of
> > darin in this thread:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl...
> > But I don't think it's consistent across the project.
> >
> >
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
> > File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):
> >
> >
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
> > content/renderer/pepper/pepper_file_ref_renderer_host.cc:63:
> > CHECK(internal_path_[0] == '/');
> > I guess I meant a CHECK that the size is >= 1. Right now, if somebody
> > misuses this class, they can create one with an empty string for
> > internal_path_, and this would be a bad memory read.
> >
> > Doing:
> > CHECK(!internal_path_.empty());
> > would help. Or you could consider hiding the constructors and using
> > static Create methods so you can fail more gracefully and not have an
> > invalid object.
> 
> Changed to add a check that it's not empty. This is actually safe
> anyway due to the call do ppapi::IsValidInternalPath, I think.
> 
You mean the call in the constructor? The problem is you can make one of these
with an empty string, and the constructor doesn't really signal error...  it
just returns early. So you can call this method on a PepperFileRefRendererHost
that has an empty string, if I'm reading correctly. With the CHECK now, it's
probably okay now.

lgtm

[teravest, 2013-09-04 17:43:43]

On Wed, Sep 4, 2013 at 11:40 AM,  <dmichael@chromium.org> wrote:
> On 2013/09/04 17:30:27, teravest wrote:
>
>> On Wed, Sep 4, 2013 at 11:23 AM,  <mailto:dmichael@chromium.org> wrote:
>> > looks pretty good to me. I think the one thing I care about is that the
>> > PepperFileRefResourceHost can be made to read invalid memory right now.
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
>>
>> > File content/renderer/pepper/content_renderer_pepper_host_factory.cc
>> > (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
>>
>> > content/renderer/pepper/content_renderer_pepper_host_factory.cc:83:
>> > message, &file_system, &internal_path)) {
>> > On 2013/09/04 14:46:25, teravest wrote:
>> >>
>> >> On 2013/09/03 22:34:50, dmichael wrote:
>> >> > nit: should this be indented another 4?
>> >> I don't think so.
>> >> However, I'm able to barely fit the arguments on the right side, so I
>> >
>> > changed to
>> >>
>> >> do that.
>> >
>> > That works for me. (But if that hadn't worked, I think function params
>> > are supposed to be 4-space indented if they don't fit aligned with the
>> > paren... I'm generalizing a bit from the last example in
>> >
>
>
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
>>
>> >
>> > )
>
>
>> My point is that they already were 4-space indented from the previous
>> line.
>
>
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
>>
>> > File content/renderer/pepper/pepper_file_chooser_host.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
>>
>> > content/renderer/pepper/pepper_file_chooser_host.cc:193:
>> > scoped_ptr<ppapi::host::ResourceHost>(renderer_host));
>> > On 2013/09/04 14:46:25, teravest wrote:
>> >>
>> >> On 2013/09/03 22:34:50, dmichael wrote:
>> >> > I don't think I have a better suggestion, but it feels unfortunate
>> >
>> > to have to
>> >>
>> >> do
>> >> > this stuff any place in the renderer you know a browser FileRef was
>> >
>> > created.
>> >
>> >> I don't think it's unfortunate. Now that URLLoader and FileIO get
>> >
>> > information
>> >>
>> >> from the FileRef ResourceHost instead of sending a message to the
>> >
>> > browser, we
>> >>
>> >> have to create a ResourceHost in the renderer.
>> >
>> > I understand it has to be done. What I mean is it's unfortunate that
>> > creating the hosts from the renderer is not encapsulated. Any place else
>> > in the future that we have to create a FileRef from the renderer, we
>> > have to do this dance of sending an appropriate message to the browser,
>> > waiting for the response, creating the PepperFileRefRendererHost, and
>> > doing AddPendingResourceHost. It may not be obvious that you have to do
>> > all that when you get the response from the browser, and it seems kind
>> > of messy to do it in more than one place.
>> >
>> > I don't know if there's a good way to pull this off, but it would be
>> > nice to have a way to say: "create the necessary FileRef hosts, and call
>> > me back when you're all done".
>
>
>> Oh, hmm, yeah. I can try to think of a cleaner way to do this, but I
>> don't want to block this change on that.
>
>
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
>>
>> > File content/renderer/pepper/pepper_file_ref_renderer_host.h (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/403001/content/renderer/pepper/...
>>
>> > content/renderer/pepper/pepper_file_ref_renderer_host.h:46: virtual
>> > int32_t OnResourceMessageReceived(
>> > On 2013/09/04 14:46:25, teravest wrote:
>> >>
>> >> On 2013/09/03 22:34:50, dmichael wrote:
>> >> > nit: this and IsFileRefHost could be private
>> >
>> >
>> >> I guess it's legal C++, but it seems weird; why bother making these
>> >
>> > private?
>> >
>> >> Regardless, this matches the style of other resource hosts. If you'd
>> >
>> > like, I can
>> >>
>> >> make another patch to make "OnResourceMessageReceived" and "Is...Host"
>> >
>> > private
>> >>
>> >> in derived classes throughout, though I don't see much benefit.
>> >
>> > Okay, if this matches our other Resource hosts, that's fine. I make
>> > almost everything private I can...  I subscribe to the philosophy of
>> > darin in this thread:
>> >
>
>
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl...
>>
>> > But I don't think it's consistent across the project.
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
>>
>> > File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/21966004/diff/437001/content/renderer/pepper/...
>>
>> > content/renderer/pepper/pepper_file_ref_renderer_host.cc:63:
>> > CHECK(internal_path_[0] == '/');
>> > I guess I meant a CHECK that the size is >= 1. Right now, if somebody
>> > misuses this class, they can create one with an empty string for
>> > internal_path_, and this would be a bad memory read.
>> >
>> > Doing:
>> > CHECK(!internal_path_.empty());
>> > would help. Or you could consider hiding the constructors and using
>> > static Create methods so you can fail more gracefully and not have an
>> > invalid object.
>
>
>> Changed to add a check that it's not empty. This is actually safe
>> anyway due to the call do ppapi::IsValidInternalPath, I think.
>
>
> You mean the call in the constructor? The problem is you can make one of
> these
> with an empty string, and the constructor doesn't really signal error...  it
> just returns early. So you can call this method on a
> PepperFileRefRendererHost
> that has an empty string, if I'm reading correctly. With the CHECK now, it's
> probably okay now.

For the sake of pedantry, fs_host_ won't be set unless internal_path
is valid, so the CHECK is still meaningless, but helpful for
documentation.

Admittedly, that's not the easiest thing to pick up from reading
through the code, though.

>
> lgtm
>
>
> https://codereview.chromium.org/21966004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2013-09-04 18:31:48]

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_io_host.cc:236: if
(!resource_host->IsFileRefHost())
Please also check NULL.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:9: #include
"base/files/file_path.h"
nit: There are some duplicate includes. There is no need to inlucde things that
the .h file has included.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:39: 
extra empty line.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:57:
HostGlobals::Get()->GetInstance(instance)->module()->
- Things like GetInstance() may return NULL.
- It seems better to split it into multiple statements.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/url_response_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/url_response_info_util.cc:50: void
DidCreateResourceHosts(const ppapi::URLResponseInfoData& in_data,
It seems the logic of creating both renderer&browser FileRef hosts is repeated
in a few places:
- new a PepperFileRefRendererHost, AddPendingResourceHost();
- CreateBrowserResourceHosts() and the corresponding callback function.

Is it possible that we share the code?

https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
File ppapi/proxy/file_ref_resource.cc (right):

https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
ppapi/proxy/file_ref_resource.cc:42: if
(create_info_.browser_pending_host_resource_id != 0 &&
Do we need to handle the case in which one of them is not 0 while the other is?

[teravest, 2013-09-04 19:28:45]

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_io_host.cc:236: if
(!resource_host->IsFileRefHost())
On 2013/09/04 18:31:49, yzshen1 wrote:
> Please also check NULL.

Done.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_ref_renderer_host.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:9: #include
"base/files/file_path.h"
On 2013/09/04 18:31:49, yzshen1 wrote:
> nit: There are some duplicate includes. There is no need to inlucde things
that
> the .h file has included.

Done.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_ref_renderer_host.cc:39: 
On 2013/09/04 18:31:49, yzshen1 wrote:
> extra empty line.

Done.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:57:
HostGlobals::Get()->GetInstance(instance)->module()->
On 2013/09/04 18:31:49, yzshen1 wrote:
> - Things like GetInstance() may return NULL.
> - It seems better to split it into multiple statements.

Done.

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/url_response_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/url_response_info_util.cc:50: void
DidCreateResourceHosts(const ppapi::URLResponseInfoData& in_data,
On 2013/09/04 18:31:49, yzshen1 wrote:
> It seems the logic of creating both renderer&browser FileRef hosts is repeated
> in a few places:
> - new a PepperFileRefRendererHost, AddPendingResourceHost();
> - CreateBrowserResourceHosts() and the corresponding callback function.
> 
> Is it possible that we share the code?
> 

I added a TODO for this here; I think it'll take a little bit of thought. Is it
ok if I do this in a follow up change?

https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
File ppapi/proxy/file_ref_resource.cc (right):

https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
ppapi/proxy/file_ref_resource.cc:42: if
(create_info_.browser_pending_host_resource_id != 0 &&
On 2013/09/04 18:31:49, yzshen1 wrote:
> Do we need to handle the case in which one of them is not 0 while the other
is?

I'm not sure what the right thing is to do in that case. Today we'd have a CHECK
failure. I can't think of anything much better to do.

[jam, 2013-09-04 19:55:39]

https://codereview.chromium.org/21966004/diff/450001/content/public/renderer/...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/21966004/diff/450001/content/public/renderer/...
content/public/renderer/renderer_ppapi_host.h:53: // Do NOT use this when
dealing with the "NaCl trusted plugin". That plugin
nit: do not mention nacl in content, even in comments. that's a layering
violation. perhaps you want to use "external plugin"

[teravest, 2013-09-04 20:05:40]

On Wed, Sep 4, 2013 at 1:55 PM,  <jam@chromium.org> wrote:
>
>
https://codereview.chromium.org/21966004/diff/450001/content/public/renderer/...
> File content/public/renderer/renderer_ppapi_host.h (right):
>
>
https://codereview.chromium.org/21966004/diff/450001/content/public/renderer/...
> content/public/renderer/renderer_ppapi_host.h:53: // Do NOT use this
> when dealing with the "NaCl trusted plugin". That plugin
> nit: do not mention nacl in content, even in comments. that's a layering
> violation. perhaps you want to use "external plugin"

Fixed. It's harder to parse now, but I guess it's not awful.

>
> https://codereview.chromium.org/21966004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2013-09-04 20:27:22]

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
File content/renderer/pepper/url_response_info_util.cc (right):

https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
content/renderer/pepper/url_response_info_util.cc:50: void
DidCreateResourceHosts(const ppapi::URLResponseInfoData& in_data,
Okay.

On 2013/09/04 19:28:46, teravest wrote:
> On 2013/09/04 18:31:49, yzshen1 wrote:
> > It seems the logic of creating both renderer&browser FileRef hosts is
repeated
> > in a few places:
> > - new a PepperFileRefRendererHost, AddPendingResourceHost();
> > - CreateBrowserResourceHosts() and the corresponding callback function.
> > 
> > Is it possible that we share the code?
> > 
> 
> I added a TODO for this here; I think it'll take a little bit of thought. Is
it
> ok if I do this in a follow up change?

https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
File ppapi/proxy/file_ref_resource.cc (right):

https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
ppapi/proxy/file_ref_resource.cc:42: if
(create_info_.browser_pending_host_resource_id != 0 &&
It is better to explicitly handle that case, either CHECK/DCHECK or using a
static Create() function so that we can return NULL on failure.
I don't think it is very important, maybe DCHECK is good enough.


On 2013/09/04 19:28:46, teravest wrote:
> On 2013/09/04 18:31:49, yzshen1 wrote:
> > Do we need to handle the case in which one of them is not 0 while the other
> is?
> 
> I'm not sure what the right thing is to do in that case. Today we'd have a
CHECK
> failure. I can't think of anything much better to do.

https://codereview.chromium.org/21966004/diff/453001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/21966004/diff/453001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:116: file_ref_ = enter.resource();
(Out of curiosity, I am fine with either way) Why changing from ScopedPPResource
to scoped_refptr<Resource>?

[yzshen1, 2013-09-04 20:28:46]

lgtm

[teravest, 2013-09-04 20:34:32]

On Wed, Sep 4, 2013 at 2:27 PM,  <yzshen@chromium.org> wrote:
>
>
https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
> File content/renderer/pepper/url_response_info_util.cc (right):
>
>
https://codereview.chromium.org/21966004/diff/475001/content/renderer/pepper/...
> content/renderer/pepper/url_response_info_util.cc:50: void
> DidCreateResourceHosts(const ppapi::URLResponseInfoData& in_data,
> Okay.
>
>
> On 2013/09/04 19:28:46, teravest wrote:
>>
>> On 2013/09/04 18:31:49, yzshen1 wrote:
>> > It seems the logic of creating both renderer&browser FileRef hosts
>
> is repeated
>>
>> > in a few places:
>> > - new a PepperFileRefRendererHost, AddPendingResourceHost();
>> > - CreateBrowserResourceHosts() and the corresponding callback
>
> function.
>>
>> >
>> > Is it possible that we share the code?
>> >
>
>
>> I added a TODO for this here; I think it'll take a little bit of
>
> thought. Is it
>>
>> ok if I do this in a follow up change?
>
>
>
https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
> File ppapi/proxy/file_ref_resource.cc (right):
>
>
https://codereview.chromium.org/21966004/diff/475001/ppapi/proxy/file_ref_res...
> ppapi/proxy/file_ref_resource.cc:42: if
> (create_info_.browser_pending_host_resource_id != 0 &&
> It is better to explicitly handle that case, either CHECK/DCHECK or
> using a static Create() function so that we can return NULL on failure.
> I don't think it is very important, maybe DCHECK is good enough.

I've added specific CHECKs() that will be tripped if either of the
pending host resource ids is 0 but not the other.

>
>
>
> On 2013/09/04 19:28:46, teravest wrote:
>>
>> On 2013/09/04 18:31:49, yzshen1 wrote:
>> > Do we need to handle the case in which one of them is not 0 while
>
> the other
>>
>> is?
>
>
>> I'm not sure what the right thing is to do in that case. Today we'd
>
> have a CHECK
>>
>> failure. I can't think of anything much better to do.
>
>
>
https://codereview.chromium.org/21966004/diff/453001/ppapi/proxy/file_io_reso...
> File ppapi/proxy/file_io_resource.cc (right):
>
>
https://codereview.chromium.org/21966004/diff/453001/ppapi/proxy/file_io_reso...
> ppapi/proxy/file_io_resource.cc:116: file_ref_ = enter.resource();
> (Out of curiosity, I am fine with either way) Why changing from
> ScopedPPResource to scoped_refptr<Resource>?

Dave said he'd prefer this approach because it would behave better if
a user (wrongly) freed a resource too early.


>
> https://codereview.chromium.org/21966004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2013-09-04 20:35:42]

>
>
> Dave said he'd prefer this approach because it would behave better if
> a user (wrongly) freed a resource too early.
>

I see. Thanks for explaining.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2013-09-04 22:49:09]

Committed patchset #57 manually as r221284 (presubmit successful).

[commit-bot: I haz the power, 2013-09-17 23:25:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/371002

[commit-bot: I haz the power, 2013-09-17 23:59:07]

Retried try job too often on linux_aura for step(s) aura_unittests,
browser_tests, compositor_unittests, content_browsertests, content_unittests,
interactive_ui_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-18 15:43:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/21966004/453002

[commit-bot: I haz the power, 2013-09-18 22:42:44]

Change committed as 223963