Remove passing WebString across threads

content/child/fileapi/webfilesystem_impl.cc

Index: content/child/fileapi/webfilesystem_impl.cc
diff --git a/content/child/fileapi/webfilesystem_impl.cc b/content/child/fileapi/webfilesystem_impl.cc
index 62e9f0eda62f8f94b220f912c0a08585cebf20a7..0ecae8154e5a7e3381ba3df4f16d9012daa55b8d 100644
--- a/content/child/fileapi/webfilesystem_impl.cc
+++ b/content/child/fileapi/webfilesystem_impl.cc
@@ -123,7 +123,68 @@ enum CallbacksUnregisterMode {
   DO_NOT_UNREGISTER_CALLBACKS,
 };
 
-// Run WebFileSystemCallbacks's |method| with |params|.
+// Bridging functions that convert the arguments into Blink objects
+// (e.g. WebFileInfo, WebString, WebVector<WebFileSystemEntry>)
+// and call WebFileSystemCallbacks's methods.
+// These are called by RunCallbacks after crossing threads to ensure
+// thread safety, because the Blink objects cannot be passed across
+// threads by base::Bind().
+class WebFileSystemCallbacksBridge {

[tzik, 2014/07/15 10:41:26]

I think it's not worth adding new class to combine these method.
Can we add functions separately for each method of the class?
Such as:
void didOpenFilesystem(const base::string16& name, const GURL& root,
WebFileSystemCallbacks* callbacks);

[hiroshige, 2014/07/15 12:30:31]

Done.

+public:
+  WebFileSystemCallbacksBridge(WebFileSystemCallbacks *callbacks)
+      : callbacks_(callbacks) {}
+
+  void didSucceed() {
+    callbacks_->didSucceed();
+  }
+
+  void didReadMetadata(const base::File::Info& file_info) {
+    WebFileInfo web_file_info;
+    FileInfoToWebFileInfo(file_info, &web_file_info);
+    callbacks_->didReadMetadata(web_file_info);
+  }
+
+  void didReadDirectory(
+      const std::vector<fileapi::DirectoryEntry>& entries,
+      bool has_more) {
+    WebVector<WebFileSystemEntry> file_system_entries(entries.size());
+    for (size_t i = 0; i < entries.size(); i++) {
+      file_system_entries[i].name =
+          base::FilePath(entries[i].name).AsUTF16Unsafe();
+      file_system_entries[i].isDirectory = entries[i].is_directory;
+    }
+    callbacks_->didReadDirectory(file_system_entries, has_more);
+  }
+
+  void didOpenFileSystem(const base::string16& name, const GURL& root) {
+    callbacks_->didOpenFileSystem(
+        static_cast<WebString>(name),
+        static_cast<WebURL>(root));
+  }
+
+  void didResolveURL(
+      const base::string16& name,
+      const GURL& root_url,
+      fileapi::FileSystemType mount_type,
+      const base::string16& file_path,
+      bool is_directory) {
+    callbacks_->didResolveURL(
+        static_cast<WebString>(name),
+        static_cast<WebURL>(root_url),
+        static_cast<blink::WebFileSystemType>(mount_type),
+        static_cast<WebString>(file_path),
+        is_directory);
+  }
+
+  void didFail(base::File::Error error) {
+    callbacks_->didFail(fileapi::FileErrorToWebFileError(error));
+  }
+
+private:
+  WebFileSystemCallbacks* callbacks_;
+};
+
+// Run WebFileSystemCallbacks's |method| with |params| via
 template <typename Method, typename Params>
 void RunCallbacks(int callbacks_id, Method method, const Params& params,

[tzik, 2014/07/15 10:41:26]

(continued from above)
And replace Method and Params with a
base::Callback<void(WebFileSystemCallbacks*)>?

[hiroshige, 2014/07/15 12:30:31]

Done.

                   CallbacksUnregisterMode callbacks_unregister_mode) {
@@ -134,7 +195,8 @@ void RunCallbacks(int callbacks_id, Method method, const Params& params,
   WebFileSystemCallbacks callbacks = filesystem->GetCallbacks(callbacks_id);
   if (callbacks_unregister_mode == UNREGISTER_CALLBACKS)
     filesystem->UnregisterCallbacks(callbacks_id);
-  DispatchToMethod(&callbacks, method, params);
+  WebFileSystemCallbacksBridge bridge(&callbacks);
+  DispatchToMethod(&bridge, method, params);
 }
 
 void DispatchResultsClosure(int thread_id, int callbacks_id,
@@ -180,7 +242,7 @@ void OpenFileSystemCallbackAdapter(
     const std::string& name, const GURL& root) {
   CallbackFileSystemCallbacks(

[tzik, 2014/07/15 10:41:26]

(continued from above)
And then, could you replace CallbackFilesystemCallbacks with
DispatchResultClosure + base::Bind?

[hiroshige, 2014/07/15 12:30:31]

I retained CallbackFilesystemCallbacks to make the code diff smaller.

       thread_id, callbacks_id, waitable_results,
-      &WebFileSystemCallbacks::didOpenFileSystem,
+      &WebFileSystemCallbacksBridge::didOpenFileSystem,
       MakeTuple(base::UTF8ToUTF16(name), root),
       UNREGISTER_CALLBACKS);
 }
@@ -194,9 +256,9 @@ void ResolveURLCallbackAdapter(
       fileapi::VirtualPath::GetNormalizedFilePath(file_path));
   CallbackFileSystemCallbacks(
       thread_id, callbacks_id, waitable_results,
-      &WebFileSystemCallbacks::didResolveURL,
+      &WebFileSystemCallbacksBridge::didResolveURL,
       MakeTuple(base::UTF8ToUTF16(info.name), info.root_url,
-                static_cast<blink::WebFileSystemType>(info.mount_type),
+                info.mount_type,
                 normalized_path.AsUTF16Unsafe(), is_directory),
       UNREGISTER_CALLBACKS);
 }
@@ -207,13 +269,13 @@ void StatusCallbackAdapter(int thread_id, int callbacks_id,
   if (error == base::File::FILE_OK) {
     CallbackFileSystemCallbacks(
         thread_id, callbacks_id, waitable_results,
-        &WebFileSystemCallbacks::didSucceed, MakeTuple(),
+        &WebFileSystemCallbacksBridge::didSucceed, MakeTuple(),
         UNREGISTER_CALLBACKS);
   } else {
     CallbackFileSystemCallbacks(
         thread_id, callbacks_id, waitable_results,
-        &WebFileSystemCallbacks::didFail,
-        MakeTuple(fileapi::FileErrorToWebFileError(error)),
+        &WebFileSystemCallbacksBridge::didFail,
+        MakeTuple(error),
         UNREGISTER_CALLBACKS);
   }
 }
@@ -221,12 +283,10 @@ void StatusCallbackAdapter(int thread_id, int callbacks_id,
 void ReadMetadataCallbackAdapter(int thread_id, int callbacks_id,
                                  WaitableCallbackResults* waitable_results,
                                  const base::File::Info& file_info) {
-  WebFileInfo web_file_info;
-  FileInfoToWebFileInfo(file_info, &web_file_info);
   CallbackFileSystemCallbacks(
       thread_id, callbacks_id, waitable_results,
-      &WebFileSystemCallbacks::didReadMetadata,
-      MakeTuple(web_file_info),
+      &WebFileSystemCallbacksBridge::didReadMetadata,
+      MakeTuple(file_info),
       UNREGISTER_CALLBACKS);
 }
 
@@ -234,16 +294,10 @@ void ReadDirectoryCallbackAdapter(
     int thread_id, int callbacks_id, WaitableCallbackResults* waitable_results,
     const std::vector<fileapi::DirectoryEntry>& entries,
     bool has_more) {
-  WebVector<WebFileSystemEntry> file_system_entries(entries.size());
-  for (size_t i = 0; i < entries.size(); i++) {
-    file_system_entries[i].name =
-        base::FilePath(entries[i].name).AsUTF16Unsafe();
-    file_system_entries[i].isDirectory = entries[i].is_directory;
-  }
   CallbackFileSystemCallbacks(
       thread_id, callbacks_id, waitable_results,
-      &WebFileSystemCallbacks::didReadDirectory,
-      MakeTuple(file_system_entries, has_more),
+      &WebFileSystemCallbacksBridge::didReadDirectory,
+      MakeTuple(entries, has_more),
       has_more ? DO_NOT_UNREGISTER_CALLBACKS : UNREGISTER_CALLBACKS);
 }