Implement File API

I've implemented the File I/O API we seem to need, that is:

- read
- write
- move
- remove
- stat

I began to fight duplication, but there's still some in the code base. I'm planning to create a common base class for all the JS objects with some base callback thread class like IoThread at some point.

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

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/File...
File include/AdblockPlus/FileSystem.h (right):

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/File...
include/AdblockPlus/FileSystem.h:22: virtual std::auto_ptr<std::istream>
Read(const std::string& path) const = 0;
I guess the reason for not returning std::string here is memory efficiency?

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/JsEn...
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/JsEn...
include/AdblockPlus/JsEngine.h:33: const FileSystem* const fileSystem;
Do we still need that variable? It's only being used by JsEngine::Load() which
is unused and should probably be removed.

http://codereview.adblockplus.org/10296001/diff/5001/shell/src/Main.cpp
File shell/src/Main.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/shell/src/Main.cpp#newco...
shell/src/Main.cpp:20: file->open(("lib/" + path).c_str());
I don't think this dummy implementation is useful. We no longer load scripts
from the file system, if anything we should have a full-fetched implementation
here that would actually allow ABP to save data. But then it doesn't make sense
to keep the implementation private to the shell. How about a DefaultFileSystem
class that would work out of the box on platforms we support? It could take the
base directory as constructor parameter to make sure special scenarios are
considered properly (Android).

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.cpp
File src/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:17: IoThread(FileSystem& fileSystem,
v8::Persistent<v8::Function> callback)
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.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:70: result->Set(v8::String::New("error"),
v8::String::New(error.c_str()));
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.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:230: v8::Number::New(statResult.lastModified));
Probably better to use v8::Integer::New() here and declare lastModified as
int64_t - avoid an int to double conversion.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:242: const std::string& functionName,
Will arguments.Callee()->GetName() do as well?

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:259: || (requiredType == "number" &&
!argument->IsNumber())
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.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:270: stream << "Argument " << i;
I would opt against unnecessary complexity - please get rid of countWords and
just go into the else case every time.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:285: requiredTypes);
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...

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:320: path, content);
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?

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:401: v8::FunctionTemplate::New(ReadCallback,
v8::External::New(&fileSystem)));
How about using the same v8::External instance for all callbacks?

http://codereview.adblockplus.org/10296001/diff/5001/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/src/GlobalJsObject.cpp#n...
src/GlobalJsObject.cpp:107: const v8::Handle<v8::ObjectTemplate> console =
console => consoleObject for consistency?

http://codereview.adblockplus.org/10296001/diff/5001/src/JsEngine.cpp
File src/JsEngine.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/src/JsEngine.cpp#newcode88
src/JsEngine.cpp:88: if (!file.get() || !*file)
I would suspect that !file.get() is the same as !file (IMHO smart pointers
overload that operation).

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

I'll prepare a new change set in a bit, comments below.

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/File...
File include/AdblockPlus/FileSystem.h (right):

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/File...
include/AdblockPlus/FileSystem.h:22: virtual std::auto_ptr<std::istream>
Read(const std::string& path) const = 0;
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> I guess the reason for not returning std::string here is memory efficiency?

It's probably not really needed, as the whole string is slurped later anyway.
Would make things easier, any objections to making this return string?

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/JsEn...
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/10296001/diff/5001/include/AdblockPlus/JsEn...
include/AdblockPlus/JsEngine.h:33: const FileSystem* const fileSystem;
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> Do we still need that variable? It's only being used by JsEngine::Load() which
> is unused and should probably be removed.

Wasn't aware that Load was unused. Yeah, we should remove it then, and won't
need fileSystem anymore either.

http://codereview.adblockplus.org/10296001/diff/5001/shell/src/Main.cpp
File shell/src/Main.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/shell/src/Main.cpp#newco...
shell/src/Main.cpp:20: file->open(("lib/" + path).c_str());
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> I don't think this dummy implementation is useful. We no longer load scripts
> from the file system, if anything we should have a full-fetched implementation
> here that would actually allow ABP to save data. But then it doesn't make
sense
> to keep the implementation private to the shell. How about a DefaultFileSystem
> class that would work out of the box on platforms we support? It could take
the
> base directory as constructor parameter to make sure special scenarios are
> considered properly (Android).

Fair enough, will do that.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.cpp
File src/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:17: IoThread(FileSystem& fileSystem,
v8::Persistent<v8::Function> callback)
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> 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.

Agreed, I actually meant to change this but forgot it seems.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:70: result->Set(v8::String::New("error"),
v8::String::New(error.c_str()));
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> 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.

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

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:230: v8::Number::New(statResult.lastModified));
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> Probably better to use v8::Integer::New() here and declare lastModified as
> int64_t - avoid an int to double conversion.

I used a signed int for the time as that's what time_t actually is on most
systems, but you're right, that's not 2038 safe.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:259: || (requiredType == "number" &&
!argument->IsNumber())
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> 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.

You're right. I somehow thought that IsString() would just see if conversion is
possible, but looking into how this is implemented in V8, I don't see any such
thing. (Nor would it make much sense now that I think about it.)

