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

content/renderer/pepper/resource_converter.cc

Index: content/renderer/pepper/resource_converter.cc
diff --git a/content/renderer/pepper/resource_converter.cc b/content/renderer/pepper/resource_converter.cc
index 118a1ac4dfe81cd18bee3c643f7e073aa1322aea..979868112bf05d6a8f17648c4db3e5a872abc9ae 100644
--- a/content/renderer/pepper/resource_converter.cc
+++ b/content/renderer/pepper/resource_converter.cc
@@ -7,9 +7,15 @@
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "content/public/renderer/renderer_ppapi_host.h"
+#include "content/renderer/pepper/pepper_file_system_host.h"
 #include "ipc/ipc_message.h"
+#include "ppapi/host/ppapi_host.h"
+#include "ppapi/host/resource_host.h"
+#include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/shared_impl/resource_var.h"
 #include "ppapi/shared_impl/scoped_pp_var.h"
+#include "third_party/WebKit/public/platform/WebFileSystem.h"
+#include "third_party/WebKit/public/web/WebDOMFileSystem.h"
 
 namespace {
 
@@ -24,6 +30,56 @@ void FlushComplete(
   callback.Run(true);
 }
 
+// Converts a WebKit::WebFileSystem::Type to a PP_FileSystemType.
+// Returns true on success, false on failure.

[raymes, 2013/10/11 04:02:24]

Seems like this comment doesn't match the behavior.

[Matt Giuca, 2013/10/15 00:25:16]

Done.

+PP_FileSystemType WebFileSystemTypeToPPAPI(
+    WebKit::WebFileSystem::Type type) {

[raymes, 2013/10/11 04:02:24]

Does this fit on the previous line?

[Matt Giuca, 2013/10/15 00:25:16]

Done.

+  switch (type) {
+    case WebKit::WebFileSystem::TypeTemporary:
+      return PP_FILESYSTEMTYPE_LOCALTEMPORARY;
+    case WebKit::WebFileSystem::TypePersistent:
+      return PP_FILESYSTEMTYPE_LOCALPERSISTENT;
+    case WebKit::WebFileSystem::TypeIsolated:
+      return PP_FILESYSTEMTYPE_ISOLATED;
+    case WebKit::WebFileSystem::TypeExternal:
+      return PP_FILESYSTEMTYPE_EXTERNAL;
+  }

[raymes, 2013/10/11 04:02:24]

Do we need a default case for safety here? 

[Matt Giuca, 2013/10/15 00:25:16]

Done.

+}
+
+// Given a V8 value containing a DOMFileSystem, creates a resource host and
+// returns the resource information for serialization.
+// On error, false.
+bool DOMFileSystemToResource(
+    PP_Instance instance,
+    content::RendererPpapiHost* host,
+    const WebKit::WebDOMFileSystem& dom_file_system,
+    int* pending_renderer_id,
+    IPC::Message* create_message,
+    IPC::Message* browser_host_create_message) {
+  PP_FileSystemType file_system_type;

[dmichael (off chromium), 2013/10/11 19:21:36]

would be better to initialize this immediately. You can probably just move the
declaration down to where you call WebFileSystemTypeToPPAPI

[Matt Giuca, 2013/10/15 00:25:16]

Done.

+  // TODO(mgiuca): Ensure that external file systems are supported.
+  DCHECK(!dom_file_system.isNull());
+
+  file_system_type = WebFileSystemTypeToPPAPI(dom_file_system.type());
+  GURL root_url = dom_file_system.rootURL();
+
+  content::PepperFileSystemHost* file_system_host =

[raymes, 2013/10/11 04:02:24]

nit: maybe initialize a scoped_ptr here to make ownership clear

[Matt Giuca, 2013/10/15 00:25:16]

That doesn't work; scoped_ptr can't be copied so it has to be initialized in the
argument list.

Actually, I'll just inline the call to "new PepperFileSystemHost". It is a bit
messier but it avoids having a raw pointer sitting around.

[yzshen1, 2013/10/15 18:38:03]

nit: You could call Pass() on the scoped_ptr if you want to create it outside of
the argument list.

On 2013/10/15 00:25:16, Matt Giuca wrote:

[Matt Giuca, 2013/10/17 09:17:58]

Fair enough, but I think it looks fine the way I currently have it.

+      new content::PepperFileSystemHost(host, instance, 0, root_url,
+                                        file_system_type);
+  *pending_renderer_id = host->GetPpapiHost()->AddPendingResourceHost(
+      scoped_ptr<::ppapi::host::ResourceHost>(file_system_host));
+  if (!*pending_renderer_id)
+    return false;
+
+  *create_message = PpapiPluginMsg_FileSystem_CreateFromPendingHost(
+      file_system_type);

[dmichael (off chromium), 2013/10/11 19:21:36]

Maybe it would make sense to use scoped_ptr<IPC::Message>* (i.e., a scoped_ptr
out-param.)

Every time you copy a Message, it has to allocate the buffer and copy the bytes.
That can be somewhat expensive, especially for big messages.

Using a scoped_ptr out-param, you can allocate it once and (almost) never have
to copy. You could extend that all the way down to HostResourceVar having a
scoped_ptr<IPC::Message>.

This might be a bit much to throw in to this CL, so maybe just consider it as a
future optimization.

[Matt Giuca, 2013/10/17 09:17:58]

These are already being passed out of this function by reference. They will be
created in-place in the pointer provided from CreateV8Value.

CreateV8Value passes them by reference down to CreateResourceVarWithBrowserHost,
which has to do a full copy of each of them to populate the HostResourceVar and
vector, respectively. Avoiding that copy would be possible but introduce some
annoying complexity (it isn't really conventional to have constructors accept
scoped_ptrs).

Basically each of these will be copied one time each, which I think is OK. The
current message is very small (a few bytes). Eventually if we start embedding
entire objects like blobs in messages, it would be appropriate to revisit
avoiding any copy.

[dmichael (off chromium), 2013/10/17 17:36:11]

Because of the interface you have here, you also copy them right here. You could
alleviate that by having scoped_ptrs for out-params...
in the param list: scoped_ptr<IPC::Message>* create_message
then, here:
*create_message = new PpapiPluginMsg_...

There's also nothing wrong or unconventional about having a constructor accept a
scoped_ptr. I would read it as "this takes ownership of the given object", which
would be accurate in this case.

That said, it's probably a really minor performance difference. So might not be
worth the trouble. Definitely okay to leave it out for this CL.

[Matt Giuca, 2013/10/18 07:09:30]

Oh I see where the extra copy is here. I've fixed the one you mention using a
scoped_ptr. The other copy is a bit more work and I'm not convinced it's worth
the change.

+
+  *browser_host_create_message =

[dmichael (off chromium), 2013/10/11 19:21:36]

(same suggestion for this message)

[Matt Giuca, 2013/10/18 07:09:30]

Done.

+      PpapiHostMsg_FileSystem_CreateFromRenderer(root_url.spec(),
+                                                 file_system_type);
+  return true;
+}
+
 }  // namespace
 
 namespace content {
@@ -50,8 +106,24 @@ bool ResourceConverterImpl::FromV8Value(v8::Handle<v8::Object> val,
   v8::HandleScope handle_scope(context->GetIsolate());
 
   *was_resource = false;
-  // TODO(mgiuca): There are currently no values which can be converted to
-  // resources.
+
+  WebKit::WebDOMFileSystem dom_file_system =
+      WebKit::WebDOMFileSystem::fromV8Value(val);
+  if (!dom_file_system.isNull()) {

[yzshen1, 2013/10/11 17:45:45]

if this if-block doesn't run, |result| is not touched and we return true, is
that intended?

[dmichael (off chromium), 2013/10/11 19:21:36]

+1. I think we should leave |result| untouched and return false.

[Matt Giuca, 2013/10/15 00:25:16]

Yes, this was intentional (but perhaps not the best design). It's trickier than
that. See the comment here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

There are basically three outcomes:
- Success.
- Not a resource.
- Unexpected error.

The "not a resource" case cannot be considered an error. We have no way to tell
whether a V8 value is going to convert to a resource until we call this method.
If it's a resource, v8_var_converter will proceed to treat the V8 value as an
ordinary dictionary. If it's an error, v8_var_converter will error out. This
logic is already committed, and it's a pain to change it (if you want to change
it, it's outside the scope of this CL).

I agree, it's not ideal to have the function return true but leave some inputs
unchanged. A quick fix would be to simply set |result| to PP_MakeNull() in the
"not a resource" case. Is that acceptable?

[yzshen1, 2013/10/15 18:38:03]

More comment sounds like a good idea. :)
On 2013/10/15 00:25:16, Matt Giuca wrote:

[Matt Giuca, 2013/10/17 09:17:58]

Done.

+    int pending_renderer_id;
+    IPC::Message create_message;
+    IPC::Message browser_host_create_message;
+    if (!DOMFileSystemToResource(instance_, host_, dom_file_system,
+                                 &pending_renderer_id, &create_message,
+                                 &browser_host_create_message))
+      return false;

[raymes, 2013/10/11 04:02:24]

nit: usually use { with multi-line if's

[Matt Giuca, 2013/10/15 00:25:16]

Done.

+    scoped_refptr<HostResourceVar> result_var =
+        CreateResourceVarWithBrowserHost(
+            pending_renderer_id, create_message, browser_host_create_message);
+    *result = result_var->GetPPVar();

[raymes, 2013/10/11 04:02:24]

optional: is it clearer to combine the previous two statements?

[Matt Giuca, 2013/10/15 00:25:16]

I don't think so, because then you'll never see that it was a scoped_refptr.

[raymes, 2013/10/16 00:45:14]

It shouldn't be important to know that it's a scoped_refptr. But either way is
fine.

[Matt Giuca, 2013/10/17 09:17:58]

OK. (Also it would be a very long line, so I'll leave it.)

+    *was_resource = true;
+    return true;
+  }
 
   return true;
 }