Notify CloseFile from Pepper to FileSystem.

Notify CloseFile from Pepper to FileSystem.

Remote filesystems need to know when a plugin finished
updating a local cache file in order to commit the change.
This CL is to wire the notification of file close between
the two components.

Since writable OpenFile is not yet implemented for remote filesystem,
the CL itself should not cause behavior change.

BUG=132236
TEST=browser_tests --gtest_filter='*FileIO*'

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

[kinuko, 2012-06-13 06:38:29]

http://codereview.chromium.org/10541113/diff/8001/content/browser/fileapi/fil...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/8001/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:402: Send(new
FileSystemMsg_DidFail(request_id, error));
What could happen if one opens a writable file, writing something but then loses
the write permission before closing?

http://codereview.chromium.org/10541113/diff/8001/content/common/fileapi/file...
File content/common/fileapi/file_system_dispatcher.cc (right):

http://codereview.chromium.org/10541113/diff/8001/content/common/fileapi/file...
content/common/fileapi/file_system_dispatcher.cc:15: class CloseFileDispatcher :
public fileapi::FileSystemCallbackDispatcher {
We usually put these dispatchers on the caller's site (as
AsyncOpenFileSystemURLCallbackTranslator does in pepper_plugin_delegate_impl.cc)
since it's not what the common FSDispatcher wants to care in general.

Or, I guess we can make this IPC send-only and don't let the browser reply?

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:742:
base::FileUtilProxy::StatusCallback());
Do we need to call NotifyCloseFile on this case?

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:900:
base::FileUtilProxy::StatusCallback());
Ditto. Close is often called in async helper destructors like this. Do we need
to call NotifyCloseFile on these cases?

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:924: AsWeakPtr(), path));
What happens if the plugin goes away before this is handled?

http://codereview.chromium.org/10541113/diff/8001/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):

http://codereview.chromium.org/10541113/diff/8001/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:209:
plugin_delegate->AsyncNotifyCloseFileSystemURL(file_system_url_);
It looks like we can call this in Close's callback task and get rid of another
PostTaskAndReply in pepper_plugin_delegate_impl?

[kinaba, 2012-06-13 07:15:30]

Reply for the most critical comment.

On 2012/06/13 06:38:29, kinuko wrote:
http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
> content/renderer/pepper/pepper_plugin_delegate_impl.cc:924: AsWeakPtr(),
path));
> What happens if the plugin goes away before this is handled?

Very good catch.
The lifetime of PepperPluginDelegateImpl is nearly equal to RenderView so as
long as RenderView is alive it is handled,
but this means, in other words, if the renderer died before closing files they
won't be notified at all.
Let me take some time to think how to handle it. (Or do you have any good
ideas?)

[kinaba, 2012-06-25 11:14:02]

Revised the patch.

Kinuko, can you take a look once again? Main difference is the addition of
mechanism to call NotifyCloseFile() when the renderer is dead.

http://codereview.chromium.org/10541113/diff/8001/content/browser/fileapi/fil...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/8001/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:402: Send(new
FileSystemMsg_DidFail(request_id, error));
On 2012/06/13 06:38:29, kinuko wrote:
> What could happen if one opens a writable file, writing something but then
loses
> the write permission before closing?

Fm, right. Removed the permission check.

http://codereview.chromium.org/10541113/diff/8001/content/common/fileapi/file...
File content/common/fileapi/file_system_dispatcher.cc (right):

http://codereview.chromium.org/10541113/diff/8001/content/common/fileapi/file...
content/common/fileapi/file_system_dispatcher.cc:15: class CloseFileDispatcher :
public fileapi::FileSystemCallbackDispatcher {
On 2012/06/13 06:38:29, kinuko wrote:
> We usually put these dispatchers on the caller's site (as
> AsyncOpenFileSystemURLCallbackTranslator does in
pepper_plugin_delegate_impl.cc)
> since it's not what the common FSDispatcher wants to care in general.
> 
> Or, I guess we can make this IPC send-only and don't let the browser reply?

Sounds good. I made it into a send-only message.

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:742:
base::FileUtilProxy::StatusCallback());
On 2012/06/13 06:38:29, kinuko wrote:
> Do we need to call NotifyCloseFile on this case?