I'll just do the conversion and handle exceptions then.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:285: requiredTypes);
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> 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...

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.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:320: path, content);
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> 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?

I agree that strings can potentially be large, but I'd rather keep this simple
unless it turns out to be a problem, which I doubt.

Also, we already copy it twice above: Once within v8::String::Utf8Value, once by
creating an std::string from that. So if we were to optimise this, I'd pass in a
shared_ptr to the char* we get from Utf8Value. Or probably rather an ostream.

http://codereview.adblockplus.org/10296001/diff/5001/src/FileSystemJsObject.c...
src/FileSystemJsObject.cpp:401: v8::FunctionTemplate::New(ReadCallback,
v8::External::New(&fileSystem)));
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> How about using the same v8::External instance for all callbacks?

Agreed.

http://codereview.adblockplus.org/10296001/diff/5001/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/src/GlobalJsObject.cpp#n...
src/GlobalJsObject.cpp:107: const v8::Handle<v8::ObjectTemplate> console =
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> console => consoleObject for consistency?

Agreed.

http://codereview.adblockplus.org/10296001/diff/5001/src/JsEngine.cpp
File src/JsEngine.cpp (right):

http://codereview.adblockplus.org/10296001/diff/5001/src/JsEngine.cpp#newcode88
src/JsEngine.cpp:88: if (!file.get() || !*file)
On 2013/04/12 16:10:35, Wladimir Palant wrote:
> I would suspect that !file.get() is the same as !file (IMHO smart pointers
> overload that operation).

No, auto_ptr doesn't. Surprised me as well.

[Felix Dahlke, 2013-04-15 13:42:15]

I've addressed all the issues, except:

- Use std::ostream for Write (discussed that with Wladimir)
- Provide a default implementation of FileSystem to be used by abpshell

I'll work on that tomorrow.

[Wladimir Palant, 2013-04-16 06:50:13]

I assume that you want to remove JsEngine::Load() in a separate change. What
about implementing DefaultFileSystem()?

http://codereview.adblockplus.org/10296001/diff/13001/src/FileSystemJsObject.cpp
File src/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/13001/src/FileSystemJsObject....
src/FileSystemJsObject.cpp:247: requiredTypes.push_back("function");
requiredTypes is no longer used - and arguments.Length() isn't being checked
(same in all other callbacks).

http://codereview.adblockplus.org/10296001/diff/13001/src/FileSystemJsObject....
src/FileSystemJsObject.cpp:248: v8::TryCatch tryCatch;
If you are using TryCatch then you might want to be consistent and have it in
all callbacks...

[Wladimir Palant, 2013-04-16 06:52:44]

http://codereview.adblockplus.org/10296001/diff/13001/src/WebRequestJsObject.cpp
File src/WebRequestJsObject.cpp (left):

http://codereview.adblockplus.org/10296001/diff/13001/src/WebRequestJsObject....
src/WebRequestJsObject.cpp:20: }
You might want to revert that change. My refactoring in
http://codereview.adblockplus.org/10184021/ already made these functions
unnecessary, no point making the merge more complicated than it already is.

[Felix Dahlke, 2013-04-16 11:57:49]

Uploaded a new change set fixing all remaining issues, adding some tests, using
and std::ostream for Write and providing a default implementation of FileSystem.
I also used shared_ptr for the streams.

On 2013/04/16 06:50:13, Wladimir Palant wrote:
> I assume that you want to remove JsEngine::Load() in a separate change.

Yes, not looking to make this any more messy :)

> What about implementing DefaultFileSystem()?

Like I said in the previous comment, was still working on it. It's done now.

>
http://codereview.adblockplus.org/10296001/diff/13001/src/FileSystemJsObject.cpp
> File src/FileSystemJsObject.cpp (right):
> 
>
http://codereview.adblockplus.org/10296001/diff/13001/src/FileSystemJsObject....
> src/FileSystemJsObject.cpp:247: requiredTypes.push_back("function");
> requiredTypes is no longer used - and arguments.Length() isn't being checked
> (same in all other callbacks).

Oops, done.

>
http://codereview.adblockplus.org/10296001/diff/13001/src/FileSystemJsObject....
> src/FileSystemJsObject.cpp:248: v8::TryCatch tryCatch;
> If you are using TryCatch then you might want to be consistent and have it in
> all callbacks...

That was a leftover, it's not actually necessary. Added tests with illegal
invocations of all FS functions.

>
http://codereview.adblockplus.org/10296001/diff/13001/src/WebRequestJsObject....
> src/WebRequestJsObject.cpp:20: }
> You might want to revert that change. My refactoring in
> http://codereview.adblockplus.org/10184021/ already made these functions
> unnecessary, no point making the merge more complicated than it already is.

Done.

