Replace FileUtilProxy with FileProxy in renderer_host/pepper

Replace FileUtilProxy with FileProxy in renderer_host/pepper

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

[rvargas (doing something else), 2014-04-26 01:03:03]

This is not ready for review but I need some feedback in how to proceed.

Given that all operations on |file_| are performed through the file util proxy,
I'm replacing it with FileProxy (the equivalent methods of FileUtilProxy are
going to be removed).

However, file_ can also be created through the webkit fileapi and that poses a
question.

FileProxy assumes that it will be the proxy of all operations, including Create
because there is an implicit need to use a worker thread for everything. On the
other hand, fileapi also provides complete functionality over what can be done
with the files it creates[*]

But this code grabs a handle from fileapi and starts using it directly with the
base facilities (proxy etc).

My first reaction is that this code should be split so that if we are dealing
with a fileapi handle all operations are performed through that api.

I can modify FileProxy to allow grabbing a File, but that doesn't seem very
clean to me.

[*] at least some version of webkit fileapi does that. I just saw that
FileSystemOperationRunner doesn't. I'm not very familiar with how all things
under fileapi fit together.

[bbudge, 2014-04-28 18:21:25]

On 2014/04/26 01:03:03, rvargas wrote:
> This is not ready for review but I need some feedback in how to proceed.
> 
> Given that all operations on |file_| are performed through the file util
proxy,
> I'm replacing it with FileProxy (the equivalent methods of FileUtilProxy are
> going to be removed).
> 
> However, file_ can also be created through the webkit fileapi and that poses a
> question.
> 
> FileProxy assumes that it will be the proxy of all operations, including
Create
> because there is an implicit need to use a worker thread for everything. On
the
> other hand, fileapi also provides complete functionality over what can be done
> with the files it creates[*]
> 
> But this code grabs a handle from fileapi and starts using it directly with
the
> base facilities (proxy etc).
> 
> My first reaction is that this code should be split so that if we are dealing
> with a fileapi handle all operations are performed through that api.
> 
> I can modify FileProxy to allow grabbing a File, but that doesn't seem very
> clean to me.
> 
> [*] at least some version of webkit fileapi does that. I just saw that
> FileSystemOperationRunner doesn't. I'm not very familiar with how all things
> under fileapi fit together.

I'm not an expert on the stuff in webkit/fileapi either. In the past, we haven't
had to worry about where the file handle comes from. I'm not very familiar with
base::FileProxy but at first glance it doesn't seem so terrible to make SetFile
public and allow the class to adopt a previously existing file.

One other thing to keep in mind is that file read, write, and query take place
in the plugin process, and FileProxy isn't ported to NaCl, so it's impossible to
use FileProxy for all file operations.

[rvargas (doing something else), 2014-04-28 19:06:33]

On 2014/04/28 18:21:25, bbudge wrote:
> On 2014/04/26 01:03:03, rvargas wrote:
> > This is not ready for review but I need some feedback in how to proceed.
> > 
> > Given that all operations on |file_| are performed through the file util
> proxy,
> > I'm replacing it with FileProxy (the equivalent methods of FileUtilProxy are
> > going to be removed).
> > 
> > However, file_ can also be created through the webkit fileapi and that poses
a
> > question.
> > 
> > FileProxy assumes that it will be the proxy of all operations, including
> Create
> > because there is an implicit need to use a worker thread for everything. On
> the
> > other hand, fileapi also provides complete functionality over what can be
done
> > with the files it creates[*]
> > 
> > But this code grabs a handle from fileapi and starts using it directly with
> the
> > base facilities (proxy etc).
> > 
> > My first reaction is that this code should be split so that if we are
dealing
> > with a fileapi handle all operations are performed through that api.
> > 
> > I can modify FileProxy to allow grabbing a File, but that doesn't seem very
> > clean to me.
> > 
> > [*] at least some version of webkit fileapi does that. I just saw that
> > FileSystemOperationRunner doesn't. I'm not very familiar with how all things
> > under fileapi fit together.
> 
> I'm not an expert on the stuff in webkit/fileapi either. In the past, we
haven't
> had to worry about where the file handle comes from. I'm not very familiar
with
> base::FileProxy but at first glance it doesn't seem so terrible to make
SetFile
> public and allow the class to adopt a previously existing file.
> 
> One other thing to keep in mind is that file read, write, and query take place
> in the plugin process, and FileProxy isn't ported to NaCl, so it's impossible
to
> use FileProxy for all file operations.

