Do PPB_FileIO Query and Read in the plugin process.

Do PPB_FileIO Query and Read in the plugin process.
Use the plugin-side file descriptor for asynchronous
callbacks too.

TBR=dmichael@chromium.org
CL only removes messages that had a LGTM in a prior CL.
https://codereview.chromium.org/18063005/

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

[bbudge, 2013-08-09 05:13:22]

I thought I'd try using the TrackedCallback::CompletionTask CL and was able to
get Read and Query working for all callback types.

Maybe we should land this instead.

[bbudge, 2013-08-09 15:20:12]

The other change did land. I'll put this and the TrackedCallback CLs together.

[dmichael (off chromium), 2013-08-09 17:37:42]

Thanks for doing this...

I'm finding the result passing stuff a bit confusing, so I tried to offer some
suggestions that I hope might simplify that part so it's clearer what's going
on. Let me know what you think.

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
File ppapi/proxy/file_io_resource.cc (right):

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:123: return OnQueryComplete(info, PP_OK);
I think you could pass the result of the query to OnQueryComplete? Especially if
you can make DoQuery return the result. (see comments below about the Read
stuff...  this would be structured in an analogous way)

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:298: scoped_refptr<ReadData> read_data(new
ReadData(bytes_to_read));
It would be better to create an empty ReadData here while the lock is held, and
let DoRead tell it to allocate the read buffer while the lock is not held. Or
something. Just since allocations are relatively slow, and we can fairly easily
do it without the lock. I don't care too much how it's organized to accomplish
that.

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:303: DoRead(offset, bytes_to_read, read_data);
I'm thinking maybe DoRead can return the result (instead of it being in
ReadData, or perhaps in addition, if that's easier).

(You can probably even get rid of ReadData, if you want...  details below...)

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:305: return OnReadComplete(read_data,
array_output, PP_OK);
would it work to pass read_data->bytes_read() instead of PP_OK? I think it might
make OnReadComplete less complex...

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:313: Bind(&FileIOResource::OnFileTaskComplete,
this, callback)));
If you make OnFileTaskComplete take a result parameter, and DoRead returns the
result, you can use PostTaskAndReplyWithResult.

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:315: Bind(&FileIOResource::OnReadComplete, this,
read_data, array_output));
If you are able to remove the "result" part of ReadData, you could use a
heap-allocated std::vector<char> instead of having ReadData.

I.e., before posting DoRead, you have:
std::vector<char> buffer* = new std::vector<char>();

pass it as base::Unretained() to DoRead.
pass it as base::Owned to OnReadComplete.

(This may be more complex than just having scoped_refptr<ReadData>, so take it
as an optional suggestion)

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:346: callback->Run(PP_OK);
I think OnFileTaskComplete should take a result parameter that you pass here.
Then, the "completion_task" will only ever override the result if it fails to do
the array allocation step for array output.

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.cc:358: return result;
Given the way this is all structured currently, I think you should be returning
query_result? It kind of looks like result is always PP_OK. You always pass
PP_OK in the blocking case, and OnFileTaskComplete always passes PP_OK to Run().
But query_result_ may indicate there was a failure.

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
File ppapi/proxy/file_io_resource.h (right):

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.h:97: // and on the plugin's thread (for blocking
calls).
maybe note that they run without the ProxyLock?

https://chromiumcodereview.appspot.com/22646005/diff/9001/ppapi/proxy/file_io...
ppapi/proxy/file_io_resource.h:128: int32_t query_result_;
These aren't *that* big. What about making them parameters to OnQueryComplete
instead and letting them copy, so that we don't have to use members on >1
thread?

[bbudge, 2013-08-09 21:43:23]

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:123: return OnQueryComplete(info, PP_OK);
On 2013/08/09 17:37:42, dmichael wrote:
> I think you could pass the result of the query to OnQueryComplete? Especially
if
> you can make DoQuery return the result. (see comments below about the Read
> stuff...  this would be structured in an analogous way)

Done.

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:298: scoped_refptr<ReadData> read_data(new
ReadData(bytes_to_read));
On 2013/08/09 17:37:42, dmichael wrote:
> It would be better to create an empty ReadData here while the lock is held,
and
> let DoRead tell it to allocate the read buffer while the lock is not held. Or
> something. Just since allocations are relatively slow, and we can fairly
easily
> do it without the lock. I don't care too much how it's organized to accomplish
> that.

It would be messy, as the allocation has to happen while the lock is held in the
non-blocking case.
WDYT?

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:303: DoRead(offset, bytes_to_read, read_data);
On 2013/08/09 17:37:42, dmichael wrote:
> I'm thinking maybe DoRead can return the result (instead of it being in
> ReadData, or perhaps in addition, if that's easier).
> 
> (You can probably even get rid of ReadData, if you want...  details below...)

Done.

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:305: return OnReadComplete(read_data,
array_output, PP_OK);
On 2013/08/09 17:37:42, dmichael wrote:
> would it work to pass read_data->bytes_read() instead of PP_OK? I think it
might
> make OnReadComplete less complex...

Done.

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:313: Bind(&FileIOResource::OnFileTaskComplete,
this, callback)));
On 2013/08/09 17:37:42, dmichael wrote:
> If you make OnFileTaskComplete take a result parameter, and DoRead returns the
> result, you can use PostTaskAndReplyWithResult.

Done.

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:315: Bind(&FileIOResource::OnReadComplete, this,
read_data, array_output));
I couldn't get this to work, since in the abort case it's possible for the file
thread task to have a dangling pointer. (Thread race).

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:346: callback->Run(PP_OK);
On 2013/08/09 17:37:42, dmichael wrote:
> I think OnFileTaskComplete should take a result parameter that you pass here.
> Then, the "completion_task" will only ever override the result if it fails to
do
> the array allocation step for array output.