This method is for opening local FilePath's, not filesystem GURL's. So we don't
need to care this case.

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:900:
base::FileUtilProxy::StatusCallback());
On 2012/06/13 06:38:29, kinuko wrote:
> Ditto. Close is often called in async helper destructors like this. Do we need
> to call NotifyCloseFile on these cases?

Yes, thanks.

http://codereview.chromium.org/10541113/diff/8001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:924: AsWeakPtr(), path));
On 2012/06/13 06:38:29, kinuko wrote:
> What happens if the plugin goes away before this is handled?

PepperPluginDelegateImpl is associated with a renderer, so as long as renderrer
is alive, DoNotifyCloseFile() is called (no matter where plugin goes). For the
case when renderer is dead, I added a detection code to FileAPIMessageFilter.

http://codereview.chromium.org/10541113/diff/8001/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):

http://codereview.chromium.org/10541113/diff/8001/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_io_impl.cc:209:
plugin_delegate->AsyncNotifyCloseFileSystemURL(file_system_url_);
On 2012/06/13 06:38:29, kinuko wrote:
> It looks like we can call this in Close's callback task and get rid of another
> PostTaskAndReply in pepper_plugin_delegate_impl?

Done.
The tricky thing here is that in the callback we have no direct access to
plugin_delegate anymore. The new patch set encapsulate it in a callback object.

[kinuko, 2012-06-25 11:46:54]

Looking nicer now.  Could we somehow test this code?

http://codereview.chromium.org/10541113/diff/18002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/18002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:413: if
(open_filesystem_urls_.erase(path.spec()) == 0)
How could this happen? Do we want to assert here?

http://codereview.chromium.org/10541113/diff/18002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:417: // operation that does
not
nit: that does not ...?

http://codereview.chromium.org/10541113/diff/18002/webkit/chromeos/fileapi/re...
File webkit/chromeos/fileapi/remote_file_system_operation.cc (right):

http://codereview.chromium.org/10541113/diff/18002/webkit/chromeos/fileapi/re...
webkit/chromeos/fileapi/remote_file_system_operation.cc:175:
remote_proxy_->NotifyCloseFile(path);
Who should delete this operation in this case?

http://codereview.chromium.org/10541113/diff/18002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/10541113/diff/18002/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation.cc:404: // need synchronization with remote
server.
Should we delete this here?

[zel, 2012-06-25 16:23:03]

crap, I started pretty much the same change as a part of wiring open file
(truncated) support.

[kinaba, 2012-06-25 23:11:14]

On 2012/06/25 16:23:03, zel wrote:
> crap, I started pretty much the same change as a part of wiring open file
> (truncated) support.

Oops, do you think we should do it in a single patch? Then I'll hand this to
you.
Otherwise I'll land this asap. (Anyway let's discuss later).

[kinaba, 2012-06-26 00:15:10]

Anyhow here's the update reflecting review comments from Kinuko.

> Could we somehow test this code?
Not directly, but we can at least verify that existing browser_tests on Pepper
FileIO does not trigger the DCHECK in NotifyCloseFile(), and does not emit DLOG
in OnChannelClosing (added in the latest patch set) except *FileIO_AbortCalls
test cases. It seems it is expected for AbortCalls tests, since the test is shut
down before fully waiting all the in-flight operations.

http://codereview.chromium.org/10541113/diff/18002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/18002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:413: if
(open_filesystem_urls_.erase(path.spec()) == 0)
On 2012/06/25 11:46:54, kinuko wrote:
> How could this happen? Do we want to assert here?

It should never happen as long as it is correctly implemented. Turned it to
DCHECK.

http://codereview.chromium.org/10541113/diff/18002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:417: // operation that does
not
On 2012/06/25 11:46:54, kinuko wrote:
> nit: that does not ...?

:)

http://codereview.chromium.org/10541113/diff/18002/webkit/chromeos/fileapi/re...
File webkit/chromeos/fileapi/remote_file_system_operation.cc (right):

http://codereview.chromium.org/10541113/diff/18002/webkit/chromeos/fileapi/re...
webkit/chromeos/fileapi/remote_file_system_operation.cc:175:
remote_proxy_->NotifyCloseFile(path);
On 2012/06/25 11:46:54, kinuko wrote:
> Who should delete this operation in this case?