Thanks.

What does it mean that FileProxy has not been ported to NaCl? The only use of
FileUtilProxy from NaCl that I can see seems unrelated to this code (and it is
being fixed in https://codereview.chromium.org/251883002/)

[rvargas (doing something else), 2014-04-28 19:15:15]

@kinuko: Do you mind looking at my first comment for reference? I'm a little
hesitant to add a setter to FileProxy, but I really don't know how
FileSystemOperationRunner fits with the rest of the WebKit Fileapi (in
particular the OpenFile method).

[kinuko, 2014-04-30 04:44:35]

(Sorry for delayed response, Tokyo's in holiday season this/next week)

On 2014/04/28 19:15:15, rvargas wrote:
> @kinuko: Do you mind looking at my first comment for reference? I'm a little
> hesitant to add a setter to FileProxy, but I really don't know how
> FileSystemOperationRunner fits with the rest of the WebKit Fileapi (in
> particular the OpenFile method).

FileSystemOperationRunner is just a thin wrapper of FileSystemOperation, which
is the central point we proxy things and dispatch appropriate filesystem
operations to access sandboxed filesystem via FileAPI.  FileAPI code does
multiple things while proxying a file task like OpenFile, e.g. checking quota
(creating a file consumes a bit of quota), looking for the correct sandbox
partition, recording opened files for unflushed quota calculation, etc etc.  In
the sense I can't think of an easy way to just use FileProxy for the part of the
code, and SetFile() approach sounds more practical to me.

[rvargas (doing something else), 2014-05-01 01:35:07]

On 2014/04/30 04:44:35, kinuko wrote:
> (Sorry for delayed response, Tokyo's in holiday season this/next week)
> 
> On 2014/04/28 19:15:15, rvargas wrote:
> > @kinuko: Do you mind looking at my first comment for reference? I'm a little
> > hesitant to add a setter to FileProxy, but I really don't know how
> > FileSystemOperationRunner fits with the rest of the WebKit Fileapi (in
> > particular the OpenFile method).
> 
> FileSystemOperationRunner is just a thin wrapper of FileSystemOperation, which
> is the central point we proxy things and dispatch appropriate filesystem
> operations to access sandboxed filesystem via FileAPI.  FileAPI code does
> multiple things while proxying a file task like OpenFile, e.g. checking quota
> (creating a file consumes a bit of quota), looking for the correct sandbox
> partition, recording opened files for unflushed quota calculation, etc etc. 
In
> the sense I can't think of an easy way to just use FileProxy for the part of
the
> code, and SetFile() approach sounds more practical to me.

Thanks!,

I was not really thinking about using FileProxy directly in other parts of the
code. What bothers me a little here is actually starting to use FileProxy "in
the middle" of something else that maybe should be handled completely within
FileAPI. What I mean is that to me it is easy to reason about something like a
sandboxed file system if all operations go through the same API (even if parts
of that API are implemented as thin wrappers around something else).

In other words, given that (FileAPI::)OpenFile has to be used to open a file, it
would be better if (FileAPI::)Foo had to be used to do other operations on that
file instead of starting to use base::FileProxy directly.

On the other hand, I'm getting close to having to attack FileAPI itself to
remove PlatformFile, and then the issue may be more evident as if things get
back a base::File there's always the question about whether to use file.Foo() or
api::Foo(&file). Although in practice it's not that different today and using
FooPlatformFile() directly.

[rvargas (doing something else), 2014-05-09 22:10:19]

I think it is ready for review. PTAL.

[rvargas (doing something else), 2014-05-14 02:32:10]

On 2014/05/09 22:10:19, rvargas wrote:
> I think it is ready for review. PTAL.

bbudge: ping

[bbudge, 2014-05-14 16:11:58]

LGTM w/comment

https://codereview.chromium.org/252583007/diff/40001/base/files/file_proxy.h
File base/files/file_proxy.h (right):

https://codereview.chromium.org/252583007/diff/40001/base/files/file_proxy.h#...
base/files/file_proxy.h:92: void SetFile(File file);
It seems logical to put this with the create functions. Just below?
Also, the comment isn't clear about how file_ might be set already. How about
this?

// Sets the file to one that is already open. |file_| should not have been set
by calling CreateOrOpen or CreateTemporary.

[rvargas (doing something else), 2014-05-14 19:47:26]

Thanks!

https://codereview.chromium.org/252583007/diff/40001/base/files/file_proxy.h
File base/files/file_proxy.h (right):

https://codereview.chromium.org/252583007/diff/40001/base/files/file_proxy.h#...
base/files/file_proxy.h:92: void SetFile(File file);
On 2014/05/14 16:11:58, bbudge wrote:
> It seems logical to put this with the create functions. Just below?

The only methods in between Create and this one are IsValid(), which is a
concept implicitly needed by this method, and created(), which is only relevant
for Create and not this one. It also sits just by TakeFile() and
GetPlatformFile() which are closely related to SetFile (maybe more than Create)
so it seems better to keep it here.

> Also, the comment isn't clear about how file_ might be set already. How about
> this?
> 
> // Sets the file to one that is already open. |file_| should not have been set
> by calling CreateOrOpen or CreateTemporary.

The problem is that the requirement is not exactly that Create has not been
called, but that IsValid() should return false. Create may fail, or even succeed
as long as TakeFile() or Close() is called after that.

I updated the comments.

[bbudge, 2014-05-14 19:58:57]

On 2014/05/14 19:47:26, rvargas wrote:
> Thanks!
> 
> https://codereview.chromium.org/252583007/diff/40001/base/files/file_proxy.h
> File base/files/file_proxy.h (right):
> 
>
https://codereview.chromium.org/252583007/diff/40001/base/files/file_proxy.h#...
> base/files/file_proxy.h:92: void SetFile(File file);
> On 2014/05/14 16:11:58, bbudge wrote:
> > It seems logical to put this with the create functions. Just below?
> 
> The only methods in between Create and this one are IsValid(), which is a
> concept implicitly needed by this method, and created(), which is only
relevant
> for Create and not this one. It also sits just by TakeFile() and
> GetPlatformFile() which are closely related to SetFile (maybe more than
Create)
> so it seems better to keep it here.
> 
> > Also, the comment isn't clear about how file_ might be set already. How
about
> > this?
> > 
> > // Sets the file to one that is already open. |file_| should not have been
set
> > by calling CreateOrOpen or CreateTemporary.
> 
> The problem is that the requirement is not exactly that Create has not been
> called, but that IsValid() should return false. Create may fail, or even
succeed
> as long as TakeFile() or Close() is called after that.
> 
> I updated the comments.

Thanks, looks good.

[rvargas (doing something else), 2014-05-14 20:07:38]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-05-14 20:08:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/252583007/60001

[commit-bot: I haz the power, 2014-05-14 22:26:44]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 22:31:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 22:31:34]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[rvargas (doing something else), 2014-05-14 22:40:15]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-05-14 22:42:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/252583007/60001

[commit-bot: I haz the power, 2014-05-14 23:39:33]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-05-14 23:48:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 23:48:29]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[rvargas (doing something else), 2014-05-15 19:07:36]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-05-15 19:08:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/252583007/60001

[commit-bot: I haz the power, 2014-05-15 19:40:41]

Change committed as 270781