Issue 5183 - Provide async interface for FileSystem

src/FileSystemJsObject.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 23 matching lines @@
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return ThrowExceptionInJS(isolate, "_fileSystem.read requires 2 parameters");
     if (!converted[1].IsFunction())
       return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.read must be a function");
 
-    JsValue callback(converted[1]);
+    JsValueList values;
+    values.push_back(converted[1]);
+    auto weakCallback = jsEngine->StoreJsValues(values);
     std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
     jsEngine->GetAsyncFileSystem()->Read(converted[0].AsString(),
-      [weakJsEngine, callback]

[sergei, 2017/07/03 09:25:54]

`JsValue callback` should not be captured by value because it holds JsEngine,
instead it should be stored in JsEngine using `jsEngine->StoreJsValues` and
obscure (weak) returned ID should be used in the closure.

[hub, 2017/07/04 19:58:27]

I'll fix that here and there.

-      (std::string&& content, const std::string& error)
+      [weakJsEngine, weakCallback]
+      (IFileSystem::IOBuffer&& content, const std::string& error)
       {
         auto jsEngine = weakJsEngine.lock();
         if (!jsEngine)
           return;
 
         const JsContext context(*jsEngine);
         auto result = jsEngine->NewObject();
-        result.SetProperty("content", std::move(content));
-        result.SetProperty("error", error);
-        callback.Call(result);
+        result.SetStringBufferProperty("content", std::move(content));
+        if (!error.empty())
+          result.SetProperty("error", error);
+        jsEngine->TakeJsValues(weakCallback)[0].Call(result);
       });
   }
 
   void WriteCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 3)
       return ThrowExceptionInJS(isolate, "_fileSystem.write requires 3 parameters");
     if (!converted[2].IsFunction())
       return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.write must be a function");
 
-    JsValue callback(converted[2]);
-    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
+    JsValueList values;
+    values.push_back(converted[2]);
+    auto weakCallback = jsEngine->StoreJsValues(values);
+    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
+    auto content = converted[1].AsStringBuffer();
     jsEngine->GetAsyncFileSystem()->Write(converted[0].AsString(),
-      converted[1].AsString(),
-      [weakJsEngine, callback](const std::string& error)
-      {
-        auto jsEngine = weakJsEngine.lock();
-        if (!jsEngine)
-          return;
-
-        const JsContext context(*jsEngine);
-        JsValueList params;
-        params.push_back(jsEngine->NewValue(error));
-        callback.Call(params);
+      content,
+      [weakJsEngine, weakCallback](const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        JsValueList params;
+        if (!error.empty())
+          params.push_back(jsEngine->NewValue(error));
+        jsEngine->TakeJsValues(weakCallback)[0].Call(params);
       });
   }
 
   void MoveCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 3)
       return ThrowExceptionInJS(isolate, "_fileSystem.move requires 3 parameters");
     if (!converted[2].IsFunction())
       return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.move must be a function");
 
-    JsValue callback(converted[2]);
+    JsValueList values;
+    values.push_back(converted[2]);
+    auto weakCallback = jsEngine->StoreJsValues(values);
     std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
     jsEngine->GetAsyncFileSystem()->Move(converted[0].AsString(),
       converted[1].AsString(),
-      [weakJsEngine, callback](const std::string& error)
-      {
-        auto jsEngine = weakJsEngine.lock();
-        if (!jsEngine)
-          return;
-
-        const JsContext context(*jsEngine);
-        auto errorValue = jsEngine->NewValue(error);
-        JsValueList params;
-        params.push_back(errorValue);

[sergei, 2017/07/03 09:25:54]

But the problem is that the empty string will be considered as an error in JS
code and the author of JS code is likely expecting an convention similar to
nodejs. If `error` is an empty string than we simply should not add it to
`params`.

[hub, 2017/07/04 19:58:27]

This mean we are changing the semantics of the API as the test also assume the
empty string is "no error".

But I agree with error being undefined == no error, so I'll fix the tests too.

-        callback.Call(params);
+      [weakJsEngine, weakCallback](const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        JsValueList params;
+        if (!error.empty())
+          params.push_back(jsEngine->NewValue(error));
+        jsEngine->TakeJsValues(weakCallback)[0].Call(params);
       });
   }
 
   void RemoveCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return ThrowExceptionInJS(isolate, "_fileSystem.remove requires 2 parameters");
     if (!converted[1].IsFunction())
       return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.remove must be a function");
 
-    JsValue callback(converted[1]);
+    JsValueList values;
+    values.push_back(converted[1]);
+    auto weakCallback = jsEngine->StoreJsValues(values);
     std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
     jsEngine->GetAsyncFileSystem()->Remove(converted[0].AsString(),
-      [weakJsEngine, callback](const std::string& error)
-      {
-        auto jsEngine = weakJsEngine.lock();
-        if (!jsEngine)
-          return;
-
-        const JsContext context(*jsEngine);
-        auto errorValue = jsEngine->NewValue(error);
-        JsValueList params;
-        params.push_back(errorValue);
-        callback.Call(params);
+      [weakJsEngine, weakCallback](const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        JsValueList params;
+        if (!error.empty())
+          params.push_back(jsEngine->NewValue(error));
+        jsEngine->TakeJsValues(weakCallback)[0].Call(params);
       });
   }
 
   void StatCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return ThrowExceptionInJS(isolate, "_fileSystem.stat requires 2 parameters");
     if (!converted[1].IsFunction())
       return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.stat must be a function");
 
-    JsValue callback(converted[1]);
+    JsValueList values;
+    values.push_back(converted[1]);
+    auto weakCallback = jsEngine->StoreJsValues(values);
     std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
     jsEngine->GetAsyncFileSystem()->Stat(converted[0].AsString(),
-      [weakJsEngine, callback]
+      [weakJsEngine, weakCallback]
       (const IFileSystem::StatResult& statResult, const std::string& error)
       {
         auto jsEngine = weakJsEngine.lock();
         if (!jsEngine)
           return;
 
         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);
+        if (!error.empty())
+          result.SetProperty("error", error);
 
         JsValueList params;
         params.push_back(result);
-        callback.Call(params);
+        jsEngine->TakeJsValues(weakCallback)[0].Call(params);
       });
   }
 
   void ResolveCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 1)
       return ThrowExceptionInJS(isolate, "_fileSystem.resolve requires 1 parameter");
 
     std::string resolved = jsEngine->GetAsyncFileSystem()->Resolve(converted[0].AsString());
     arguments.GetReturnValue().Set(Utils::ToV8String(isolate, resolved));
   }
 }
 
 
 JsValue& FileSystemJsObject::Setup(JsEngine& jsEngine, JsValue& obj)
 {
   obj.SetProperty("read", jsEngine.NewCallback(::ReadCallback));
   obj.SetProperty("write", jsEngine.NewCallback(::WriteCallback));
   obj.SetProperty("move", jsEngine.NewCallback(::MoveCallback));
   obj.SetProperty("remove", jsEngine.NewCallback(::RemoveCallback));
   obj.SetProperty("stat", jsEngine.NewCallback(::StatCallback));
   obj.SetProperty("resolve", jsEngine.NewCallback(::ResolveCallback));
   return obj;
 }