Use asynchronized api for file system request. [blink] (3/4)

Source/modules/filesystem/LocalFileSystem.cpp

Index: Source/modules/filesystem/LocalFileSystem.cpp
diff --git a/Source/modules/filesystem/LocalFileSystem.cpp b/Source/modules/filesystem/LocalFileSystem.cpp
index 98d046d5beecfa556938bcf35821c9b5f255a86a..8ef69c5f5d8e6595abacaaf90d085a7b5320b796 100644
--- a/Source/modules/filesystem/LocalFileSystem.cpp
+++ b/Source/modules/filesystem/LocalFileSystem.cpp
@@ -41,6 +41,7 @@
 #include "modules/filesystem/FileSystemClient.h"
 #include "modules/filesystem/InspectorFileSystemAgent.h"
 #include "platform/AsyncFileSystemCallbacks.h"
+#include "platform/PermissionCallbacks.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebFileSystem.h"
 
@@ -66,36 +67,89 @@ LocalFileSystem::~LocalFileSystem()
 
 void LocalFileSystem::resolveURL(ExecutionContext* context, const KURL& fileSystemURL, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
 {
-    if (!client() || !client()->allowFileSystem(context)) {
-        context->postTask(createCallbackTask(&fileSystemNotAllowed, callbacks));
+    RefPtr<ExecutionContext> contextPtr(context);
+    if (!client()) {
+        fileSystemNotAllowedInternal(contextPtr, callbacks);
         return;
     }
-    blink::Platform::current()->fileSystem()->resolveURL(fileSystemURL, callbacks);
+    ASSERT(contextPtr);
+    if (!contextPtr->isDocument()) {
+        if (!client()->requestFileSystemAccessSync(contextPtr.get())) {
+            fileSystemNotAllowedInternal(contextPtr, callbacks);
+            return;
+        }
+        resolveURLInternal(fileSystemURL, callbacks);
+        return;
+    }
+    requestFileSystemAccesssAsyncInternal(contextPtr, bind(&LocalFileSystem::resolveURLInternal, this, fileSystemURL, callbacks), bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr, callbacks));

[kinuko, 2014/05/21 07:24:24]

Um, looking this closely actually this wouldn't work really well (as well as
your previous patch wouldn't either).

callbacks's given as PassOwnPtr, so one of the bind() will nullify it before the
other bind()'s evaluated.

I think you'll need to have a wrapper RefCounted class for
AsyncFileSystemCallbacks, and pass the wrapper to these internal methods instead
of the AsyncFileSystemCallbacks itself.

Also I think you can include the line 76-83 in the common method, so that the
body of each public method (resolveURL, requestFileSystem, deleteFileSystem)
would just contain single method call (+ creating the wrapper callback object
etc)

[Xi Han, 2014/05/21 19:50:05]

1. Great catch! I introduced a wrapper RefCounted structure for
AsyncFileSystemCallbacks.

2. I tried to puts lines 76-83 into a common method, but the code looks quite
complicated. This is because each public method (resolveURL, requestFileSystem)
calls different internal function (see line 81, 100). If we put them together,
we need to pass in a bind for these internal functions as well. This makes the
code become too complicated to read. It is ideal to have clean code, but it is
not right to make simple functions become so complicated. How about just leave
the code as it is now? 

[kinuko, 2014/05/22 10:37:42]

Can't the common method do just like:

{
    if (!client()) {
        denied();
        return;
    }
    if (!context->isDocument()) {
        if (!client()->requestFileSystemAccessSync(context)) {
            denied();
            return;
        }
        allowed();
        return;
    }
    client()->requestFileSystemAccessAsync(context,
PermissionCallbacks::create(allowed, denied));
}

?

[Xi Han, 2014/05/22 14:21:41]

Thank you for the correction and the rough patch. The code looks really neat and
clean now. Sorry about my comment, I realized that I accidentally introduced one
more layer of call which caused the code ugly. 

 }
 
 void LocalFileSystem::requestFileSystem(ExecutionContext* context, FileSystemType type, long long size, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
 {
-    if (!client() || !client()->allowFileSystem(context)) {
-        context->postTask(createCallbackTask(&fileSystemNotAllowed, callbacks));
+    RefPtr<ExecutionContext> contextPtr(context);
+    if (!client()) {
+        fileSystemNotAllowedInternal(contextPtr, callbacks);
         return;
     }
-    KURL storagePartition = KURL(KURL(), context->securityOrigin()->toString());
-    blink::Platform::current()->fileSystem()->openFileSystem(storagePartition, static_cast<blink::WebFileSystemType>(type), callbacks);
+    ASSERT(contextPtr);
+    if (!contextPtr->isDocument()) {
+        if (!client()->requestFileSystemAccessSync(contextPtr.get())) {
+            fileSystemNotAllowedInternal(contextPtr, callbacks);
+            return;
+        }
+        fileSystemAllowedInternal(contextPtr, type, callbacks);
+        return;
+    }
+    requestFileSystemAccesssAsyncInternal(contextPtr, bind(&LocalFileSystem::fileSystemAllowedInternal, this, contextPtr, type, callbacks), bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr, callbacks));
 }
 
 void LocalFileSystem::deleteFileSystem(ExecutionContext* context, FileSystemType type, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
 {
-    ASSERT(context);
-    ASSERT_WITH_SECURITY_IMPLICATION(context->isDocument());
-
-    if (!client() || !client()->allowFileSystem(context)) {
-        context->postTask(createCallbackTask(&fileSystemNotAllowed, callbacks));
+    RefPtr<ExecutionContext> contextPtr(context);
+    ASSERT(contextPtr);
+    ASSERT_WITH_SECURITY_IMPLICATION(contextPtr->isDocument());
+    if (!client()) {
+        fileSystemNotAllowedInternal(contextPtr, callbacks);
         return;
     }
+    requestFileSystemAccesssAsyncInternal(contextPtr, bind(&LocalFileSystem::deleteFileSystemInternal, this, contextPtr, type, callbacks), bind(&LocalFileSystem::fileSystemNotAllowedInternal, this, contextPtr, callbacks));
+}
+
+void LocalFileSystem::fileSystemNotAllowedInternal(
+    PassRefPtr<ExecutionContext> context,
+    PassOwnPtr<AsyncFileSystemCallbacks> callbacks) {
+    context->postTask(createCallbackTask(&fileSystemNotAllowed, callbacks));
+}
+
+void LocalFileSystem::fileSystemAllowedInternal(
+    PassRefPtr<ExecutionContext> context,
+    FileSystemType type,
+    PassOwnPtr<AsyncFileSystemCallbacks> callbacks) {
+    KURL storagePartition = KURL(KURL(), context->securityOrigin()->toString());
+    blink::Platform::current()->fileSystem()->openFileSystem(storagePartition, static_cast<blink::WebFileSystemType>(type), callbacks);
+}
+
+void LocalFileSystem::resolveURLInternal(
+    const KURL& fileSystemURL,
+    PassOwnPtr<AsyncFileSystemCallbacks> callbacks) {
+    blink::Platform::current()->fileSystem()->resolveURL(fileSystemURL, callbacks);
+}
+
+void LocalFileSystem::deleteFileSystemInternal(
+    PassRefPtr<ExecutionContext> context,
+    FileSystemType type,
+    PassOwnPtr<AsyncFileSystemCallbacks> callbacks) {
     KURL storagePartition = KURL(KURL(), context->securityOrigin()->toString());
     blink::Platform::current()->fileSystem()->deleteFileSystem(storagePartition, static_cast<blink::WebFileSystemType>(type), callbacks);
 }
 
+void LocalFileSystem::requestFileSystemAccesssAsyncInternal(
+    PassRefPtr<ExecutionContext> context,
+    const Closure& allowedCallback,
+    const Closure& deniedCallback) {
+    client()->requestFileSystemAccessAsync(context.get(), PermissionCallbacks::create(allowedCallback, deniedCallback));
+}
+
 LocalFileSystem::LocalFileSystem(PassOwnPtr<FileSystemClient> client)
     : m_client(client)
 {