The operation itself. Added delete this.

http://codereview.chromium.org/10541113/diff/18002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/10541113/diff/18002/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation.cc:404: // need synchronization with remote
server.
On 2012/06/25 11:46:54, kinuko wrote:
> Should we delete this here?

True. Done.

[zel, 2012-06-26 00:23:01]

I had no good way to test truncate unless file close notification was
implemented, that's how I ended up duplicating your efforts in the first
place.

I have also developed NaCl plugin that exercises this code path. So, I will
know that my stuff really works :).  My stuff is wired slightly differently
- for example, I have nothing like tracking that you have in your CL
with FileAPIMessageFilter::open_filesystem_urls_ but I can surely
cut-and-paste you logic there. Do you know when exactly gets
OnChannelClosing() invoked?



On Mon, Jun 25, 2012 at 4:11 PM, <kinaba@chromium.org> wrote:

> On 2012/06/25 16:23:03, zel wrote:
>
>> crap, I started pretty much the same change as a part of wiring open file
>> (truncated) support.
>>
>
> Oops, do you think we should do it in a single patch? Then I'll hand this
> to
> you.
> Otherwise I'll land this asap. (Anyway let's discuss later).
>
>
http://codereview.chromium.**org/10541113/<http://codereview.chromium.org/105...
>

