Implement a proxy for Pepper FileIO.

Implement a proxy for Pepper FileIO.

This splits apart the old in-process implementation into a new object in shared_impl that does most of the general tracking. This alllows that code to be shared by the proxy.

BUG=http://crbug.com/101154

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

[brettw, 2011-11-30 23:00:52]

http://codereview.chromium.org/8764003/diff/1/ppapi/ppapi_shared.gypi
File ppapi/ppapi_shared.gypi (right):

http://codereview.chromium.org/8764003/diff/1/ppapi/ppapi_shared.gypi#newcode46
ppapi/ppapi_shared.gypi:46: 'shared_impl/file_type_conversion.cc',
I moved this from webkit/plugins/ppapi

[viettrungluu, 2011-11-30 23:44:42]

Some initial comments/questions.

http://codereview.chromium.org/8764003/diff/1/ppapi/api/ppb_file_io.idl
File ppapi/api/ppb_file_io.idl (right):

http://codereview.chromium.org/8764003/diff/1/ppapi/api/ppb_file_io.idl#newco...
ppapi/api/ppb_file_io.idl:103: * FileIO object must be opened, and there must be
no other operations
We should have tests for restrictions like this.

http://codereview.chromium.org/8764003/diff/1/ppapi/api/ppb_file_io.idl#newco...
ppapi/api/ppb_file_io.idl:123: * This function will fail if the FileIO object
has not been opened.
Are there any concurrency restrictions for Touch?

http://codereview.chromium.org/8764003/diff/1/ppapi/api/ppb_file_io.idl#newco...
ppapi/api/ppb_file_io.idl:157: * but pending reads can not be interleaved with
other operations.
s/can not/cannot/

http://codereview.chromium.org/8764003/diff/1/ppapi/api/ppb_file_io.idl#newco...
ppapi/api/ppb_file_io.idl:182: * pending writes can not be interleaved with
other operations.
s/can not/cannot/

Also, does "different offsets" mean just the offsets or do we really mean
"disjoint ranges"?

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl.cc
File ppapi/shared_impl/file_io_impl.cc (right):

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl....
ppapi/shared_impl/file_io_impl.cc:177: memcpy(read_buffer, data, pp_error);
include string.h

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl....
ppapi/shared_impl/file_io_impl.cc:178:
RunAndRemoveFirstPendingCallback(pp_error);
How do you know you're running the right callback?

What if the reads complete out of order (and the "user" specified different
callback functions)?

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl.h
File ppapi/shared_impl/file_io_impl.h (right):

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl....
ppapi/shared_impl/file_io_impl.h:124: int32_t CommonCallValidation(bool
should_be_open,
virtual?

[brettw, 2011-12-01 18:50:15]

I think I addressed everything. PTAL

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl.cc
File ppapi/shared_impl/file_io_impl.cc (right):

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl....
ppapi/shared_impl/file_io_impl.cc:178:
RunAndRemoveFirstPendingCallback(pp_error);
I don't know. The existing backend worked like this and I just moved the
behavior to this shared file. I think it must sequence things currently.

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl.h
File ppapi/shared_impl/file_io_impl.h (right):

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl....
ppapi/shared_impl/file_io_impl.h:124: int32_t CommonCallValidation(bool
should_be_open,
No, the comment was wrong, so I fixed it.

[yzshen1, 2011-12-01 19:01:51]

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl.cc
File ppapi/shared_impl/file_io_impl.cc (right):

http://codereview.chromium.org/8764003/diff/1/ppapi/shared_impl/file_io_impl....
ppapi/shared_impl/file_io_impl.cc:178:
RunAndRemoveFirstPendingCallback(pp_error);
On 2011/12/01 18:50:15, brettw wrote:
> I don't know. The existing backend worked like this and I just moved the
> behavior to this shared file. I think it must sequence things currently.

When I wrote this method, I went over the logic to make sure that callbacks were
completed in order.  Hopefully nobody has broken it afterwards. There are some
(rudimentary) tests for this as well.

[viettrungluu, 2011-12-01 22:59:43]

LGTM w/nits, I think (though I'm feeling lazy today).

http://codereview.chromium.org/8764003/diff/6001/ppapi/tests/test_file_io.cc
File ppapi/tests/test_file_io.cc (right):

http://codereview.chromium.org/8764003/diff/6001/ppapi/tests/test_file_io.cc#...
ppapi/tests/test_file_io.cc:907: // Can not query while the write is pending.
nit: s/Can not/Cannot/

They mean different things. E.g., "You can *not* show up to work, but then
you'll get fired." versus "You cannot go to work because you're sick.".

http://codereview.chromium.org/8764003/diff/6001/webkit/plugins/ppapi/ppb_fil...
File webkit/plugins/ppapi/ppb_file_io_impl.h (right):

http://codereview.chromium.org/8764003/diff/6001/webkit/plugins/ppapi/ppb_fil...
webkit/plugins/ppapi/ppb_file_io_impl.h:13: //#include "ppapi/c/pp_file_info.h"
Nit: Eh?