Done.

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:358: return result;
On 2013/08/09 17:37:42, dmichael wrote:
> Given the way this is all structured currently, I think you should be
returning
> query_result? It kind of looks like result is always PP_OK. You always pass
> PP_OK in the blocking case, and OnFileTaskComplete always passes PP_OK to
Run().
> But query_result_ may indicate there was a failure.

Done.

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
File ppapi/proxy/file_io_resource.h (right):

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:97: // and on the plugin's thread (for blocking
calls).
On 2013/08/09 17:37:42, dmichael wrote:
> maybe note that they run without the ProxyLock?

I removed these and put them on the ops classes.

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:128: int32_t query_result_;
On 2013/08/09 17:37:42, dmichael wrote:
> These aren't *that* big. What about making them parameters to OnQueryComplete
> instead and letting them copy, so that we don't have to use members on >1
> thread?

Added a QueryOp ref counted class to remove these warts.

[bbudge, 2013-08-09 21:58:14]

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/22646005/diff/9001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:298: scoped_refptr<ReadData> read_data(new
ReadData(bytes_to_read));
Turns out I was needlessly worried about allocating and deleting on different
threads. I now defer allocating the buffer until the read, which always happens
outside the lock.

[dmichael (off chromium), 2013-08-09 22:20:56]

Yay, this looks a lot cleaner, thank you!

lgtm

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
File ppapi/proxy/file_io_resource.cc (right):

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.cc:73: file_handle_, offset_, buffer_.get(),
bytes_to_read_);
We don't have the lock, so file_handle_ could be closed. I'm guessing we get a
reasonable error in that situation? And this should only happen if the resource
is destroyed, so the callback should be aborted.

(tl;dr: I think this is safe, but I wanted to mention it in case you can think
of a problem with it)

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.cc:157: Bind(&FileIOResource::OnFileTaskComplete,
this, callback)));
Oh, I think you could just use:
Bind(&TrackedCallback::Run, callback)
and get rid of OnFileTaskComplete

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.cc:342: Bind(&FileIOResource::OnFileTaskComplete,
this, callback)));
ditto

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.cc:373:
ppapi::PlatformFileInfoToPepperFileInfo(query_op->file_info(),
It might be good to call out with a comment that this actually writes to the
plugin's output parameter (info)

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
File ppapi/proxy/file_io_resource.h (right):

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.h:73: QueryOp(PP_FileHandle file_handle);
nit: explicit

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.h:76: // thread (blocking).
please note it does not hold the ProxyLock (same for ReadOp)

https://chromiumcodereview.appspot.com/22646005/diff/18001/ppapi/proxy/file_i...
ppapi/proxy/file_io_resource.h:83: virtual ~QueryOp();
FWIW...  You don't actually need to make it virtual with
RefCounted/RefCountedThreadSafe. That's only necessary if you plan to derive
from QueryOp. (same holds for ReadOp)

[bbudge, 2013-08-09 23:05:28]

Thanks for all of your reviews David. This turned out much better for them.

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

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:73: file_handle_, offset_, buffer_.get(),
bytes_to_read_);
On 2013/08/09 22:20:56, dmichael wrote:
> We don't have the lock, so file_handle_ could be closed. I'm guessing we get a
> reasonable error in that situation? And this should only happen if the
resource
> is destroyed, so the callback should be aborted.
> 
> (tl;dr: I think this is safe, but I wanted to mention it in case you can think
> of a problem with it)

Yes, I think the file call will fail gracefully (even if the handle has been
reassigned and is again valid.)

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:157: Bind(&FileIOResource::OnFileTaskComplete,
this, callback)));
On 2013/08/09 22:20:56, dmichael wrote:
> Oh, I think you could just use:
> Bind(&TrackedCallback::Run, callback)
> and get rid of OnFileTaskComplete

Yep. Done.

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:342: Bind(&FileIOResource::OnFileTaskComplete,
this, callback)));
On 2013/08/09 22:20:56, dmichael wrote:
> ditto

Done.

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:373:
ppapi::PlatformFileInfoToPepperFileInfo(query_op->file_info(),
On 2013/08/09 22:20:56, dmichael wrote:
> It might be good to call out with a comment that this actually writes to the
> plugin's output parameter (info)

Done.

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
File ppapi/proxy/file_io_resource.h (right):

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.h:73: QueryOp(PP_FileHandle file_handle);
On 2013/08/09 22:20:56, dmichael wrote:
> nit: explicit

Done.

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.h:76: // thread (blocking).
On 2013/08/09 22:20:56, dmichael wrote:
> please note it does not hold the ProxyLock (same for ReadOp)

Done.

https://codereview.chromium.org/22646005/diff/18001/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.h:83: virtual ~QueryOp();
On 2013/08/09 22:20:56, dmichael wrote:
> FWIW...  You don't actually need to make it virtual with
> RefCounted/RefCountedThreadSafe. That's only necessary if you plan to derive
> from QueryOp. (same holds for ReadOp)

Done.

[commit-bot: I haz the power, 2013-08-10 00:28:52]

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

[commit-bot: I haz the power, 2013-08-10 00:40:23]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 00:55:35]

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

[commit-bot: I haz the power, 2013-08-10 01:05:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 01:18:29]

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

[bbudge, 2013-08-10 13:39:16]

Committed patchset #6 manually as r216838 (presubmit successful).