Refactor FileSystem

webkit/plugins/ppapi/ppb_file_ref_impl.h

Index: webkit/plugins/ppapi/ppb_file_ref_impl.h
diff --git a/webkit/plugins/ppapi/ppb_file_ref_impl.h b/webkit/plugins/ppapi/ppb_file_ref_impl.h
index 1b99969703904ad2e27c8999b6eecfbfd4a38921..986fd2bbe66e139b5e4f36e0f027dc24c1c7420e 100644
--- a/webkit/plugins/ppapi/ppb_file_ref_impl.h
+++ b/webkit/plugins/ppapi/ppb_file_ref_impl.h
@@ -26,19 +26,21 @@ class WEBKIT_GLUE_EXPORT PPB_FileRef_Impl
     : public ::ppapi::PPB_FileRef_Shared {
  public:
   PPB_FileRef_Impl(const ::ppapi::PPB_FileRef_CreateInfo& info,
-                   PPB_FileSystem_Impl* file_system);
+                   PP_Resource file_system);
   PPB_FileRef_Impl(const ::ppapi::PPB_FileRef_CreateInfo& info,
                    const base::FilePath& external_file_path);
   virtual ~PPB_FileRef_Impl();
 
   // The returned object will have a refcount of 0 (just like "new").
-  static PPB_FileRef_Impl* CreateInternal(PP_Resource pp_file_system,
+  static PPB_FileRef_Impl* CreateInternal(PP_Instance instance,
+                                          PP_Resource pp_file_system,
                                           const std::string& path);
 
   // The returned object will have a refcount of 0 (just like "new").
-  static PPB_FileRef_Impl* CreateExternal(PP_Instance instance,
-                                          const base::FilePath& external_file_path,
-                                          const std::string& display_name);
+  static PPB_FileRef_Impl* CreateExternal(
+      PP_Instance instance,
+      const base::FilePath& external_file_path,
+      const std::string& display_name);
 
   // PPB_FileRef_API implementation (not provided by PPB_FileRef_Shared).
   virtual PP_Resource GetParent() OVERRIDE;
@@ -59,7 +61,7 @@ class WEBKIT_GLUE_EXPORT PPB_FileRef_Impl
       scoped_refptr< ::ppapi::TrackedCallback> callback) OVERRIDE;
   virtual PP_Var GetAbsolutePath();
 
-  PPB_FileSystem_Impl* file_system() const { return file_system_.get(); }
+  PP_Resource file_system_resource() const { return file_system_; }
 
   // Returns the system path corresponding to this file. Valid only for
   // external filesystems.
@@ -70,14 +72,15 @@ class WEBKIT_GLUE_EXPORT PPB_FileRef_Impl
 
   // Checks if file ref has file system instance and if the instance is opened.
   bool HasValidFileSystem() const;
+
  private:
   // Many mutation functions are allow only to non-external filesystems, This
   // function returns true if the filesystem is opened and isn't external as an
   // access check for these functions.
   bool IsValidNonExternalFileSystem() const;
 
-  // Null for external filesystems.
-  scoped_refptr<PPB_FileSystem_Impl> file_system_;
+  // 0 for external filesystems.
+  PP_Resource file_system_;

[yzshen1, 2013/04/08 21:05:16]

Could you please comment that this is a 'plugin-side' PP_Resource that we don't
hold a reference to?

One problem here is that because PPB_FileRef_Impl is not able to hold a
reference to the file system PP_Resource. The file system resource may die
earlier than this object if the plugin doesn't explicitly hold references to it.
And then subsequent operations will fail.

If FileRef has switched to the new design, we could let the FileRefResource
obejct (at the plugin process) hold a ref to the file system PP_Resource. 
However, because PPB_FileRef is still using the old design, it is complicated to
manipulate the ref count of the file system PP_Resource: we need to decide
whether FileRef_Impl is created for out-of-process or in-process plugins: for
the former case, the plugin-side proxy should hold a ref; for the latter case,
the impl itself should hold a ref.

The approach to hold a linked_ptr<ResourceHost> was also considered, but it has
a problem: once the PP_Resource goes away, it is removed from the mapping in
PpapiHost and then PpapiHost::GetResourceHost(PP_Resource resource) won't return
the object, so the code in plugin_delegate_impl won't work.

[victorhsieh, 2013/04/08 23:44:38]

Comment added.  It should be safe now as plugin delegate checks the resource
existence before using it, and looks like we don't need to workaround this now? 
Let me know if I'm wrong.

[yzshen1, 2013/04/09 17:25:42]

Let's discuss offline. It is too long to type. :)
On 2013/04/08 23:44:38, Victor Hsieh wrote:

[victorhsieh, 2013/04/10 19:07:10]

Done.  Please verify the implementation is correct or not.

 
   // Used only for external filesystems.
   base::FilePath external_file_system_path_;