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

Source/modules/filesystem/LocalFileSystem.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 23 matching lines @@
 #include "core/dom/CrossThreadTask.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
 #include "core/fileapi/FileError.h"
 #include "core/inspector/InspectorController.h"
 #include "core/workers/WorkerGlobalScope.h"
 #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"
 
 namespace WebCore {
 
 namespace {
 
 void fileSystemNotAllowed(ExecutionContext*, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
 {
     callbacks->didFail(FileError::ABORT_ERR);
 }
 
 } // namespace
 
 PassOwnPtrWillBeRawPtr<LocalFileSystem> LocalFileSystem::create(PassOwnPtr<FileSystemClient> client)
 {
     return adoptPtrWillBeNoop(new LocalFileSystem(client));
 }
 
 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;
     }
+    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)
+{
+    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::deleteFileSystem(ExecutionContext* context, FileSystemType type, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
-{
-    ASSERT(context);
-    ASSERT_WITH_SECURITY_IMPLICATION(context->isDocument());
+void LocalFileSystem::resolveURLInternal(
+    const KURL& fileSystemURL,
+    PassOwnPtr<AsyncFileSystemCallbacks> callbacks) {
+    blink::Platform::current()->fileSystem()->resolveURL(fileSystemURL, callbacks);
+}
 
-    if (!client() || !client()->allowFileSystem(context)) {
-        context->postTask(createCallbackTask(&fileSystemNotAllowed, callbacks));
-        return;
-    }
+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)
 {
 }
 
 const char* LocalFileSystem::supplementName()
 {
     return "LocalFileSystem";
 }
 
@@ skipping 11 matching lines @@
     page.provideSupplement(LocalFileSystem::supplementName(), LocalFileSystem::create(client));
     page.inspectorController().registerModuleAgent(InspectorFileSystemAgent::create(&page));
 }
 
 void provideLocalFileSystemToWorker(WorkerClients* clients, PassOwnPtr<FileSystemClient> client)
 {
     clients->provideSupplement(LocalFileSystem::supplementName(), LocalFileSystem::create(client));
 }
 
 } // namespace WebCore