Implement File API

src/FileSystemJsObject.cpp

 #include <AdblockPlus/FileSystem.h>
 #include <stdexcept>
 #include <sstream>
 #include <vector>
 
 #include "FileSystemJsObject.h"
 #include "Utils.h"
 #include "Thread.h"
+#include "Utils.h"
 
 using namespace AdblockPlus;
 
 namespace
 {
   class IoThread : public Thread
   {
   public:
-    IoThread(FileSystem& fileSystem, v8::Persistent<v8::Function> callback)

[Wladimir Palant, 2013/04/12 16:10:35]

Passing in callback like this is rather ugly - you are disposing a handle that
you didn't create. This parameter should have type v8::Handle (typically meaning
v8::Local) that's turned into v8::Persistent in the initializer.

+    IoThread(FileSystem& fileSystem, v8::Handle<v8::Function> callback)
       : isolate(v8::Isolate::GetCurrent()),
         context(v8::Persistent<v8::Context>::New(isolate,
                                                  v8::Context::GetCurrent())),
-        fileSystem(fileSystem), callback(callback)
+        fileSystem(fileSystem),
+        callback(v8::Persistent<v8::Function>::New(isolate, callback))
     {
     }
 
     virtual ~IoThread()
     {
       callback.Dispose(isolate);
       context.Dispose(isolate);
     }
 
   protected:
     v8::Isolate* const isolate;
     v8::Persistent<v8::Context> context;
     FileSystem& fileSystem;
     v8::Persistent<v8::Function> callback;
   };
 
   class ReadThread : public IoThread
   {
   public:
-    ReadThread(FileSystem& fileSystem, v8::Persistent<v8::Function> callback,
+    ReadThread(FileSystem& fileSystem, v8::Handle<v8::Function> callback,
                const std::string& path)
       : IoThread(fileSystem, callback), path(path)
     {
     }
 
     void Run()
     {
       std::string content;
       std::string error;
       try
       {
-        std::auto_ptr<std::istream> stream = fileSystem.Read(path);
+        std::tr1::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 v8::Locker locker(isolate);
       const v8::HandleScope handleScope;
       const v8::Context::Scope contextScope(context);
       v8::Handle<v8::Object> result = v8::Object::New();
-      result->Set(v8::String::New("content"), v8::String::New(content.c_str()));
-      result->Set(v8::String::New("error"), v8::String::New(error.c_str()));

[Wladimir Palant, 2013/04/12 16:10:35]

Please always use the string length when converting between std::string and
v8::String, the strings could contain NUL chars. E.g. you could move
toV8String/fromV8String from WebRequestJsObject.cpp into Utils.cpp.

[Felix Dahlke, 2013/04/15 03:43:34]

Will do, I meant to add string conversion functions to Utils.

+      result->Set(v8::String::New("content"), Utils::ToV8String(content));
+      result->Set(v8::String::New("error"), Utils::ToV8String(error));
       callback->Call(callback, 1,
                      reinterpret_cast<v8::Handle<v8::Value>*>(&result));
       delete this;
     }
 
   private:
     std::string path;
   };
 
   class WriteThread : public IoThread
   {
   public:
-    WriteThread(FileSystem& fileSystem, v8::Persistent<v8::Function> callback,
+    WriteThread(FileSystem& fileSystem, v8::Handle<v8::Function> callback,
                 const std::string& path, const std::string& content)
       : IoThread(fileSystem, callback), path(path), content(content)
     {
     }
 
     void Run()
     {
       std::string error;
       try
       {
-        fileSystem.Write(path, content);
+        std::tr1::shared_ptr<std::ostream> stream(new std::stringstream);
+        *stream << content;
+        fileSystem.Write(path, stream);
       }
       catch (std::exception& e)
       {
         error = e.what();
       }
       catch (...)
       {
         error = "Unknown error while writing to " + path;
       }
 
       const v8::Locker locker(isolate);
       const v8::HandleScope handleScope;
       const v8::Context::Scope contextScope(context);
-      v8::Handle<v8::Value> errorValue = v8::String::New(error.c_str());
+      v8::Handle<v8::Value> errorValue = Utils::ToV8String(error);
       callback->Call(callback, 1, &errorValue);
       delete this;
     }
 
   private:
     std::string path;
     std::string content;
   };
 
   class MoveThread : public IoThread
   {
   public:
-    MoveThread(FileSystem& fileSystem, v8::Persistent<v8::Function> callback,
+    MoveThread(FileSystem& fileSystem, v8::Handle<v8::Function> callback,
                const std::string& fromPath, const std::string& toPath)
       : IoThread(fileSystem, 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 v8::Locker locker(isolate);
       const v8::HandleScope handleScope;
       const v8::Context::Scope contextScope(context);
-      v8::Handle<v8::Value> errorValue = v8::String::New(error.c_str());
+      v8::Handle<v8::Value> errorValue = Utils::ToV8String(error);
       callback->Call(callback, 1, &errorValue);
       delete this;
     }
 
   private:
     std::string fromPath;
     std::string toPath;
   };
 
   class RemoveThread : public IoThread
   {
   public:
-    RemoveThread(FileSystem& fileSystem, v8::Persistent<v8::Function> callback,
+    RemoveThread(FileSystem& fileSystem, v8::Handle<v8::Function> callback,
                  const std::string& path)
       : IoThread(fileSystem, 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 v8::Locker locker(isolate);
       const v8::HandleScope handleScope;
       const v8::Context::Scope contextScope(context);
-      v8::Handle<v8::Value> errorValue = v8::String::New(error.c_str());
+      v8::Handle<v8::Value> errorValue = Utils::ToV8String(error);
       callback->Call(callback, 1, &errorValue);
       delete this;
     }
 
   private:
     std::string path;
   };
 
   class StatThread : public IoThread
   {
   public:
-    StatThread(FileSystem& fileSystem, v8::Persistent<v8::Function> callback,
+    StatThread(FileSystem& fileSystem, v8::Handle<v8::Function> callback,
                const std::string& path)
       : IoThread(fileSystem, callback), path(path)
     {
     }
 
     void Run()
     {
       std::string error;
       FileSystem::StatResult statResult;
       try
@@ skipping 13 matching lines @@
       const v8::HandleScope handleScope;
       const v8::Context::Scope contextScope(context);
       v8::Handle<v8::Object> result = v8::Object::New();
       result->Set(v8::String::New("exists"),
                   v8::Boolean::New(statResult.exists));
       result->Set(v8::String::New("isFile"),
                   v8::Boolean::New(statResult.isFile));
       result->Set(v8::String::New("isDirectory"),
                   v8::Boolean::New(statResult.isDirectory));
       result->Set(v8::String::New("lastModified"),
-                  v8::Number::New(statResult.lastModified));
-      result->Set(v8::String::New("error"), v8::String::New(error.c_str()));
+                  v8::Integer::New(statResult.lastModified));
+      result->Set(v8::String::New("error"), Utils::ToV8String(error));
       callback->Call(callback, 1,
                      reinterpret_cast<v8::Handle<v8::Value>*>(&result));
       delete this;
     }
 
   private:
     std::string path;
   };
 
-  std::string CheckArguments(const v8::Arguments& arguments,
-                             const std::string& functionName,

[Wladimir Palant, 2013/04/12 16:10:35]

Will arguments.Callee()->GetName() do as well?

-                             const std::vector<std::string> requiredTypes)
-  {
-    const int requiredCount = requiredTypes.size();
-    if (arguments.Length() != requiredCount)
-    {
-      std::stringstream stream;
-      stream << functionName << " requires " << requiredCount
-             << (requiredCount == 1 ? " parameter" : " parameters");
-      return stream.str();
-    }
-
-    for (int i = 0; i < requiredCount; i++)
-    {
-      const v8::Handle<v8::Value> argument = arguments[i];
-      const std::string requiredType = requiredTypes[i];
-      if ((requiredType == "string" && !argument->IsString())
-          || (requiredType == "number" && !argument->IsNumber())

[Wladimir Palant, 2013/04/12 16:10:35]

Please note that a parameter doesn't need to be a string or a number for the
conversion to string/number to be possible. On the other hand, a string can have
an overloaded toString() method that will throw an exception - a conversion to a
string will fail then. Similarly with a valueOf() method of numbers.

You just have to try the conversion, no point checking the types up front.

-          || (requiredType == "function" && !argument->IsFunction()))
-      {
-        std::vector<std::string> countWords;
-        countWords.push_back("First");
-        countWords.push_back("Second");
-        countWords.push_back("Third");
-        std::stringstream stream;
-        if (i < countWords.size())
-          stream << countWords[i] << " argument";
-        else
-          stream << "Argument " << i;

[Wladimir Palant, 2013/04/12 16:10:35]

I would opt against unnecessary complexity - please get rid of countWords and
just go into the else case every time.

-        stream << " to " << functionName << " must be a " << requiredType;
-        return stream.str();
-      }
-    }
-
-    return "";
-  }
-
   v8::Handle<v8::Value> ReadCallback(const v8::Arguments& arguments)
   {
-    std::vector<std::string> requiredTypes;
-    requiredTypes.push_back("string");
-    requiredTypes.push_back("function");
-    const std::string error = CheckArguments(arguments, "_fileSystem.read",
-                                             requiredTypes);

[Wladimir Palant, 2013/04/12 16:10:35]

Mozilla opted for a more trivial CheckArguments("sf", ...) (see
https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_Conv...)
- maybe three additional lines and two extra objects really aren't worth the
effort here?

I seriously wonder whether we can both check the arguments and convert them in
the same helper function...

[Felix Dahlke, 2013/04/15 03:43:34]

I mainly introduced this to get the duplication down, the argument checking made
these callback functions twice as big. Seems like Mozilla's version is just the
same, but with a format string instead of the admittedly not very elegant type
vector.

But as you observed above, this doesn't make much sense as it is - rather than
checking the types and ignoring exceptions during the conversion, I should
ignore the types, attempt conversions and handle any exceptions. I'll do it like
that, shouldn't be too much duplication.

Actually converting the arguments in the check function would be possible using
void** or boost::any, but I guess it's not too bad to have the exception
checking duplicated.

-    if (error.length())
-      return v8::ThrowException(v8::String::New(error.c_str()));
-
-    const v8::Handle<const v8::External> external =
-      v8::Handle<const v8::External>::Cast(arguments.Data());
-    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
-    const std::string path = *v8::String::Utf8Value(arguments[0]->ToString());
-    v8::Persistent<v8::Function> callback = v8::Persistent<v8::Function>::New(
-      v8::Isolate::GetCurrent(), v8::Handle<v8::Function>::Cast(arguments[1]));
+    const v8::Handle<const v8::External> external =
+      v8::Handle<const v8::External>::Cast(arguments.Data());
+    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
+    if (arguments.Length() != 2)
+      return v8::ThrowException(v8::String::New(
+        "_fileSystem.read requires 2 parameters"));
+    const std::string path = Utils::FromV8String(arguments[0]);
+    v8::Handle<v8::Value> callbackValue = arguments[1];
+    if (!callbackValue->IsFunction())
+      return v8::ThrowException(v8::String::New(
+        "Second argument to _fileSystem.read must be a function"));
+    v8::Handle<v8::Function> callback =
+      v8::Handle<v8::Function>::Cast(callbackValue);
     ReadThread* const readThread = new ReadThread(*fileSystem, callback, path);
     readThread->Start();
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> WriteCallback(const v8::Arguments& arguments)
   {
-    std::vector<std::string> requiredTypes;
-    requiredTypes.push_back("string");
-    requiredTypes.push_back("string");
-    requiredTypes.push_back("function");
-    const std::string error = CheckArguments(arguments, "_fileSystem.write",
-                                             requiredTypes);
-    if (error.length())
-      return v8::ThrowException(v8::String::New(error.c_str()));
-
-    const v8::Handle<const v8::External> external =
-      v8::Handle<const v8::External>::Cast(arguments.Data());
-    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
-    const std::string path = *v8::String::Utf8Value(arguments[0]->ToString());
-    const std::string content =
-      *v8::String::Utf8Value(arguments[1]->ToString());
-    v8::Persistent<v8::Function> callback = v8::Persistent<v8::Function>::New(
-      v8::Isolate::GetCurrent(), v8::Handle<v8::Function>::Cast(arguments[2]));
+    const v8::Handle<const v8::External> external =
+      v8::Handle<const v8::External>::Cast(arguments.Data());
+    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
+    if (arguments.Length() != 3)
+      return v8::ThrowException(v8::String::New(
+        "_fileSystem.write requires 3 parameters"));
+    const std::string path = Utils::FromV8String(arguments[0]);
+    const std::string content = Utils::FromV8String(arguments[1]);
+    v8::Handle<v8::Value> callbackValue = arguments[2];
+    if (!callbackValue->IsFunction())
+      return v8::ThrowException(v8::String::New(
+        "Third argument to _fileSystem.write must be a function"));
+    v8::Handle<v8::Function> callback =
+      v8::Handle<v8::Function>::Cast(callbackValue);
     WriteThread* const writeThread = new WriteThread(*fileSystem, callback,
                                                      path, content);

[Wladimir Palant, 2013/04/12 16:10:35]

Just a thought: with the content parameter being a potentially large string that
WriteThread will have to copy, maybe it's worth instantiating std::string
instances on the heap and passing them via shared_ptr?

     writeThread->Start();
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> MoveCallback(const v8::Arguments& arguments)
   {
-    std::vector<std::string> requiredTypes;
-    requiredTypes.push_back("string");
-    requiredTypes.push_back("string");
-    requiredTypes.push_back("function");
-    const std::string error = CheckArguments(arguments, "_fileSystem.move",
-                                             requiredTypes);
-    if (error.length())
-      return v8::ThrowException(v8::String::New(error.c_str()));
-
-    const v8::Handle<const v8::External> external =
-      v8::Handle<const v8::External>::Cast(arguments.Data());
-    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
-    const std::string fromPath =
-      *v8::String::Utf8Value(arguments[0]->ToString());
-    const std::string toPath = *v8::String::Utf8Value(arguments[1]->ToString());
-    v8::Persistent<v8::Function> callback = v8::Persistent<v8::Function>::New(
-      v8::Isolate::GetCurrent(), v8::Handle<v8::Function>::Cast(arguments[2]));
+    const v8::Handle<const v8::External> external =
+      v8::Handle<const v8::External>::Cast(arguments.Data());
+    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
+    if (arguments.Length() != 3)
+      return v8::ThrowException(v8::String::New(
+        "_fileSystem.move requires 3 parameters"));
+    const std::string fromPath = Utils::FromV8String(arguments[0]);
+    const std::string toPath = Utils::FromV8String(arguments[1]);
+    v8::Handle<v8::Value> callbackValue = arguments[2];
+    if (!callbackValue->IsFunction())
+      return v8::ThrowException(v8::String::New(
+        "Third argument to _fileSystem.move must be a function"));
+    v8::Handle<v8::Function> callback =
+      v8::Handle<v8::Function>::Cast(callbackValue);
     MoveThread* const moveThread = new MoveThread(*fileSystem, callback,
                                                   fromPath, toPath);
     moveThread->Start();
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> RemoveCallback(const v8::Arguments& arguments)
   {
-    std::vector<std::string> requiredTypes;
-    requiredTypes.push_back("string");
-    requiredTypes.push_back("function");
-    const std::string error = CheckArguments(arguments, "_fileSystem.remove",
-                                             requiredTypes);
-    if (error.length())
-      return v8::ThrowException(v8::String::New(error.c_str()));
-
-    const v8::Handle<const v8::External> external =
-      v8::Handle<const v8::External>::Cast(arguments.Data());
-    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
-    const std::string path = *v8::String::Utf8Value(arguments[0]->ToString());
-    v8::Persistent<v8::Function> callback = v8::Persistent<v8::Function>::New(
-      v8::Isolate::GetCurrent(), v8::Handle<v8::Function>::Cast(arguments[1]));
+    const v8::Handle<const v8::External> external =
+      v8::Handle<const v8::External>::Cast(arguments.Data());
+    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
+    if (arguments.Length() != 2)
+      return v8::ThrowException(v8::String::New(
+        "_fileSystem.remove requires 2 parameters"));
+    const std::string path = Utils::FromV8String(arguments[0]);
+    v8::Handle<v8::Value> callbackValue = arguments[1];
+    if (!callbackValue->IsFunction())
+      return v8::ThrowException(v8::String::New(
+        "Second argument to _fileSystem.remove must be a function"));
+    v8::Handle<v8::Function> callback =
+      v8::Handle<v8::Function>::Cast(callbackValue);
     RemoveThread* const removeThread = new RemoveThread(*fileSystem, callback,
                                                         path);
     removeThread->Start();
     return v8::Undefined();
   }
 
   v8::Handle<v8::Value> StatCallback(const v8::Arguments& arguments)
   {
-    std::vector<std::string> requiredTypes;
-    requiredTypes.push_back("string");
-    requiredTypes.push_back("function");
-    const std::string error = CheckArguments(arguments, "_fileSystem.stat",
-                                             requiredTypes);
-    if (error.length())
-      return v8::ThrowException(v8::String::New(error.c_str()));
-
-    const v8::Handle<const v8::External> external =
-      v8::Handle<const v8::External>::Cast(arguments.Data());
-    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
-    const std::string path = *v8::String::Utf8Value(arguments[0]->ToString());
-    v8::Persistent<v8::Function> callback = v8::Persistent<v8::Function>::New(
-      v8::Isolate::GetCurrent(), v8::Handle<v8::Function>::Cast(arguments[1]));
+    const v8::Handle<const v8::External> external =
+      v8::Handle<const v8::External>::Cast(arguments.Data());
+    FileSystem* const fileSystem = static_cast<FileSystem*>(external->Value());
+    if (arguments.Length() != 2)
+      return v8::ThrowException(v8::String::New(
+        "_fileSystem.stat requires 2 parameters"));
+    const std::string path = Utils::FromV8String(arguments[0]);
+    v8::Handle<v8::Value> callbackValue = arguments[1];
+    if (!callbackValue->IsFunction())
+      return v8::ThrowException(v8::String::New(
+        "Second argument to _fileSystem.stat must be a function"));
+    v8::Handle<v8::Function> callback =
+      v8::Handle<v8::Function>::Cast(callbackValue);
     StatThread* const statThread = new StatThread(*fileSystem, callback, path);
     statThread->Start();
     return v8::Undefined();
   }
 }
 
 v8::Handle<v8::ObjectTemplate>
 FileSystemJsObject::Create(FileSystem& fileSystem)
 {
   const v8::Locker locker(v8::Isolate::GetCurrent());
   v8::HandleScope handleScope;
   const v8::Handle<v8::ObjectTemplate> file = v8::ObjectTemplate::New();
+  const v8::Local<v8::External> fileSystemExternal =
+    v8::External::New(&fileSystem);
   file->Set(v8::String::New("read"),
-    v8::FunctionTemplate::New(ReadCallback, v8::External::New(&fileSystem)));

[Wladimir Palant, 2013/04/12 16:10:35]

How about using the same v8::External instance for all callbacks?

+            v8::FunctionTemplate::New(ReadCallback, fileSystemExternal));
   file->Set(v8::String::New("write"),
-    v8::FunctionTemplate::New(WriteCallback, v8::External::New(&fileSystem)));
+            v8::FunctionTemplate::New(WriteCallback, fileSystemExternal));
   file->Set(v8::String::New("move"),
-    v8::FunctionTemplate::New(MoveCallback, v8::External::New(&fileSystem)));
+            v8::FunctionTemplate::New(MoveCallback, fileSystemExternal));
   file->Set(v8::String::New("remove"),
-    v8::FunctionTemplate::New(RemoveCallback, v8::External::New(&fileSystem)));
+            v8::FunctionTemplate::New(RemoveCallback, fileSystemExternal));
   file->Set(v8::String::New("stat"),
-    v8::FunctionTemplate::New(StatCallback, v8::External::New(&fileSystem)));
+            v8::FunctionTemplate::New(StatCallback, fileSystemExternal));
   return handleScope.Close(file);
 }