[Wladimir Palant, 2013-04-16 13:03:41]

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:1: #include <AdblockPlus/DefaultFileSystem.h>
A general concern with this API: what about non-ASCII file paths? Our strings
use UTF-8, my understanding is that it should work on most Linux systems. On
Windows we need to use the wide-char variants of the functions however.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:16: DefaultFileSystem::Read(const std::string& path)
const
Is this expecting an absolute path? I'm not sure whether this is wise - the JS
code has no way of knowing in which directory it needs to store its files. That
directory is app/os dependent and it would probably be easiest to use relative
paths here - and the base path would be passed in to the DefaultFileSystem
constructor. Alternatively the JS code would need a way to get the base
directory (e.g. via info module).

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:19: file->open(path.c_str());
Why not open the file in the constructor already? Same in ::Write() below.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:35: throw std::runtime_error("Failed to move " +
fromPath + " to " + toPath);
How about adding strerror(errno) to the error string? Otherwise we might have a
hard time debugging what went wrong.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:41: throw std::runtime_error("Failed to remove " +
path);
Same here, how about adding strerror(errno) to the output?

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:47: throw std::runtime_error("Stat is not implemented
on Windows");
Why not use _stat64 on Windows? You have the right header file included already.

http://codereview.adblockplus.org/10296001/diff/29001/src/FileSystemJsObject.cpp
File src/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/29001/src/FileSystemJsObject....
src/FileSystemJsObject.cpp:250: if (!arguments.Length() == 2)
Does this actually work? :)
arguments.Length() != 2

[Felix Dahlke, 2013-04-16 14:00:09]

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:1: #include <AdblockPlus/DefaultFileSystem.h>
On 2013/04/16 13:03:41, Wladimir Palant wrote:
> A general concern with this API: what about non-ASCII file paths? Our strings
> use UTF-8, my understanding is that it should work on most Linux systems. On
> Windows we need to use the wide-char variants of the functions however.

Which functions in particular? rename and remove below? I assumed they'd use
UTF-8 even on Windows, but I didn't test it yet.

Generally, we agreed to make libadblockplus use UTF-8 only, that means anyone
implementing FileSystem needs to work with UTF-8 paths.

According to the docs for Windows' _stat, we might get into trouble here:
"_stat automatically handles multibyte-character string arguments as
appropriate, recognizing multibyte-character sequences according to the
multibyte code page currently in use."

I'd rather tackle that once it's committed though.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:16: DefaultFileSystem::Read(const std::string& path)
const
On 2013/04/16 13:03:41, Wladimir Palant wrote:
> Is this expecting an absolute path? I'm not sure whether this is wise - the JS
> code has no way of knowing in which directory it needs to store its files.
That
> directory is app/os dependent and it would probably be easiest to use relative
> paths here - and the base path would be passed in to the DefaultFileSystem
> constructor. Alternatively the JS code would need a way to get the base
> directory (e.g. via info module).

This supports both relative and absolute paths, I'm using relative paths in the
tests. The client can just change the working directory to get the behaviour
you'd like.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:19: file->open(path.c_str());
On 2013/04/16 13:03:41, Wladimir Palant wrote:
> Why not open the file in the constructor already? Same in ::Write() below.

Done.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:35: throw std::runtime_error("Failed to move " +
fromPath + " to " + toPath);
On 2013/04/16 13:03:41, Wladimir Palant wrote:
> How about adding strerror(errno) to the error string? Otherwise we might have
a
> hard time debugging what went wrong.

Done.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:41: throw std::runtime_error("Failed to remove " +
path);
On 2013/04/16 13:03:41, Wladimir Palant wrote:
> Same here, how about adding strerror(errno) to the output?

Done.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:47: throw std::runtime_error("Stat is not implemented
on Windows");
On 2013/04/16 13:03:41, Wladimir Palant wrote:
> Why not use _stat64 on Windows? You have the right header file included
already.

I really didn't expect Windows to support POSIX stat :D Using _stat on Windows
and stat on other systems now. stat64 appears to be deprecated, at least
according to Clang. My book "Advanced Programming in the UNIX Environment"
doesn't even mention it.

http://codereview.adblockplus.org/10296001/diff/29001/src/FileSystemJsObject.cpp
File src/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/29001/src/FileSystemJsObject....
src/FileSystemJsObject.cpp:250: if (!arguments.Length() == 2)
On 2013/04/16 13:03:41, Wladimir Palant wrote:
> Does this actually work? :)
> arguments.Length() != 2

lol, yeah it does, but it sure is weird :) Fixed it here and below.

[Wladimir Palant, 2013-04-16 14:10:12]

LGTM

Feel free to push, I'll update my patches.

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10296001/diff/29001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:1: #include <AdblockPlus/DefaultFileSystem.h>
I suspect that all of cstdio and fstream maps to ASCII variants of Windows
functions - or does OEM conversion if we are lucky. I doubt that any of these
functions recognize UTF-8.

Looking into this after commit is fine with me.

http://codereview.adblockplus.org/10296001/diff/29001/src/FileSystemJsObject.cpp
File src/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10296001/diff/29001/src/FileSystemJsObject....
src/FileSystemJsObject.cpp:250: if (!arguments.Length() == 2)
Weird operator precedence rules then...