Remove passing WebString across threads

content/child/fileapi/webfilesystem_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/child/fileapi/webfilesystem_impl.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ skipping 105 matching lines @@
   DCHECK(!waitable_results);
   DispatchToMethod(ChildThread::current()->file_system_dispatcher(),
                    method, params);
 }
 
 enum CallbacksUnregisterMode {
   UNREGISTER_CALLBACKS,
   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) {
   WebFileSystemImpl* filesystem =
       WebFileSystemImpl::ThreadSpecificInstance(NULL);
   if (!filesystem)
     return;
   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,
                             WaitableCallbackResults* waitable_results,
                             const base::Closure& results_closure) {
   if (thread_id != CurrentWorkerId()) {
     if (waitable_results) {
       // If someone is waiting, this should result in running the closure.
       waitable_results->AddResultsAndSignal(results_closure);
       // In case no one is waiting, post a task to run the closure.
@@ skipping 23 matching lines @@
 
 //-----------------------------------------------------------------------------
 // Callback adapters. Callbacks must be called on the original calling thread,
 // so these callback adapters relay back the results to the calling thread
 // if necessary.
 
 void OpenFileSystemCallbackAdapter(
     int thread_id, int callbacks_id,
     WaitableCallbackResults* waitable_results,
     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);
 }
 
 void ResolveURLCallbackAdapter(
     int thread_id, int callbacks_id,
     WaitableCallbackResults* waitable_results,
     const fileapi::FileSystemInfo& info,
     const base::FilePath& file_path, bool is_directory) {
   base::FilePath normalized_path(
       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);
 }
 
 void StatusCallbackAdapter(int thread_id, int callbacks_id,
                            WaitableCallbackResults* waitable_results,
                            base::File::Error error) {
   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);
   }
 }
 
 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);
 }
 
 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);
 }
 
 void DidCreateFileWriter(
     int callbacks_id,
     const GURL& path,
     blink::WebFileWriterClient* client,
     base::MessageLoopProxy* main_thread_loop,
     const base::File::Info& file_info) {
   WebFileSystemImpl* filesystem =
@@ skipping 389 matching lines @@
 WaitableCallbackResults* WebFileSystemImpl::MaybeCreateWaitableResults(
     const WebFileSystemCallbacks& callbacks, int callbacks_id) {
   if (!callbacks.shouldBlockUntilCompletion())
     return NULL;
   WaitableCallbackResults* results = new WaitableCallbackResults();
   waitable_results_[callbacks_id] = results;
   return results;
 }
 
 }  // namespace content