Pepper: Use an adaptor to invoke FileSystemOperationRunner::OpenFile

content/browser/renderer_host/pepper/pepper_file_io_host.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/pepper/pepper_file_io_host.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/files/file_util_proxy.h"
@@ skipping 60 matching lines @@
   if (!host)
     return false;
   return client->IsPluginAllowedToCallRequestOSFileHandle(
       host->GetBrowserContext(), document_url);
 }
 
 bool FileOpenForWrite(int32_t open_flags) {
   return (open_flags & (PP_FILEOPENFLAG_WRITE | PP_FILEOPENFLAG_APPEND)) != 0;
 }
 
+void FileCloser(base::File auto_close) {
+}
+
+class OpenFileAdapter {
+ public:
+  OpenFileAdapter(PepperFileIOHost* file_host,
+                  fileapi::FileSystemOperation::OpenFileCallback callback);
+
+  void DidOpenFile(base::File file,
+                   const base::Closure& on_close_callback);
+ private:
+  base::WeakPtr<PepperFileIOHost> file_host_;
+  fileapi::FileSystemOperation::OpenFileCallback callback_;
+  DISALLOW_COPY_AND_ASSIGN(OpenFileAdapter);
+};
+
+OpenFileAdapter::OpenFileAdapter(
+    PepperFileIOHost* file_host,
+    fileapi::FileSystemOperation::OpenFileCallback callback)
+    : file_host_(AsWeakPtr(file_host)),
+      callback_(callback) {
+}
+
+void OpenFileAdapter::DidOpenFile(base::File file,
+                                  const base::Closure& on_close_callback) {
+  if (file_host_) {
+    callback_.Run(file.Pass(), on_close_callback);
+  } else {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                            base::Bind(&FileCloser, Passed(&file)));

[bbudge, 2014/06/03 18:21:53]

Shouldn't we call 'on_close_callback' on this path too?

[rvargas (doing something else), 2014/06/03 23:04:54]

That is an interesting question.

FileSystemOperationRunner::OpenFile() calls
FileSystemOperationImpl::OpenFile() which relies on
AsyncFileUtilAdapter::CreateOrOpen() which uses
RunCreateOrOpenCallback() to return a NULL on_close_callback.

On the other hand, as I said somewhere else, FileSystemOperationRunner has no
documentation about threading at all, so it is not obvious where that callback
should be called if it were not NULL. The normal pattern would be to call it on
the same thread that issued OpenFile(), and that means _this_ thread, which
would result in the DCHECK firing again.

Maybe DCHECKing that the callback is null at this point is good enough?

[rvargas (doing something else), 2014/06/03 23:19:58]

On second thought, now there is code to invoke the callback.

+  }
+}
+
 }  // namespace
 
 PepperFileIOHost::PepperFileIOHost(BrowserPpapiHostImpl* host,
                                    PP_Instance instance,
                                    PP_Resource resource)
     : ResourceHost(host->GetPpapiHost(), instance, resource),
       browser_ppapi_host_(host),
       render_process_host_(NULL),
       file_(BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)),
       open_flags_(0),
@@ skipping 129 matching lines @@
   }
 
   if (!file_system_context_->GetFileSystemBackend(file_system_url_.type())) {
     reply_context.params.set_result(PP_ERROR_FAILED);
     SendOpenErrorReply(reply_context);
     return;
   }
 
   DCHECK(file_system_host_.get());
   DCHECK(file_system_host_->GetFileSystemOperationRunner());
+
+  OpenFileAdapter* open_adapter(new OpenFileAdapter(
+      this,
+      base::Bind(&PepperFileIOHost::DidOpenInternalFile,
+                 weak_factory_.GetWeakPtr(),
+                 reply_context)));
+
   file_system_host_->GetFileSystemOperationRunner()->OpenFile(
       file_system_url_,
       platform_file_flags,
-      base::Bind(&PepperFileIOHost::DidOpenInternalFile,
-                 weak_factory_.GetWeakPtr(),
-                 reply_context));
+      base::Bind(&OpenFileAdapter::DidOpenFile, base::Owned(open_adapter)));

[bbudge, 2014/06/03 18:21:53]

Rather than define a new class, could you could define another static function
that takes the host and callback as params and pass that to the
FileSystemOperationRunner? Like this:

void DidOpenFile(base::weak_ptr<PepperFileIOHost> host,
                 fileapi::FileSystemOperation::OpenFileCallback cb,
                 base::File file,
                 const base::Closure& on_close_callback) {
  if (host) {
    cb.Run(file.Pass(), on_close_callback);
  } else {
    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                            base::Bind(&FileCloser,
                                       Passed(&file)));
  }
}

[rvargas (doing something else), 2014/06/03 23:04:54]

Done.

 }
 
 void PepperFileIOHost::DidOpenInternalFile(
     ppapi::host::ReplyMessageContext reply_context,
     base::File file,
     const base::Closure& on_close_callback) {
   if (file.IsValid()) {
     on_close_callback_ = on_close_callback;
 
     if (FileOpenForWrite(open_flags_) && file_system_host_->ChecksQuota()) {
@@ skipping 217 matching lines @@
 
   ppapi::proxy::SerializedHandle file_handle;
   // A non-zero resource id signals NaClIPCAdapter to create a NaClQuotaDesc.
   PP_Resource quota_file_io = check_quota_ ? pp_resource() : 0;
   file_handle.set_file_handle(transit_file, open_flags, quota_file_io);
   reply_context->params.AppendHandle(file_handle);
   return true;
 }
 
 }  // namespace content