[PPAPI] It is now possible to pass filesystems from JavaScript to NaCl modules.

ppapi/proxy/raw_var_data.cc

Index: ppapi/proxy/raw_var_data.cc
diff --git a/ppapi/proxy/raw_var_data.cc b/ppapi/proxy/raw_var_data.cc
index 91bcfbfd1f0b0448126f2da92cc6081076cd1f14..fcbda78cef9e253015e0760d26ed4ff2b025d19f 100644
--- a/ppapi/proxy/raw_var_data.cc
+++ b/ppapi/proxy/raw_var_data.cc
@@ -9,7 +9,12 @@
 #include "base/containers/hash_tables.h"
 #include "base/stl_util.h"
 #include "ipc/ipc_message.h"
+#include "ppapi/proxy/file_system_resource.h"
+#include "ppapi/proxy/plugin_dispatcher.h"
+#include "ppapi/proxy/plugin_globals.h"
+#include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/ppapi_param_traits.h"
+#include "ppapi/proxy/resource_creation_proxy.h"
 #include "ppapi/shared_impl/array_var.h"
 #include "ppapi/shared_impl/dictionary_var.h"
 #include "ppapi/shared_impl/ppapi_globals.h"
@@ -64,6 +69,11 @@ bool CanHaveChildren(PP_Var var) {
   return var.type == PP_VARTYPE_ARRAY || var.type == PP_VARTYPE_DICTIONARY;
 }
 
+Connection GetConnectionForInstance(PP_Instance instance) {
+  PluginDispatcher* dispatcher = PluginDispatcher::GetForInstance(instance);
+  return Connection(PluginGlobals::Get()->GetBrowserSender(), dispatcher);

[dmichael (off chromium), 2013/10/22 16:04:28]

I don't think this would do the right thing if it was called in-process. Maybe
it would be good to document that or add a DCHECK?

[Matt Giuca, 2013/10/23 08:05:54]

Done. (We reasoned that GetForInstance will always return NULL in the in-process
case.)

+}
+
 }  // namespace
 
 // RawVarDataGraph ------------------------------------------------------------
@@ -695,12 +705,39 @@ bool ResourceRawVarData::Init(const PP_Var& var, PP_Instance /*instance*/) {
 }
 
 PP_Var ResourceRawVarData::CreatePPVar(PP_Instance instance) {
-  // If pp_resource_ is NULL, it could be because we are on the plugin side and
-  // there is a pending resource host on the renderer.
-  // TODO(mgiuca): Create a plugin-side resource in this case.
-  // Currently, this should never occur. This will be needed when passing a
-  // resource from the renderer to the plugin (http://crbug.com/177017).
-  DCHECK(pp_resource_);
+  // If we are on the plugin side, and the var is a pending resource host,
+  // create a plugin-side resource. A var is a pending resource host if it has a
+  // pp_resource of 0, and a non-null creation message.
+  if (PpapiGlobals::Get()->IsPluginGlobals() && !pp_resource_ &&
+      creation_message_) {
+    DCHECK(pending_renderer_host_id_);
+    DCHECK(pending_browser_host_id_);
+    switch (creation_message_->type()) {
+      case PpapiPluginMsg_FileSystem_CreateFromPendingHost::ID: {

[dmichael (off chromium), 2013/10/22 16:04:28]

switch seems like an odd construct here. How about just an if/else?

[Matt Giuca, 2013/10/23 08:05:54]

Why? This is just the first type of resource we are going to allow passing -- in
the future there may be many others. This code is taking an enum value, and
performing a different action depending on which specific value it takes, with a
default case. It is perfect for a switch.

I realise that similar code in pepper_renderer_connection.cc also uses an
if/else -- I think that should be a switch also. I'm happy to change that over
to a switch for consistency. But if you really want these to be ifs, I can
change this one.

[dmichael (off chromium), 2013/10/23 17:33:21]

Oh, right, switch/case is fine. I wasn't thinking about future resource types.

+        PP_FileSystemType file_system_type;
+        if (!UnpackMessage<PpapiPluginMsg_FileSystem_CreateFromPendingHost>(
+                 *creation_message_, &file_system_type)) {

[dmichael (off chromium), 2013/10/22 16:04:28]

nit: indent is one space too far

[Matt Giuca, 2013/10/23 08:05:54]

ClangFormat did it. It's indenting four spaces from the start of the function
name (not from the start of the expression), which I believe is correct.

+          NOTREACHED() << "Invalid message of type "
+                          "PpapiPluginMsg_FileSystem_CreateFromPendingHost";
+          return PP_MakeNull();
+        }
+        // Create a plugin-side resource and attach it to the host resource.
+        pp_resource_ =
+            (new FileSystemResource(GetConnectionForInstance(instance),
+                                    instance,
+                                    pending_renderer_host_id_,
+                                    pending_browser_host_id_,
+                                    file_system_type))->GetReference();
+        DCHECK(pp_resource_);

[raymes, 2013/10/22 05:59:21]

I think this DCHECK might be a bit uneeded. This case shouldn't really happen.

[Matt Giuca, 2013/10/23 08:05:54]

Well, the point of a DCHECK is to check for things that shouldn't really happen
:). But in this case it is quite unlikely, so I removed it.

+        break;
+      }
+      default: {
+        NOTREACHED() << "Creation message has unexpected type "
+                     << creation_message_->type();
+        return PP_MakeNull();
+      }
+    }
+  }
 
   return PpapiGlobals::Get()->GetVarTracker()->MakeResourcePPVar(pp_resource_);
 }