Refactor FileIO to the new design

Refactor FileIO to the new design.

GetOSDescriptor is done by passing raw file descriptor via in-process messaging.

Operations are kept exclusive, but the restriction can be easily removed if needed.

BUG=

[victorhsieh, 2012-11-22 08:16:37]

The refactoring is done, but I have a few questions.

Is the exclusiveness of operation intentional?  If not we can easily allow
random operation order now (except for open,  obviously).  It might be a
performance gain, too.

In IDL, GetOSFileDescriptor is marked as deprecated.  There seems to be a
generator bug that ABI is not touched?

PPAPINaCl* tests depends on FileDownloader and LocalTempFile that are using
GetOSFileDescriptor.  Is that about the "old NaCl proxy" you mentioned earlier?

[brettw, 2012-11-26 18:37:18]

->raymes

[raymes, 2012-11-26 22:30:54]

Some initial comments. I'll finish the review tonight.

https://codereview.chromium.org/11419131/diff/2001/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:84: // Returns the PluginDelegate
for the given plugin instance, or NULL if the
I would change "plugin instance" -> "PP_Instance" in order not to confuse it
with a PluginInstance.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:38: virtual int32_t
OnResourceMessageReceived(
Add "ppapi::host::ResourceHost override"

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:44:
ppapi::host::HostMessageContext* context,
nit: please move this to the previous line and indent the arguments below. (the
same for the other functions here)

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
File ppapi/api/trusted/ppb_file_io_trusted.idl (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
ppapi/api/trusted/ppb_file_io_trusted.idl:11: M14= 0.4
Please fix the spacing while you're here (to M14 = 0.4)

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
ppapi/api/trusted/ppb_file_io_trusted.idl:23: [deprecate=0.5]
You need to add 0.5 to the "label Chrome" above to get it to generate the api.

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

https://codereview.chromium.org/11419131/diff/2001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:23: public
NON_EXPORTED_BASE(thunk::PPB_FileIO_API) {
NON_EXPORTED_BASE shouldn't be needed

https://codereview.chromium.org/11419131/diff/2001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:26: PP_Instance instance);
This should fit on the previous line

https://codereview.chromium.org/11419131/diff/2001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:32: // PPB_FileIO_API overrides.
overrides->implementation

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
My preference would be to not have this base class but to be explicit about the
checks. The reasons being:
-The checks are quite different between the host/plugin side
-Not every function is consistent in the way the checks happen (e.g. there are
already cases where we need to call CommonPreCondition separately and on the
host side CommonPostCondition isn't needed at all). This makes things confusing.
-It eliminates the need to have 2 or 3 functions for every function we implement
- instead we just have one.

I realize that this results in a small amount of code duplication but to me it
results in more understandable code overall.

A helper class could be used to share some logic depending on how this was done.

Functions would then look like:
int32_t Open(...) {
  int rv = CommonPrecondition(...);
  if (rv != PP_OK)
    return rv;
  ...
  CommonPostcondition(...);
}

[raymes, 2012-11-26 22:33:45]

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
File ppapi/api/trusted/ppb_file_io_trusted.idl (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
ppapi/api/trusted/ppb_file_io_trusted.idl:23: [deprecate=0.5]
Also I'm curious what the background is behind this being deprecated?

[brettw, 2012-11-26 23:38:45]

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
File ppapi/api/trusted/ppb_file_io_trusted.idl (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
ppapi/api/trusted/ppb_file_io_trusted.idl:23: [deprecate=0.5]
This was only used to implement the SRPC NaCl proxy.

[raymes, 2012-11-27 06:54:28]

Some more comments but I still have to look at the plugin side of the resource.

With regard to your question on whether 2 operations are allowed to happen
concurrently I'm not sure the answer. Brett might know. If we decide that they
are not allowed to, we should enforce this on the host side. Note that the API
explicitly mentions that operations are exclusive and so if we decide that 2
operations can happen concurrently then it would mean we have to change the API
description.

My feeling for now would be to keep operations exclusive and enforce it in the
host side.

https://codereview.chromium.org/11419131/diff/2001/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:86: virtual
webkit::ppapi::PluginDelegate* GetDelegateForInstance(
The plugin delegate shouldn't be needed for new-style resources. The resource
host should be able to access everything it needs directly (more comments in the
host). I would suggest removing this function. You can still get to the delegate
if needed via the plugininstance.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:7: #include <string>
This should probably be in the header instead

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:17: #include
"ppapi/shared_impl/file_type_conversion.h"
you've included this twice

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:19: #include
"ppapi/shared_impl/time_conversion.h"
you've included this twice

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:31: using
webkit::ppapi::PluginDelegate;
I think this list is slightly out of order (at least the last line is)

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:48: const
PlatformGeneralCallback& callback)
nit: push this onto the previous line

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:58: const base::PlatformFileInfo&
file_info,
nit: Push this onto the previous line

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:103:
PepperFileIOHost::~PepperFileIOHost() {
Should this call into the same code as OnHostMsgClose()?

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:160: 
uneeded newline

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:201:
plugin_delegate_->GetFileThreadMessageLoopProxy(),
Please look at the code for this function in the delegate. You should be able to
replace this call to the delegate with that code directly. Similarly for other
uses of the delegate. The aim is to remove all of these functions from the
delegate.

That said, it's probably better to do this in a separate CL. So please just add
a TODO (probably next to the definition of the plugin_delegate_ instance
variable)

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:382: if
(!quota_file_io_->SetLength(
Why does the existing implementation call plugin_delegate.SetLength here as
opposed to quota_file_io_.SetLength? Also this happens under a different
condition

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:389: if
(!base::FileUtilProxy::Truncate(
You've erased a TODO here. Is http://crbug.com/156077 no longer applicable here?

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:479: pp_error = pp_error == PP_OK
? bytes_read : pp_error;
I think it would be better to avoid doing funny stuff with pp_error in this
function. It makes things very confusing and I guess there is no longer a good
reason to do this.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:481: // Debug checks
I would get rid of these checks as well.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:488: std::string buffer(data,
pp_error > 0 ? pp_error : 0);
Just change this to:
std::string buffer;
if (pp_error == PP_OK)
  buffer.append(data, bytes_read);
reply_context.params.set_result(pp_error);

(or equivalent)

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:499:
reply_context.params.set_result(pp_error == PP_OK ? bytes_written : pp_error);
It would be better to set the result to pp_error and return the # bytes in the
reply message.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:511:
reply_context.params.set_result(bytes_written);
Same here

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:28: class CONTENT_EXPORT
PepperFileIOHost
You probably don't need to export this (and so can remove the #include as well)

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:99: // statelessly and parallelly.
parallely -> in parallel

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:149: // OnHostMsg{OP} to
{OP}Validated.  It is a short-living data and must be
"It is a short-living" -> "It is short-living"

[victorhsieh, 2012-11-27 09:44:42]

https://codereview.chromium.org/11419131/diff/2001/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:84: // Returns the PluginDelegate
for the given plugin instance, or NULL if the
On 2012/11/26 22:30:54, raymes wrote:
> I would change "plugin instance" -> "PP_Instance" in order not to confuse it
> with a PluginInstance.

Deleted

https://codereview.chromium.org/11419131/diff/2001/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:86: virtual
webkit::ppapi::PluginDelegate* GetDelegateForInstance(
On 2012/11/27 06:54:28, raymes wrote:
> The plugin delegate shouldn't be needed for new-style resources. The resource
> host should be able to access everything it needs directly (more comments in
the
> host). I would suggest removing this function. You can still get to the
delegate
> if needed via the plugininstance.

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:7: #include <string>
On 2012/11/27 06:54:28, raymes wrote:
> This should probably be in the header instead

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:17: #include
"ppapi/shared_impl/file_type_conversion.h"
On 2012/11/27 06:54:28, raymes wrote:
> you've included this twice

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:19: #include
"ppapi/shared_impl/time_conversion.h"
On 2012/11/27 06:54:28, raymes wrote:
> you've included this twice

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:31: using
webkit::ppapi::PluginDelegate;
On 2012/11/27 06:54:28, raymes wrote:
> I think this list is slightly out of order (at least the last line is)

How should I sort them?  Capital letter shouldn't come first?

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:48: const
PlatformGeneralCallback& callback)
On 2012/11/27 06:54:28, raymes wrote:
> nit: push this onto the previous line

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:58: const base::PlatformFileInfo&
file_info,
On 2012/11/27 06:54:28, raymes wrote:
> nit: Push this onto the previous line

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:103:
PepperFileIOHost::~PepperFileIOHost() {
On 2012/11/27 06:54:28, raymes wrote:
> Should this call into the same code as OnHostMsgClose()?

Yes, the old implementation does, so I made it the same now.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:160: 
On 2012/11/27 06:54:28, raymes wrote:
> uneeded newline

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:201:
plugin_delegate_->GetFileThreadMessageLoopProxy(),
On 2012/11/27 06:54:28, raymes wrote:
> Please look at the code for this function in the delegate. You should be able
to
> replace this call to the delegate with that code directly. Similarly for other
> uses of the delegate. The aim is to remove all of these functions from the
> delegate.
> 
> That said, it's probably better to do this in a separate CL. So please just
add
> a TODO (probably next to the definition of the plugin_delegate_ instance
> variable)

Done.  Leave a TODO.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:382: if
(!quota_file_io_->SetLength(
On 2012/11/27 06:54:28, raymes wrote:
> Why does the existing implementation call plugin_delegate.SetLength here as
> opposed to quota_file_io_.SetLength? Also this happens under a different
> condition

Thanks for catching!  A change (http://crrev.com/164030) happened after my
initial change so I didn't realize it.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:389: if
(!base::FileUtilProxy::Truncate(
On 2012/11/27 06:54:28, raymes wrote:
> You've erased a TODO here. Is http://crbug.com/156077 no longer applicable
here?

Oops!  Have put it back.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:479: pp_error = pp_error == PP_OK
? bytes_read : pp_error;
On 2012/11/27 06:54:28, raymes wrote:
> I think it would be better to avoid doing funny stuff with pp_error in this
> function. It makes things very confusing and I guess there is no longer a good
> reason to do this.

The callback still expect a parameter that is bytes_read if non-negative, or
pp_error if negative.  We can make the code readable here, but still need to
translate again in the resource.

I leave a comment here for now, but am happy to change for a benefit to move to
resource.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:481: // Debug checks
On 2012/11/27 06:54:28, raymes wrote:
> I would get rid of these checks as well.

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:488: std::string buffer(data,
pp_error > 0 ? pp_error : 0);
On 2012/11/27 06:54:28, raymes wrote:
> Just change this to:
> std::string buffer;
> if (pp_error == PP_OK)
>   buffer.append(data, bytes_read);
> reply_context.params.set_result(pp_error);
> 
> (or equivalent)

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:499:
reply_context.params.set_result(pp_error == PP_OK ? bytes_written : pp_error);
On 2012/11/27 06:54:28, raymes wrote:
> It would be better to set the result to pp_error and return the # bytes in the
> reply message.

Same as read.  Leave a comment for now.  I'm happy to change if there is any
reason to move it to resource.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:511:
reply_context.params.set_result(bytes_written);
On 2012/11/27 06:54:28, raymes wrote:
> Same here

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:28: class CONTENT_EXPORT
PepperFileIOHost
On 2012/11/27 06:54:28, raymes wrote:
> You probably don't need to export this (and so can remove the #include as
well)

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:38: virtual int32_t
OnResourceMessageReceived(
On 2012/11/26 22:30:54, raymes wrote:
> Add "ppapi::host::ResourceHost override"

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:44:
ppapi::host::HostMessageContext* context,
On 2012/11/26 22:30:54, raymes wrote:
> nit: please move this to the previous line and indent the arguments below.
(the
> same for the other functions here)

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:99: // statelessly and parallelly.
On 2012/11/27 06:54:28, raymes wrote:
> parallely -> in parallel

Done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:149: // OnHostMsg{OP} to
{OP}Validated.  It is a short-living data and must be
On 2012/11/27 06:54:28, raymes wrote:
> "It is a short-living" -> "It is short-living"

Done.

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
File ppapi/api/trusted/ppb_file_io_trusted.idl (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
ppapi/api/trusted/ppb_file_io_trusted.idl:11: M14= 0.4
On 2012/11/26 22:30:54, raymes wrote:
> Please fix the spacing while you're here (to M14 = 0.4)

Done.

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
ppapi/api/trusted/ppb_file_io_trusted.idl:23: [deprecate=0.5]
On 2012/11/26 22:30:54, raymes wrote:
> You need to add 0.5 to the "label Chrome" above to get it to generate the api.

Even with 0.5 label above, the generator won't generate the code, until you add
another function for 0.5.

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

https://codereview.chromium.org/11419131/diff/2001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:23: public
NON_EXPORTED_BASE(thunk::PPB_FileIO_API) {
On 2012/11/26 22:30:54, raymes wrote:
> NON_EXPORTED_BASE shouldn't be needed

Done.

https://codereview.chromium.org/11419131/diff/2001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:26: PP_Instance instance);
On 2012/11/26 22:30:54, raymes wrote:
> This should fit on the previous line

Done.

https://codereview.chromium.org/11419131/diff/2001/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:32: // PPB_FileIO_API overrides.
On 2012/11/26 22:30:54, raymes wrote:
> overrides->implementation

Done.

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/11/26 22:30:54, raymes wrote:
> My preference would be to not have this base class but to be explicit about
the
> checks. The reasons being:
> -The checks are quite different between the host/plugin side
> -Not every function is consistent in the way the checks happen (e.g. there are
> already cases where we need to call CommonPreCondition separately and on the
> host side CommonPostCondition isn't needed at all). This makes things
confusing.
> -It eliminates the need to have 2 or 3 functions for every function we
implement
> - instead we just have one.
> 
> I realize that this results in a small amount of code duplication but to me it
> results in more understandable code overall.
> 
> A helper class could be used to share some logic depending on how this was
done.
> 
> Functions would then look like:
> int32_t Open(...) {
>   int rv = CommonPrecondition(...);
>   if (rv != PP_OK)
>     return rv;
>   ...
>   CommonPostcondition(...);
> }

Will wait until the decision of state management on the host side is made.  We
probably want to keep some of the state code shared.

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:260: //if
(!host()->IsRunningInProcess())
Once https://codereview.chromium.org/11414171/ is submitted, we can allow only
in-process access.

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:266:
reinterpret_cast<uintptr_t>(file_);
Sending this pointer through IPC looks weird, maybe it's fine.  But I don't have
windows machine to verify.  If no one can see a problem, I'll have to rely on
trybot to verify for me.

[raymes, 2012-11-27 16:41:29]

It looks like you might have forgotten to update with the latest patch set.
There were many comments marked 'Done' but there was no change in the code.
Thanks!

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:7: #include <string>
On 2012/11/27 09:44:42, Victor Hsieh wrote:
> On 2012/11/27 06:54:28, raymes wrote:
> > This should probably be in the header instead
> 
> Done.

I think you forgot to upload a patch set because I don't see this (or the
comments below) fixed.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:31: using
webkit::ppapi::PluginDelegate;
Oh sorry you're right the order is good.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:149: // OnHostMsg{OP} to
{OP}Validated.  It is a short-living data and must be
Also this doesn't seem updated.

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
File ppapi/api/trusted/ppb_file_io_trusted.idl (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/api/trusted/ppb_file...
ppapi/api/trusted/ppb_file_io_trusted.idl:23: [deprecate=0.5]
Interesting. It worked for me when I recently revved the flash interface but I
have definitely seen it not work in the past. Anyhow it is no big deal for now
since we aren't deprecating the function.

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I'll check with Brett.

You can definitely still have some shared code but I think the idea of having a
shared base class just confuses things. It was a little different under the old
design which the interface into the two classes was identical.

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:260: //if
(!host()->IsRunningInProcess())
You can also rebase on that CL to remove the comment now.

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:266:
reinterpret_cast<uintptr_t>(file_);
I agree - sending the raw file descriptor looks bad even though it's only used
in-process. But I can see why it's the simple approach. I'll get Brett's
thoughts on this.

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

https://codereview.chromium.org/11419131/diff/7002/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:31: // PPB_FileIO_API implementations.
nit: implementations -> implementation

https://codereview.chromium.org/11419131/diff/10003/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/10003/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.h:36: // ppapi::host::ResourceHost
override
nit: period at the end

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

https://codereview.chromium.org/11419131/diff/10003/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:158:
PpapiGlobals::Get()->GetResourceTracker()->GetResource(file_ref_resource);
You can use EnterResourceNoLock here.

https://codereview.chromium.org/11419131/diff/10003/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:268: if (output.is_valid())
If this fails, maybe we should run the callback witha failure?

[raymes, 2012-11-27 21:06:17]

I asked Brett about the two things:
1) Let's keep the operations mutually exclusive as they were before.
2) We can pass the raw file descriptor in the IPC for now. It would be better to
do it the same way as other handles but this might take a bit more work. Please
add a TODO about this though.

[victorhsieh, 2012-11-28 04:11:36]

Operation restriction is added back to the host.

[victorhsieh, 2012-11-28 04:11:55]

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:7: #include <string>
On 2012/11/27 16:41:30, raymes wrote:
> On 2012/11/27 09:44:42, Victor Hsieh wrote:
> > On 2012/11/27 06:54:28, raymes wrote:
> > > This should probably be in the header instead
> > 
> > Done.
> 
> I think you forgot to upload a patch set because I don't see this (or the
> comments below) fixed.

Oops, done.

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/2001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:149: // OnHostMsg{OP} to
{OP}Validated.  It is a short-living data and must be
On 2012/11/27 16:41:30, raymes wrote:
> Also this doesn't seem updated.

Oops, it's somehow got reverted.  Done.

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/11/27 16:41:30, raymes wrote:
> I'll check with Brett.
> 
> You can definitely still have some shared code but I think the idea of having
a
> shared base class just confuses things. It was a little different under the
old
> design which the interface into the two classes was identical.

Agree.  Please let me know the best way to do in the new design.

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/7002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:266:
reinterpret_cast<uintptr_t>(file_);
On 2012/11/27 16:41:30, raymes wrote:
> I agree - sending the raw file descriptor looks bad even though it's only used
> in-process. But I can see why it's the simple approach. I'll get Brett's
> thoughts on this.

Left a TODO per discussion.

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

https://codereview.chromium.org/11419131/diff/7002/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.h:31: // PPB_FileIO_API implementations.
On 2012/11/27 16:41:30, raymes wrote:
> nit: implementations -> implementation

Done.

https://codereview.chromium.org/11419131/diff/10003/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/10003/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.h:36: // ppapi::host::ResourceHost
override
On 2012/11/27 16:41:30, raymes wrote:
> nit: period at the end

Done.

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

https://codereview.chromium.org/11419131/diff/10003/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:158:
PpapiGlobals::Get()->GetResourceTracker()->GetResource(file_ref_resource);
On 2012/11/27 16:41:30, raymes wrote:
> You can use EnterResourceNoLock here.

Done.

https://codereview.chromium.org/11419131/diff/10003/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:268: if (output.is_valid())
On 2012/11/27 16:41:30, raymes wrote:
> If this fails, maybe we should run the callback witha failure?

Done.

[raymes1, 2012-11-28 05:26:26]

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
My suggestion would be to explicitly call the Precondition function before each
operation and the postcondition function after each op (as suggested in the
first comment). The common state tracking code can go in a helper class which is
used by both the host/plugin.

On 2012/11/28 04:11:55, Victor Hsieh wrote:
> On 2012/11/27 16:41:30, raymes wrote:
> > I'll check with Brett.
> > 
> > You can definitely still have some shared code but I think the idea of
having
> a
> > shared base class just confuses things. It was a little different under the
> old
> > design which the interface into the two classes was identical.
> 
> Agree.  Please let me know the best way to do in the new design.

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.cc:273: // TODO: Pass file handle
instead.
-Please add your username to all TODOs
-TODO(victorhsieh): Pass the file handle in the reply params once this works
in-process.

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.h:90: // necessary information to
reply.  This enables the host to handle operations
We should probably remove the last sentence here because it's no longer
completely true

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

https://codereview.chromium.org/11419131/diff/10006/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:111: // Only available when runs in process.
runs->running

https://codereview.chromium.org/11419131/diff/10006/ppapi/shared_impl/ppb_fil...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/10006/ppapi/shared_impl/ppb_fil...
ppapi/shared_impl/ppb_file_io_shared.h:84: // Marks the state of current
operations on started or finished.
on -> as

[victorhsieh, 2012-11-28 07:11:04]

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/2001/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/11/28 05:26:26, raymes1 wrote:
> My suggestion would be to explicitly call the Precondition function before
each
> operation and the postcondition function after each op (as suggested in the
> first comment). The common state tracking code can go in a helper class which
is
> used by both the host/plugin.
> 
> On 2012/11/28 04:11:55, Victor Hsieh wrote:
> > On 2012/11/27 16:41:30, raymes wrote:
> > > I'll check with Brett.
> > > 
> > > You can definitely still have some shared code but I think the idea of
> having
> > a
> > > shared base class just confuses things. It was a little different under
the
> > old
> > > design which the interface into the two classes was identical.
> > 
> > Agree.  Please let me know the best way to do in the new design.
> 

Done.

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.cc:273: // TODO: Pass file handle
instead.
On 2012/11/28 05:26:27, raymes1 wrote:
> -Please add your username to all TODOs
> -TODO(victorhsieh): Pass the file handle in the reply params once this works
> in-process.

Sure.  I'll try to fix the TODO in this week before my leave.

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/10006/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.h:90: // necessary information to
reply.  This enables the host to handle operations
On 2012/11/28 05:26:27, raymes1 wrote:
> We should probably remove the last sentence here because it's no longer
> completely true

Done.

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

https://codereview.chromium.org/11419131/diff/10006/ppapi/proxy/file_io_resou...
ppapi/proxy/file_io_resource.cc:111: // Only available when runs in process.
On 2012/11/28 05:26:27, raymes1 wrote:
> runs->running

Done.

https://codereview.chromium.org/11419131/diff/10006/ppapi/shared_impl/ppb_fil...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/10006/ppapi/shared_impl/ppb_fil...
ppapi/shared_impl/ppb_file_io_shared.h:84: // Marks the state of current
operations on started or finished.
On 2012/11/28 05:26:27, raymes1 wrote:
> on -> as

Done.

[victorhsieh, 2012-11-28 09:52:17]

@jln, please have a look at ppapi_message.h.  Overall the change is a
refactoring and doesn't change the behavior.

Two new messages (GetOSFileDescriptor and the reply) are created in order to
plumb the file descriptor out.  It will be in-process only, but still needs to
go through in-process rpc router.

[raymes, 2012-11-28 18:39:35]

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:154: int32_t rv =
CommonPreCondition(false, OPERATION_EXCLUSIVE);
If we make the PPB_FileIO_Shared a helper class (maybe you can call it
FileIOStateChecker or something, then you can call
state_checker_.CommonPrecondition here (and similarly in the plugin side).

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:159: return
OpenValidated(context->MakeReplyMessageContext(),
We can get rid of the "Validated" functions here and below and just inline the
code.

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:31: public
ppapi::PPB_FileIO_Shared,
We want to remove this as a base class. I think it can just be a helper class.

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:71: int32_t OpenValidated(const
ReplyMessageContext& reply_context,
We want to remove these "validated" functions

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

https://codereview.chromium.org/11419131/diff/5036/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:58: return OpenValidated(enter.resource(),
enter.object(), open_flags, callback);
Again, please remove the "Validated" functions and just move the code into each
function, except for maybe "ReadValidated" where the function is used twice. You
could maybe name it "ReadHelper" though instead.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.cc (right):

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.cc:25: bool
PPB_FileIO_Shared::CheckOpenState(bool should_be_open) const {
We can inline this function, it's only used in one place

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:18: class PPAPI_SHARED_EXPORT
PPB_FileIO_Shared {
Let's change the name to FileIOStateChecker (or maybe you can think of something
better). This should just be a helper class not a base class.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:49: int32_t CommonPreCondition(bool
should_be_open, OperationType new_op);
Let's make this something more descriptive, like "CheckPreOperationState".

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:52: void
CommonPostCondition(OperationType new_op);
This function can be removed - it just calls SetOpInProgress, which can be
called directly and is far more descriptive.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:55: void SetOpInProgress(OperationType
op);
I'd prefer SetOperationInProgress (as opposed to "Op") but let's at least be
consistent across the function names

[raymes, 2012-11-29 00:50:58]

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:93: void
ExecutePlatformGeneralCallback(
We should be able to fit the first argument on the previous line.

https://codereview.chromium.org/11419131/diff/5036/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11419131/diff/5036/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1485: // FileIO, Host -> Plugin.
nit: I would just group these together in the same way that the other IPCs are.
Usually we have call/reply's next to each other.

[victorhsieh, 2012-11-29 06:34:55]

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:154: int32_t rv =
CommonPreCondition(false, OPERATION_EXCLUSIVE);
On 2012/11/28 18:39:35, raymes wrote:
> If we make the PPB_FileIO_Shared a helper class (maybe you can call it
> FileIOStateChecker or something, then you can call
> state_checker_.CommonPrecondition here (and similarly in the plugin side).

Done.

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.cc:159: return
OpenValidated(context->MakeReplyMessageContext(),
On 2012/11/28 18:39:35, raymes wrote:
> We can get rid of the "Validated" functions here and below and just inline the
> code.

Done.

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_file_io_host.h (right):

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:31: public
ppapi::PPB_FileIO_Shared,
On 2012/11/28 18:39:35, raymes wrote:
> We want to remove this as a base class. I think it can just be a helper class.

Done.

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:71: int32_t OpenValidated(const
ReplyMessageContext& reply_context,
On 2012/11/28 18:39:35, raymes wrote:
> We want to remove these "validated" functions

Done.

https://codereview.chromium.org/11419131/diff/5036/content/renderer/pepper/pe...
content/renderer/pepper/pepper_file_io_host.h:93: void
ExecutePlatformGeneralCallback(
On 2012/11/29 00:50:58, raymes wrote:
> We should be able to fit the first argument on the previous line.

Done.

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

https://codereview.chromium.org/11419131/diff/5036/ppapi/proxy/file_io_resour...
ppapi/proxy/file_io_resource.cc:58: return OpenValidated(enter.resource(),
enter.object(), open_flags, callback);
On 2012/11/28 18:39:35, raymes wrote:
> Again, please remove the "Validated" functions and just move the code into
each
> function, except for maybe "ReadValidated" where the function is used twice.
You
> could maybe name it "ReadHelper" though instead.

Done.

https://codereview.chromium.org/11419131/diff/5036/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/11419131/diff/5036/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1485: // FileIO, Host -> Plugin.
On 2012/11/29 00:50:58, raymes wrote:
> nit: I would just group these together in the same way that the other IPCs
are.
> Usually we have call/reply's next to each other.

Done.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.cc (right):

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.cc:25: bool
PPB_FileIO_Shared::CheckOpenState(bool should_be_open) const {
On 2012/11/28 18:39:35, raymes wrote:
> We can inline this function, it's only used in one place

Done.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
File ppapi/shared_impl/ppb_file_io_shared.h (right):

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:18: class PPAPI_SHARED_EXPORT
PPB_FileIO_Shared {
On 2012/11/28 18:39:35, raymes wrote:
> Let's change the name to FileIOStateChecker (or maybe you can think of
something
> better). This should just be a helper class not a base class.

Done.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:49: int32_t CommonPreCondition(bool
should_be_open, OperationType new_op);
On 2012/11/28 18:39:35, raymes wrote:
> Let's make this something more descriptive, like "CheckPreOperationState".

Done.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:52: void
CommonPostCondition(OperationType new_op);
On 2012/11/28 18:39:35, raymes wrote:
> This function can be removed - it just calls SetOpInProgress, which can be
> called directly and is far more descriptive.

Done.

https://codereview.chromium.org/11419131/diff/5036/ppapi/shared_impl/ppb_file...
ppapi/shared_impl/ppb_file_io_shared.h:55: void SetOpInProgress(OperationType
op);
On 2012/11/28 18:39:35, raymes wrote:
> I'd prefer SetOperationInProgress (as opposed to "Op") but let's at least be
> consistent across the function names

Done.

[jln (very slow on Chromium), 2012-11-29 20:34:23]

On 2012/11/28 09:52:17, Victor Hsieh wrote:
> @jln, please have a look at ppapi_message.h.  Overall the change is a
> refactoring and doesn't change the behavior.

This doesn't apply to top of tree, would you mind re-basing it?

> Two new messages (GetOSFileDescriptor and the reply) are created in order to
> plumb the file descriptor out.  It will be in-process only, but still needs to
> go through in-process rpc router.

Ohh, I had not read your message properly and I was just trying to assess that.
Would you mind adding a comment for future reviewers?

How does this work though ? In the previous API, I didn't understand yet how the
file descriptor would end-up in the in process or out of process ppapi plugin.

There are a few subtle differences in terms of attack surface between providing
access to a file via a Read/Write API and providing a file descriptor. So I
would like to make sure this didn't change.

[raymes, 2012-11-29 23:55:19]

lgtm with comments. You will need OWNERS approval from Brett and security
signoff for the IPCs.

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource.cc
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:231: FileIOStateManager::OPERATION_EXCLUSIVE ||
Indentation here looks wrong. Please align with the opening "(" and the same for
all following lines.

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:242: FileIOStateManager::OPERATION_EXCLUSIVE);
indentation

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:255: FileIOStateManager::OPERATION_EXCLUSIVE);
indentation

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:267: FileIOStateManager::OPERATION_READ);
indentation

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
File ppapi/shared_impl/file_io_state_manager.h (right):

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:16: // operation could be recjected
because of the current pending operation.  Also,
-recjected->rejected
-There are 2 spaces at the end of the sentence. Please change it to 1. There are
more cases of this below.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:40: OperationType
GetPendingOperation() const { return pending_op_; }
Check the style guide for naming of inline getters. This should be called
get_pending_operation().

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:44: // Called before every "Validated"
function.  It is responsible to make sure
There are no longer "Validated" functions. Just say it is called before each
FileIO operation begins.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:45: // that "state" is correct.  For
example, some operations are only valid after
"state"->state (no quotations needed)

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:46: // the file is opened, or
operations might need to run excludively.  See
-excludively->exclusively
-There are no subclasses. Remove the last sentence

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:54: void OperationFinished();
I would change this to SetOperationFinished to be consistent

[brettw, 2012-11-30 00:01:56]

owners lgtm

[victorhsieh, 2012-11-30 01:17:21]

Done and rebased.

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource.cc
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:231: FileIOStateManager::OPERATION_EXCLUSIVE ||
On 2012/11/29 23:55:19, raymes wrote:
> Indentation here looks wrong. Please align with the opening "(" and the same
for
> all following lines.

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:242: FileIOStateManager::OPERATION_EXCLUSIVE);
On 2012/11/29 23:55:19, raymes wrote:
> indentation

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:255: FileIOStateManager::OPERATION_EXCLUSIVE);
On 2012/11/29 23:55:19, raymes wrote:
> indentation

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/proxy/file_io_resource...
ppapi/proxy/file_io_resource.cc:267: FileIOStateManager::OPERATION_READ);
On 2012/11/29 23:55:19, raymes wrote:
> indentation

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
File ppapi/shared_impl/file_io_state_manager.h (right):

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:16: // operation could be recjected
because of the current pending operation.  Also,
On 2012/11/29 23:55:19, raymes wrote:
> -recjected->rejected
> -There are 2 spaces at the end of the sentence. Please change it to 1. There
are
> more cases of this below.

I do see other 2 spaces after period in other ppapi code, but  will make it 1
here.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:40: OperationType
GetPendingOperation() const { return pending_op_; }
On 2012/11/29 23:55:19, raymes wrote:
> Check the style guide for naming of inline getters. This should be called
> get_pending_operation().

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:44: // Called before every "Validated"
function.  It is responsible to make sure
On 2012/11/29 23:55:19, raymes wrote:
> There are no longer "Validated" functions. Just say it is called before each
> FileIO operation begins.

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:45: // that "state" is correct.  For
example, some operations are only valid after
On 2012/11/29 23:55:19, raymes wrote:
> "state"->state (no quotations needed)

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:46: // the file is opened, or
operations might need to run excludively.  See
On 2012/11/29 23:55:19, raymes wrote:
> -excludively->exclusively
> -There are no subclasses. Remove the last sentence

Done.

https://codereview.chromium.org/11419131/diff/77/ppapi/shared_impl/file_io_st...
ppapi/shared_impl/file_io_state_manager.h:54: void OperationFinished();
On 2012/11/29 23:55:19, raymes wrote:
> I would change this to SetOperationFinished to be consistent

Done.

[jln (very slow on Chromium), 2012-11-30 23:16:53]

IPC lgtm on the basis that this is a re-factor that doesn't create additional
attack surface.

I didn't perform a security review of these IPCs though.

[commit-bot: I haz the power, 2012-12-01 00:05:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11419131/9018

[commit-bot: I haz the power, 2012-12-01 00:05:31]

Presubmit check for 11419131-9018 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
Missing PPAPI header, no change or skipped generation?

***************
ppapi/api/trusted/ppb_file_io_trusted.idl
***************

Presubmit checks took 6.5s to calculate.

[commit-bot: I haz the power, 2012-12-01 00:30:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11419131/14043

[commit-bot: I haz the power, 2012-12-01 00:58:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-01 01:03:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11419131/14045

[commit-bot: I haz the power, 2012-12-01 02:40:34]

Retried try job too often on mac_rel for step(s) nacl_integration

[victorhsieh, 2012-12-01 06:10:59]

A NaCl chrome_browser_tests crashed at the DCHECK in
ppapi_host.cc:OnHostMsgResourceCreated.

When pm_manifest_file_test.cc:ManifestOpenTest runs,
ContentRendererPepperHostFactory::CreateResourceHost returns a null host because
host_->IsValidInstance(instance) is false.

Going deeper, GetAndValidateInstance returns null because instance->module() has
been changed by ppapi_plugin_instance.cc:ResetAsProxied().

My knowledge is limited here.  What's the right fix here?

Command to reproduce the failure:
  (cd native_client; ./scons MODE=opt-linux,nacl platform=x86-64
chrome_browser_path=../out/Debug/chrome chrome_browser_tests)

Failed test:
ManifestOpenTest in
ppapi/native_client/tests/nacl_browser/manifest_file/pm_manifest_file_test.cc

[victorhsieh, 2012-12-04 01:12:30]

Hi Bill, do you have an idea about the issue here?

[victorhsieh, 2012-12-04 14:19:41]

The underlying infrastructure of those tests actually replies on FileIO and
URLLoader (Brett, so your refactoring might have the same issue?).  In the
initial phase of the test, it relies on FileDownloader (as trusted code) to
open the NaCl manifest it downloads (in this case, the unit test nmf).  It
uses pp::FileIO for that, before the actual tests.
 Later, InitAsProxiedNaCl is called when starting ppapi proxy, and that
change the module of the plugin instance.  By the time the actual
(untrusted) test is run, ContentRendererPepperHostFactory tries to create
another PepperFileIOHost by first making sure the instance is valid.  But
the module has been changed, thus the factory refuses to create a host for
invalid instance.

I'm not sure this behavior is test only or not (I guess not, since NaCl
tests in browser_tests passed?).  How should we fix it?  I probably have to
leave the change to someone if there is no simply workaround (e.g.
disable IsValidInstance() check in ContentRendererPepperHostFactory).
 Please suggest the next step.  Thanks.

On Tue, Dec 4, 2012 at 9:12 AM, <victorhsieh@chromium.org> wrote:

> Hi Bill, do you have an idea about the issue here?
>
>
https://codereview.chromium.**org/11419131/<https://codereview.chromium.org/1...
>

[raymes1, 2012-12-04 15:53:39]

Yes this seems bad and my understanding of the NaCl stuff is also
limited. Bill or Brett may be able to help out here. My guess would be
that ResetAsProxied should also trigger updating the module_ instance
variable in the RendererPpapiHostImpl that's associated with the
instance. This might need a little plumbing to make it work.

On Tue, Dec 4, 2012 at 6:19 AM, Victor Hsieh <victorhsieh@chromium.org> wrote:
> The underlying infrastructure of those tests actually replies on FileIO and
> URLLoader (Brett, so your refactoring might have the same issue?).  In the
> initial phase of the test, it relies on FileDownloader (as trusted code) to
> open the NaCl manifest it downloads (in this case, the unit test nmf).  It
> uses pp::FileIO for that, before the actual tests.  Later, InitAsProxiedNaCl
> is called when starting ppapi proxy, and that change the module of the
> plugin instance.  By the time the actual (untrusted) test is run,
> ContentRendererPepperHostFactory tries to create another PepperFileIOHost by
> first making sure the instance is valid.  But the module has been changed,
> thus the factory refuses to create a host for invalid instance.
>
> I'm not sure this behavior is test only or not (I guess not, since NaCl
> tests in browser_tests passed?).  How should we fix it?  I probably have to
> leave the change to someone if there is no simply workaround (e.g. disable
> IsValidInstance() check in ContentRendererPepperHostFactory).  Please
> suggest the next step.  Thanks.
>
> On Tue, Dec 4, 2012 at 9:12 AM, <victorhsieh@chromium.org> wrote:
>>
>> Hi Bill, do you have an idea about the issue here?
>>
>> https://codereview.chromium.org/11419131/
>
>

[dmichael(do not use this one), 2012-12-04 22:24:22]

On Tue, Dec 4, 2012 at 2:24 PM, Brett Wilson <brettw@chromium.org> wrote:

> +dmichael
>
> I did some looking at this.
>
> The "nacl plugin" (which is the trusted code that starts NaCl
> developed by Google) starts off as an in-process plugin. It has a
> PluginModule and a RendererPpapiHost. It uses this weird in-process
> router thing to make the resources work. It will use the URLLoader and
> FileIO pepper functions to load the nexe (the untrusted code from the
> web) from the web,
>
> When we load the nexe and start it, we do this crazy thing where we
> switch it over to using the out-of-process implementation (since as
> far as WebKit is concerned, there has only bee one plugin), but from
> our perspective, there are two. The most interesting function here is
> PluginInstance::ResetAsProxied.
>
> Apparently what happens is that we create a new PluginModule and
> RendererPpapiHost that's proxied (the original one was in-process) and
> switch the PluginInstance over to using that one. I wasn't really
> aware this was how it works until I just looked at this. From looking
> at it, it looks consistent to me, but it's weird that there are two
> modules and two RendererPpapiHosts, but one PluginInstance.
>
Yes, it's tricky. I was afraid this might cause trouble. The problem is:
- Every NaCl app instance runs in its own process
- therefore, every NaCl instance needs its own out-of-process proxy
- therefore, every NaCl instance needs its own PluginModule (which owns
each proxy)

This is all mostly workable, except it all goes through the same NaCl
trusted in-process plugin, as you found. You could maybe argue that there
should be 2 PpapiPluginInstances for each actual NaCl instance (one
representing the NaCl plugin, and one for the untrusted app). But as you
say, WebKit only sees it as one instance.

Longer-term, the sane thing to do is to get rid of the trusted NaCl plugin,
so we don't access the APIs from two different places (i.e., plugin and
untrusted app). Shorter-term, we've admittedly been crossing our fingers a
little bit and putting band-aids on the problems that turn up.

I'd have to look a bit harder to see what the best fix is here, but I
suspect it's not too hard to either:
- have a separate set of Host impl stuff to service the NaCl app
or
- Reset the existing host impl stuff to properly talk to the NaCl app



> One guess as to what's happening for you is that the nacl plugin is
> doing something after this switch has taken place. If we get anything
> coming in on the in-process one (like it's doing some FileIO after the
> switchover), it will check and see that there is no instance
> associated with it, since it was moved to the out-of-process one.
>
> Can you verify that's what's happening? If not, can you figure out
> what's happening with this background information? Once we know
> exactly what's happening we can work on a solution.
>
> Brett
>
> On Tue, Dec 4, 2012 at 7:53 AM, Raymes Khoury <raymes@google.com> wrote:
> > Yes this seems bad and my understanding of the NaCl stuff is also
> > limited. Bill or Brett may be able to help out here. My guess would be
> > that ResetAsProxied should also trigger updating the module_ instance
> > variable in the RendererPpapiHostImpl that's associated with the
> > instance. This might need a little plumbing to make it work.
> >
> > On Tue, Dec 4, 2012 at 6:19 AM, Victor Hsieh <victorhsieh@chromium.org>
> wrote:
> >> The underlying infrastructure of those tests actually replies on FileIO
> and
> >> URLLoader (Brett, so your refactoring might have the same issue?).  In
> the
> >> initial phase of the test, it relies on FileDownloader (as trusted
> code) to
> >> open the NaCl manifest it downloads (in this case, the unit test nmf).
>  It
> >> uses pp::FileIO for that, before the actual tests.  Later,
> InitAsProxiedNaCl
> >> is called when starting ppapi proxy, and that change the module of the
> >> plugin instance.  By the time the actual (untrusted) test is run,
> >> ContentRendererPepperHostFactory tries to create another
> PepperFileIOHost by
> >> first making sure the instance is valid.  But the module has been
> changed,
> >> thus the factory refuses to create a host for invalid instance.
> >>
> >> I'm not sure this behavior is test only or not (I guess not, since NaCl
> >> tests in browser_tests passed?).  How should we fix it?  I probably
> have to
> >> leave the change to someone if there is no simply workaround (e.g.
> disable
> >> IsValidInstance() check in ContentRendererPepperHostFactory).  Please
> >> suggest the next step.  Thanks.
> >>
> >> On Tue, Dec 4, 2012 at 9:12 AM, <victorhsieh@chromium.org> wrote:
> >>>
> >>> Hi Bill, do you have an idea about the issue here?
> >>>
> >>> https://codereview.chromium.org/11419131/
> >>
> >>
>