Issue 5183 - Provide async interface for FileSystem

src/FileSystemJsObject.cpp

Index: src/FileSystemJsObject.cpp
===================================================================
--- a/src/FileSystemJsObject.cpp
+++ b/src/FileSystemJsObject.cpp
@@ -18,321 +18,169 @@
 #include <AdblockPlus/FileSystem.h>
 #include <stdexcept>
 #include <sstream>
 #include <vector>
 
 #include <AdblockPlus/JsValue.h>
 #include "FileSystemJsObject.h"
 #include "JsContext.h"
-#include "Thread.h"
 #include "Utils.h"
 
 using namespace AdblockPlus;
 
 namespace
 {
-  class IoThread : public Thread
-  {
-  public:
-    IoThread(const JsEnginePtr& jsEngine, const JsValue& callback)
-      : Thread(true), jsEngine(jsEngine), fileSystem(jsEngine->GetFileSystem()),
-        callback(callback)
-    {
-    }
-
-  protected:
-    JsEnginePtr jsEngine;
-    FileSystemPtr fileSystem;
-    JsValue callback;
-  };
-
-  class ReadThread : public IoThread
-  {
-  public:
-    ReadThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-               const std::string& path)
-      : IoThread(jsEngine, callback), path(path)
-    {
-    }
-
-    void Run()
-    {
-      std::string content;
-      std::string error;
-      try
-      {
-        std::shared_ptr<std::istream> stream = fileSystem->Read(path);
-        content = Utils::Slurp(*stream);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error =  "Unknown error while reading from " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto result = jsEngine->NewObject();
-      result.SetProperty("content", content);
-      result.SetProperty("error", error);
-      JsValueList params;
-      params.push_back(result);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-  };
-
-  class WriteThread : public IoThread
-  {
-  public:
-    WriteThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-                const std::string& path, const std::string& content)
-      : IoThread(jsEngine, callback), path(path), content(content)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      try
-      {
-        std::stringstream stream;
-        stream << content;
-        fileSystem->Write(path, stream);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while writing to " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto errorValue = jsEngine->NewValue(error);
-      JsValueList params;
-      params.push_back(errorValue);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-    std::string content;
-  };
-
-  class MoveThread : public IoThread
-  {
-  public:
-    MoveThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-               const std::string& fromPath, const std::string& toPath)
-      : IoThread(jsEngine, callback), fromPath(fromPath), toPath(toPath)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      try
-      {
-        fileSystem->Move(fromPath, toPath);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while moving " + fromPath + " to " + toPath;
-      }
-
-      const JsContext context(*jsEngine);
-      auto errorValue = jsEngine->NewValue(error);
-      JsValueList params;
-      params.push_back(errorValue);
-      callback.Call(params);
-    }
-
-  private:
-    std::string fromPath;
-    std::string toPath;
-  };
-
-  class RemoveThread : public IoThread
-  {
-  public:
-    RemoveThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-                 const std::string& path)
-      : IoThread(jsEngine, callback), path(path)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      try
-      {
-        fileSystem->Remove(path);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while removing " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto errorValue = jsEngine->NewValue(error);
-      JsValueList params;
-      params.push_back(errorValue);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-  };
-
-
-  class StatThread : public IoThread
-  {
-  public:
-    StatThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-               const std::string& path)
-      : IoThread(jsEngine, callback), path(path)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      FileSystem::StatResult statResult;
-      try
-      {
-        statResult = fileSystem->Stat(path);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while calling stat on " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto result = jsEngine->NewObject();
-      result.SetProperty("exists", statResult.exists);
-      result.SetProperty("isFile", statResult.isFile);
-      result.SetProperty("isDirectory", statResult.isDirectory);
-      result.SetProperty("lastModified", statResult.lastModified);
-      result.SetProperty("error", error);
-
-      JsValueList params;
-      params.push_back(result);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-  };
-
   v8::Handle<v8::Value> ReadCallback(const v8::Arguments& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return v8::ThrowException(Utils::ToV8String(isolate,
         "_fileSystem.read requires 2 parameters"));
     if (!converted[1].IsFunction())
       return v8::ThrowException(Utils::ToV8String(isolate,
         "Second argument to _fileSystem.read must be a function"));
-    ReadThread* const readThread = new ReadThread(jsEngine, converted[1],
-        converted[0].AsString());
-    readThread->Start();
+
+
+    JsValue callback(converted[1]);
+    jsEngine->GetFileSystem()->Read(converted[0].AsString(),
+      [jsEngine, callback]

[sergei, 2017/06/16 15:05:55]

Here and below that's what we also want to avoid. Currently FileSystem is held
by JsEngine and storing of either std::shared_ptr<JsEngine> or JsValue in the
closure here creates a circular reference. Instead std::weak_ptr<JsEngine> and
paramsID (auto paramsID = jsEngine->StoreJsValues(converted);) should be
captured.
Like here,
https://github.com/adblockplus/libadblockplus/blob/master/src/WebRequestJsObj...

[hub, 2017/06/16 21:52:54]

Done.

+      (std::string&& content, const std::string& error)
+      {
+        const JsContext context(*jsEngine);
+        auto result = jsEngine->NewObject();
+        result.SetProperty("content", content);

[sergei, 2017/06/16 15:05:55]

`content` should be moved, but getting into account std::vector, I guess, it
will be copied for present anyway.

[hub, 2017/06/16 21:52:54]

Acknowledged.

+        result.SetProperty("error", error);
+        JsValueList params;
+        params.push_back(result);
+        callback.Call(params);

[sergei, 2017/06/16 15:05:55]

We can simply call `callback.Call(result);` without `params`.

[hub, 2017/06/16 21:52:54]

Done.

+      });
+
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> WriteCallback(const v8::Arguments& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 3)
       return v8::ThrowException(Utils::ToV8String(isolate,
         "_fileSystem.write requires 3 parameters"));
     if (!converted[2].IsFunction())
       return v8::ThrowException(Utils::ToV8String(isolate,
         "Third argument to _fileSystem.write must be a function"));
-    WriteThread* const writeThread = new WriteThread(jsEngine, converted[2],
-        converted[0].AsString(), converted[1].AsString());
-    writeThread->Start();
+
+    JsValue callback(converted[2]);
+    auto stream = std::make_shared<std::stringstream>();
+    *stream << converted[1].AsString();
+    jsEngine->GetFileSystem()->Write(converted[0].AsString(), stream,
+      [jsEngine, callback](const std::string& error)
+      {
+        const JsContext context(*jsEngine);
+        auto errorValue = jsEngine->NewValue(error);
+        JsValueList params;
+        params.push_back(errorValue);

[sergei, 2017/06/16 15:05:55]

If the error is an empty string it will be still empty string in JS and `!""` is
`true`, so I think that JS will recognize it as an error with an empty
description. I think that in case of absence of an error we should not pass
error parameter to JS, so it will be `undefined`.

BTW, I find the conversion of an empty string to nothing in C++ a bit bad, but
maybe it's OK here, since it's pretty internal thing and is documented in the
C++ interface IFileSystem.

[hub, 2017/06/16 21:52:54]

We can rework this later I think. Currently the FileSystemJsObjectTest.Write
test assume error to be "" if none.

+        callback.Call(params);
+      });
+
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> MoveCallback(const v8::Arguments& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 3)
       return v8::ThrowException(Utils::ToV8String(isolate,
         "_fileSystem.move requires 3 parameters"));
     if (!converted[2].IsFunction())
       return v8::ThrowException(Utils::ToV8String(isolate,
         "Third argument to _fileSystem.move must be a function"));
-    MoveThread* const moveThread = new MoveThread(jsEngine, converted[2],
-        converted[0].AsString(), converted[1].AsString());
-    moveThread->Start();
+
+    JsValue callback(converted[2]);
+    jsEngine->GetFileSystem()->Move(converted[0].AsString(),
+      converted[1].AsString(),
+      [jsEngine, callback](const std::string& error)
+      {
+        const JsContext context(*jsEngine);
+        auto errorValue = jsEngine->NewValue(error);
+        JsValueList params;
+        params.push_back(errorValue);
+        callback.Call(params);
+      });
+
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> RemoveCallback(const v8::Arguments& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return v8::ThrowException(Utils::ToV8String(isolate,
         "_fileSystem.remove requires 2 parameters"));
     if (!converted[1].IsFunction())
       return v8::ThrowException(Utils::ToV8String(isolate,
         "Second argument to _fileSystem.remove must be a function"));
-    RemoveThread* const removeThread = new RemoveThread(jsEngine, converted[1],
-        converted[0].AsString());
-    removeThread->Start();
+
+    JsValue callback(converted[1]);
+    jsEngine->GetFileSystem()->Remove(converted[0].AsString(),
+      [jsEngine, callback](const std::string& error)
+      {
+        const JsContext context(*jsEngine);
+        auto errorValue = jsEngine->NewValue(error);
+        JsValueList params;
+        params.push_back(errorValue);
+        callback.Call(params);
+      });
+
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> StatCallback(const v8::Arguments& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return v8::ThrowException(Utils::ToV8String(isolate,
         "_fileSystem.stat requires 2 parameters"));
     if (!converted[1].IsFunction())
       return v8::ThrowException(Utils::ToV8String(isolate,
         "Second argument to _fileSystem.stat must be a function"));
-    StatThread* const statThread = new StatThread(jsEngine, converted[1],
-        converted[0].AsString());
-    statThread->Start();
+
+    JsValue callback(converted[1]);
+    jsEngine->GetFileSystem()->Stat(converted[0].AsString(),
+      [jsEngine, callback]
+      (const IFileSystem::StatResult& statResult, const std::string& error)
+      {
+        const JsContext context(*jsEngine);
+        auto result = jsEngine->NewObject();
+
+        result.SetProperty("exists", statResult.exists);
+        result.SetProperty("isFile", statResult.isFile);
+        result.SetProperty("isDirectory", statResult.isDirectory);
+        result.SetProperty("lastModified", statResult.lastModified);
+        result.SetProperty("error", error);
+
+        JsValueList params;
+        params.push_back(result);
+        callback.Call(params);
+      });
+
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> ResolveCallback(const v8::Arguments& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);