[zel, 2012-06-26 01:42:18]

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_file_io_impl.cc:214: if (!file_system_url_.is_empty())
{
file_system_url_ is going to be set only for local (temp/permanent) file
systems. - see OpenValidated() above

[kinaba, 2012-06-26 04:19:22]

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_file_io_impl.cc:214: if (!file_system_url_.is_empty())
{
On 2012/06/26 01:42:18, zel wrote:
> file_system_url_ is going to be set only for local (temp/permanent) file
> systems. - see OpenValidated() above

Fmm, is it?
I thought the code pass (not HasValidFileSystem() and proceed to AsyncOpenFile)
is only for files given as a single isolated file (like the ones from file
chooser dialog). If the file is opened via normal Pepper API pp::FileSystem and
pp::FileRef, I thought it should come with a valid file system.
Could you send me your test NaCl plugin? I'd like to test on it.

[kinuko, 2012-06-26 11:13:36]

http://codereview.chromium.org/10541113/diff/25002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10541113/diff/25002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:397: 
nit: extra empty line

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:421: // Do not use
GetNewOperation() here, because NotifyCloseFile is an one-way
nit: an one way -> a one-way

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.h (right):

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.h:9: #include <set>
nit: not alphabetically sorted

http://codereview.chromium.org/10541113/diff/25002/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/25002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:904: private:  //
TODO(ericu): Delete this?
nit: can you delete this comment (and fix indent) while you're there?

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_file_io_impl.cc:214: if (!file_system_url_.is_empty())
{
On 2012/06/26 04:19:22, kinaba wrote:
> On 2012/06/26 01:42:18, zel wrote:
> > file_system_url_ is going to be set only for local (temp/permanent) file
> > systems. - see OpenValidated() above
> 
> Fmm, is it?
> I thought the code pass (not HasValidFileSystem() and proceed to
AsyncOpenFile)
> is only for files given as a single isolated file (like the ones from file
> chooser dialog). If the file is opened via normal Pepper API pp::FileSystem
and
> pp::FileRef, I thought it should come with a valid file system.
> Could you send me your test NaCl plugin? I'd like to test on it.

EXTERNAL file could be created without file system instance, but the code looks
it's also possible to open an EXTERNAL filesystem and open a file in the
filesystem, in that case we have file_system_url_ set in the file's FileIO.
(I haven't tested the code path by myself so I'm not 100% sure though)

[zel, 2012-06-26 23:51:57]

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_file_io_impl.cc:214: if (!file_system_url_.is_empty())
{
On 2012/06/26 11:13:36, kinuko wrote:
> On 2012/06/26 04:19:22, kinaba wrote:
> > On 2012/06/26 01:42:18, zel wrote:
> > > file_system_url_ is going to be set only for local (temp/permanent) file
> > > systems. - see OpenValidated() above
> > 
> > Fmm, is it?
> > I thought the code pass (not HasValidFileSystem() and proceed to
> AsyncOpenFile)
> > is only for files given as a single isolated file (like the ones from file
> > chooser dialog). If the file is opened via normal Pepper API pp::FileSystem
> and
> > pp::FileRef, I thought it should come with a valid file system.
> > Could you send me your test NaCl plugin? I'd like to test on it.
> 
> EXTERNAL file could be created without file system instance, but the code
looks
> it's also possible to open an EXTERNAL filesystem and open a file in the
> filesystem, in that case we have file_system_url_ set in the file's FileIO.
> (I haven't tested the code path by myself so I'm not 100% sure though)

Yeah, that seems correct. NaCl has two inconsistent paths for dealing with
external files. The one that works with file picker won't really work for this
notification.

[zel, 2012-06-27 00:04:08]

I have merged and verified your changes with my CL (for file open+truncate) at
https://chromiumcodereview.appspot.com/10600013.

LGTM

[zel, 2012-06-27 00:08:03]

http://codereview.chromium.org/10541113/diff/25002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10541113/diff/25002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:395: // corresponding
NotifyCloseFile for committing dirty cache.
please see how I implemented this method at:
https://chromiumcodereview.appspot.com/10600013/diff/9020/chrome/browser/chro...

[kinaba, 2012-06-27 03:47:42]

Updated. Who will be the good person to ask for Pepper-side review?

http://codereview.chromium.org/10541113/diff/25002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10541113/diff/25002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:395: // corresponding
NotifyCloseFile for committing dirty cache.
On 2012/06/27 00:08:03, zel wrote:
> please see how I implemented this method at:
>
https://chromiumcodereview.appspot.com/10600013/diff/9020/chrome/browser/chro...

Thanks, it is the exact implementation I imagined!

http://codereview.chromium.org/10541113/diff/25002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:397: 
On 2012/06/26 11:13:36, kinuko wrote:
> nit: extra empty line

Done.

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:421: // Do not use
GetNewOperation() here, because NotifyCloseFile is an one-way
On 2012/06/26 11:13:36, kinuko wrote:
> nit: an one way -> a one-way

Done.

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.h (right):

http://codereview.chromium.org/10541113/diff/25002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.h:9: #include <set>
On 2012/06/26 11:13:36, kinuko wrote:
> nit: not alphabetically sorted

Done.

http://codereview.chromium.org/10541113/diff/25002/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/25002/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:904: private:  //
TODO(ericu): Delete this?
On 2012/06/26 11:13:36, kinuko wrote:
> nit: can you delete this comment (and fix indent) while you're there?

Done.

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_file_io_impl.cc (right):

http://codereview.chromium.org/10541113/diff/25002/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_file_io_impl.cc:214: if (!file_system_url_.is_empty())
{
On 2012/06/26 23:51:57, zel wrote:
> On 2012/06/26 11:13:36, kinuko wrote:
> > On 2012/06/26 04:19:22, kinaba wrote:
> > > On 2012/06/26 01:42:18, zel wrote:
> > > > file_system_url_ is going to be set only for local (temp/permanent) file
> > > > systems. - see OpenValidated() above
> > > 
> > > Fmm, is it?
> > > I thought the code pass (not HasValidFileSystem() and proceed to
> > AsyncOpenFile)
> > > is only for files given as a single isolated file (like the ones from file
> > > chooser dialog). If the file is opened via normal Pepper API
pp::FileSystem
> > and
> > > pp::FileRef, I thought it should come with a valid file system.
> > > Could you send me your test NaCl plugin? I'd like to test on it.
> > 
> > EXTERNAL file could be created without file system instance, but the code
> looks
> > it's also possible to open an EXTERNAL filesystem and open a file in the
> > filesystem, in that case we have file_system_url_ set in the file's FileIO.
> > (I haven't tested the code path by myself so I'm not 100% sure though)
> 
> Yeah, that seems correct. NaCl has two inconsistent paths for dealing with
> external files. The one that works with file picker won't really work for this
> notification.

And I believe it is meant for read-only file open that doesn't require the
open/close notification.

[kinuko, 2012-06-27 04:02:58]

Can you rebase since I've just landed somewhat larger cleanup patch around
FileAPIMessageFilter?

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:134: if (operation)
I suppose at this point this should not be NULL (e.g. the file_system_url must
be valid if it could be opened)

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.h (right):

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.h:187: std::multiset<GURL>
open_filesystem_urls_;
We may call multiple NotifyCloseFile for the same URL, is it correct?

[kinaba, 2012-06-27 04:26:06]

Rebase & fix.

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:134: if (operation)
On 2012/06/27 04:02:58, kinuko wrote:
> I suppose at this point this should not be NULL (e.g. the file_system_url must
> be valid if it could be opened)

Done. (Removed the null-check).

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.h (right):

http://codereview.chromium.org/10541113/diff/31003/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.h:187: std::multiset<GURL>
open_filesystem_urls_;
On 2012/06/27 04:02:58, kinuko wrote:
> We may call multiple NotifyCloseFile for the same URL, is it correct?

Correct, as long as the underlying filesystem allows multiple OpenFile (like the
current non-Remote filesystems).

[kinuko, 2012-06-27 04:29:08]

yzshen@ or dmichael@ could you take a look at the pepper-side change?

[kinuko, 2012-06-27 04:29:30]

lgtm for fileapi changes

[kinaba, 2012-06-27 04:55:48]

Fixing build failures caused by my blind rebasing...

> Can you rebase since I've just landed somewhat larger cleanup patch around
> FileAPIMessageFilter?

The clean up is ok for this patch (targeting M22), but wouldn't there be a
danger to refactor chrome/browser/chromeos/gdata/* now? I thought several GData
patches still need to be merged to M21.

[kinuko, 2012-06-27 05:44:33]

On 2012/06/27 04:55:48, kinaba wrote:
> Fixing build failures caused by my blind rebasing...
> 
> > Can you rebase since I've just landed somewhat larger cleanup patch around
> > FileAPIMessageFilter?
> 
> The clean up is ok for this patch (targeting M22), but wouldn't there be a
> danger to refactor chrome/browser/chromeos/gdata/* now? I thought several
GData
> patches still need to be merged to M21.

Might be, though it's always hard to determine when is the best timing to land
such patch.  Hoping this doesn't cause much scream (changes in gdata/* are only
in the proxy/glue code), or may it get reverted.

[yzshen1, 2012-06-28 20:14:17]

https://chromiumcodereview.appspot.com/10541113/diff/47003/webkit/plugins/ppa...
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/10541113/diff/47003/webkit/plugins/ppa...
webkit/plugins/ppapi/plugin_delegate.h:428: virtual base::Callback<void
(base::PlatformFileError)>
It seems more intuitive to have such an interface:
- Don't have a separate GetCloseFileSystemURLCallback method;
AsyncOpenFileSystemURL will take a new type of callback,
AsyncOpenFile*SystemURL*Callback.

- AsyncOpenFileSystemURLCallback has one more input parameter, which is a close
callback. This callback has remembered the corresponding plugin delegate
instance, as well as the file system URL.
If the open succeeds, the user is responsible for closing the file handle and
then call the close callback. They can easily do this by passing both of them to
base::FileUtilProxy::Close.

(Maybe it is just more intuitive to me? :))

[kinaba, 2012-06-29 09:28:33]

Thanks for the comment!

http://codereview.chromium.org/10541113/diff/47003/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10541113/diff/47003/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:428: virtual base::Callback<void
(base::PlatformFileError)>
On 2012/06/28 20:14:17, yzshen1 wrote:
> It seems more intuitive to have such an interface:
> - Don't have a separate GetCloseFileSystemURLCallback method;
> AsyncOpenFileSystemURL will take a new type of callback,
> AsyncOpenFile*SystemURL*Callback.
> 
> - AsyncOpenFileSystemURLCallback has one more input parameter, which is a
close
> callback. This callback has remembered the corresponding plugin delegate
> instance, as well as the file system URL.
> If the open succeeds, the user is responsible for closing the file handle and
> then call the close callback. They can easily do this by passing both of them
to
> base::FileUtilProxy::Close.
> 
> (Maybe it is just more intuitive to me? :))

No reason to object. Done.

[dmichael (off chromium), 2012-06-29 16:06:40]

a couple of nits. O/w, webkit/plugins/ppapi lgtm

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:927: path)));
nit: indentation is off

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/mock_...
File webkit/plugins/ppapi/mock_plugin_delegate.h (right):

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/mock_...
webkit/plugins/ppapi/mock_plugin_delegate.h:73:
GetCloseFileSystemURLCallback(const GURL& path);
Can you delete this now?

[yzshen1, 2012-06-29 17:15:52]

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:890:
close_file_callback_);
[optional] Is it better to pass a no-op as the third parameter in this case?

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:930: void
PepperPluginDelegateImpl::DoNotifyCloseFile(
- please keep the definitions in the same order as the declarations in the .h
file.
- this could be a static method, right? (Then you don't need to use AsWeakPtr()
on line 926.) Or is this intended?

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:426: typedef base::Callback<
Please add a comment about how the callback(s) work, and what is the users'
responsibility when they finish using the file.

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_file_io_impl.h (right):

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_file_io_impl.h:77: void
ExecuteOpenFileSystemURLCallback(
All these handlers have 'Platform' in it. (But maybe it makes the name too
long?)

[kinaba, 2012-07-03 06:46:34]

Done.

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:890:
close_file_callback_);
On 2012/06/29 17:15:52, yzshen1 wrote:
> [optional] Is it better to pass a no-op as the third parameter in this case?

Done.

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:927: path)));
On 2012/06/29 16:06:40, dmichael wrote:
> nit: indentation is off

Done.

http://codereview.chromium.org/10541113/diff/51024/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:930: void
PepperPluginDelegateImpl::DoNotifyCloseFile(
On 2012/06/29 17:15:52, yzshen1 wrote:
> - please keep the definitions in the same order as the declarations in the .h
> file.
> - this could be a static method, right? (Then you don't need to use
AsWeakPtr()
> on line 926.) Or is this intended?

Good point, I haven't realized.
Moved the function (and the Dispatcher class) to the anonymous namespace on top
of this file.

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/mock_...
File webkit/plugins/ppapi/mock_plugin_delegate.h (right):

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/mock_...
webkit/plugins/ppapi/mock_plugin_delegate.h:73:
GetCloseFileSystemURLCallback(const GURL& path);
On 2012/06/29 16:06:40, dmichael wrote:
> Can you delete this now?

Done.

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:426: typedef base::Callback<
On 2012/06/29 17:15:52, yzshen1 wrote:
> Please add a comment about how the callback(s) work, and what is the users'
> responsibility when they finish using the file.

Done.

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/ppb_f...
File webkit/plugins/ppapi/ppb_file_io_impl.h (right):

http://codereview.chromium.org/10541113/diff/51024/webkit/plugins/ppapi/ppb_f...
webkit/plugins/ppapi/ppb_file_io_impl.h:77: void
ExecuteOpenFileSystemURLCallback(
On 2012/06/29 17:15:52, yzshen1 wrote:
> All these handlers have 'Platform' in it. (But maybe it makes the name too
> long?)

Done.

[yzshen1, 2012-07-03 17:26:31]

ppapi part LGTM (with 2 nits)

Thanks!

http://codereview.chromium.org/10541113/diff/50007/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/50007/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:240: base::PlatformFile
file) {
nit: No need to put it on a separate line.

http://codereview.chromium.org/10541113/diff/50007/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10541113/diff/50007/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:427: // finished using the file, it must
close the file handle and then must call
nit: it -> they.

[kinaba, 2012-07-04 04:26:22]

Done.

http://codereview.chromium.org/10541113/diff/50007/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10541113/diff/50007/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:240: base::PlatformFile
file) {
On 2012/07/03 17:26:31, yzshen1 wrote:
> nit: No need to put it on a separate line.

Done.

http://codereview.chromium.org/10541113/diff/50007/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10541113/diff/50007/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:427: // finished using the file, it must
close the file handle and then must call
On 2012/07/03 17:26:31, yzshen1 wrote:
> nit: it -> they.

Done.

[kinaba, 2012-07-04 04:54:27]

+brettw

Brett, may I beg you an OWNER lgtm for content/ ?

[brettw, 2012-07-10 04:56:47]

content lgtm rubberstamp

[commit-bot: I haz the power, 2012-07-10 08:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/10541113/49014

[commit-bot: I haz the power, 2012-07-10 09:03:41]

Change committed as 145856