Pepper: Use an adaptor to invoke FileSystemOperationRunner::OpenFile

content/browser/renderer_host/pepper/pepper_file_io_host.cc

Index: content/browser/renderer_host/pepper/pepper_file_io_host.cc
diff --git a/content/browser/renderer_host/pepper/pepper_file_io_host.cc b/content/browser/renderer_host/pepper/pepper_file_io_host.cc
index d9a11b7cc440a3eb2dc011f595e70b53de988957..3b4f864e324e0f9c9404de19e0019236d9512091 100644
--- a/content/browser/renderer_host/pepper/pepper_file_io_host.cc
+++ b/content/browser/renderer_host/pepper/pepper_file_io_host.cc
@@ -78,6 +78,39 @@ 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,
@@ -227,12 +260,17 @@ void PepperFileIOHost::GotUIThreadStuffForInternalFileSystems(
 
   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(