Avoid memory allocation for PPB_FileIO Read when callback is blocking.

Avoid memory allocation for PPB_FileIO Read when callback is blocking.
Performs file read directly into caller's buffer.

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

[dmichael (off chromium), 2013-10-22 18:02:58]

lgtm, sorry it took me so long to get to this.

I think it might be good to revisit PPB_FileIO::Read documentation at some
point...  it's very scary right now, but the real takeaway should be that the
memory must remain valid until the operation completes. It seems that all the
stuff about the resource ref count is not necessarily that meaningful in a
multithreaded environment.

https://codereview.chromium.org/27730003/diff/52001/ppapi/proxy/file_io_resou...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/27730003/diff/52001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:333:
array_output.GetDataBuffer(array_output.user_data, bytes_to_read, 1));
I had to read a bunch of code to convince myself we don't leak this in error
conditions for the ReadToArray case. It looks like our C++ wrapper code handles
it properly.

It is slightly confusing. Maybe we should note in the C interface somewhere
(perhaps in PP_ArrayOutput) that the plugin is responsible for freeing the
memory returned by GetDataBuffer when it's finished with that buffer, even in
the case of error.

[bbudge, 2013-10-22 20:51:04]

I've tried to make the docs a little clearer in the blocking callback case.
I also made a few other tweaks.

https://codereview.chromium.org/27730003/diff/52001/ppapi/proxy/file_io_resou...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/27730003/diff/52001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:333:
array_output.GetDataBuffer(array_output.user_data, bytes_to_read, 1));
On 2013/10/22 18:02:58, dmichael wrote:
> I had to read a bunch of code to convince myself we don't leak this in error
> conditions for the ReadToArray case. It looks like our C++ wrapper code
handles
> it properly.
> 
> It is slightly confusing. Maybe we should note in the C interface somewhere
> (perhaps in PP_ArrayOutput) that the plugin is responsible for freeing the
> memory returned by GetDataBuffer when it's finished with that buffer, even in
> the case of error.

Done.

[dmichael (off chromium), 2013-10-22 21:07:51]

Maybe apply the same comments in the C++ versions. O/w, lgtm, thanks!

https://codereview.chromium.org/27730003/diff/152001/ppapi/api/ppb_file_io.idl
File ppapi/api/ppb_file_io.idl (right):

https://codereview.chromium.org/27730003/diff/152001/ppapi/api/ppb_file_io.id...
ppapi/api/ppb_file_io.idl:125: * <code>info</code> must remain valid until after
the callback runs.
This is true not just for blocking callbacks...  and for blocking callbacks,
there really is no callback...

Maybe:
<code>info</code> must remain valud until after callback runs. If you pass a
blocking callback, <code>info</code> must remain valid until after Query
returns.

Similar for Read.

[bbudge, 2013-10-22 22:13:33]

https://codereview.chromium.org/27730003/diff/152001/ppapi/api/ppb_file_io.idl
File ppapi/api/ppb_file_io.idl (right):

https://codereview.chromium.org/27730003/diff/152001/ppapi/api/ppb_file_io.id...
ppapi/api/ppb_file_io.idl:125: * <code>info</code> must remain valid until after
the callback runs.
On 2013/10/22 21:07:51, dmichael wrote:
> This is true not just for blocking callbacks...  and for blocking callbacks,
> there really is no callback...
> 
> Maybe:
> <code>info</code> must remain valud until after callback runs. If you pass a
> blocking callback, <code>info</code> must remain valid until after Query
> returns.
> 
> Similar for Read.

Done.

[commit-bot: I haz the power, 2013-10-22 22:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/27730003/232001

[commit-bot: I haz the power, 2013-10-23 01:49:44]

Change committed as 230282