Issue 5183 - Provide async interface for FileSystem

Issue 5183 - Provide async interface for FileSystem

[hub, 2017-05-26 12:45:29]

the DefaultFileSystem implementation of the asynchronous API is still
synchronous.

Also I wasn't sure if you preferred signature overload or a different name for
the Async functions. I chose the former. Let me know if you prefer the latter.

[sergei, 2017-05-26 13:32:04]

On 2017/05/26 12:45:29, hub wrote:
> the DefaultFileSystem implementation of the asynchronous API is still
> synchronous.
> 
> Also I wasn't sure if you preferred signature overload or a different name for
> the Async functions. I chose the former. Let me know if you prefer the latter.
See comments in the code, I would like to have a different interface :)

I think it should be the same as for web request (this commit, BTW, pay
attention to that part
https://github.com/adblockplus/libadblockplus/commit/f454e507a392ad67e26d972c...)
but now for file system.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
File include/AdblockPlus/DefaultFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/DefaultFileSystem.h:37: class DefaultFileSystem : public
FileSystem
I think that there should be a different class implementing asynchronous file
system interface and accepting a smart pointer to synchronous implementation.
The implementation of async interface should run methods of sync interface in
detached threads so far.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
File include/AdblockPlus/FileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:31: class FileSystem
I expected to have a completely new interface with ansynchronous methods. I
think it will allow to gradually migrate to that new interface in other
projects. Currently it will require to make changes on the next update.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:70: typedef std::function<void (void)>
Callback;
We need a documentation for public things.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:70: typedef std::function<void (void)>
Callback;
Not sure whether we use a space between return type and arguments.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:70: typedef std::function<void (void)>
Callback;
Could you please not use (void), it's equivalent to ().

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:71: typedef std::function<void
(std::shared_ptr<std::istream>)> ReadCallback;
I would propose to change the signature to
std::function<void(const std::vector<uint8_t>&)>)>.
The idea of the asynchronous function is to not block the current execution by
IO operations, so when the callback is invoked the data should be available as
fast as possible, at least the content of the buffer used in the callback should
be already read into the memory, however the work with std::istream will block
the execution because it should read the data from the disk, for instance.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:129: virtual void Resolve(const std::string&
path,
I'm not sure whether resolve should be asynchronous. Even more, I suppose it
will be removed very soon, so I would propose to keep it as simple and small as
possible.

[hub, 2017-06-02 16:04:17]

Reworked the whole thing. Including the tests.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
File include/AdblockPlus/FileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:70: typedef std::function<void (void)>
Callback;
On 2017/05/26 13:32:04, sergei wrote:
> Not sure whether we use a space between return type and arguments.

I would put a space, but then elsewhere it doesn't have one, so I'll match that
style for consistency.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:70: typedef std::function<void (void)>
Callback;
On 2017/05/26 13:32:04, sergei wrote:
> Could you please not use (void), it's equivalent to ().

Acknowledged.

https://codereview.adblockplus.org/29449592/diff/29449593/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:129: virtual void Resolve(const std::string&
path,
On 2017/05/26 13:32:04, sergei wrote:
> I'm not sure whether resolve should be asynchronous. Even more, I suppose it
> will be removed very soon, so I would propose to keep it as simple and small
as
> possible.

I thought twice about that. I looked at NodeJS for example, and they have a
similar API that has both sync and async. 

I'll remove the async version.

https://codereview.adblockplus.org/29449592/diff/29454660/lib/init.js
File lib/init.js (right):

https://codereview.adblockplus.org/29449592/diff/29454660/lib/init.js#newcode38
lib/init.js:38: checkInitialized();
This is to fix an ordering issue where this might not work. It is needed for
this patch. (see the changes in prefs.js too)

[hub, 2017-06-02 19:36:15]

we can wait that you land the update of v8 first as it will need rebase.

[sergei, 2017-06-16 15:05:56]

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&,
const std::string&)> ReadCallback;
I think that the data type for the read data should be std::vector<uint8_t>
instead of std::string.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:99: std::shared_ptr<std::istream> data,
Instead of `std::shared_ptr<std::istream>` it would be better to use
std::vector<uint8_t>.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:144: typedef std::shared_ptr<IFileSystem>
FileSystemPtr;
OK now, but in future I think it should be std::unique_ptr.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:270: FileSystemPtr GetFileSystem() const;
I think we can remove the getter but still have the setter which accepts sync
`FileSystem`. This backward compatibility is important to ease the update in
other projects and also in tests (though with the proposed changes in
LazyFileSystem it's not needed). For instance, we often need to quickly update
libadblockplus in libadblockplus-android but have no time to change and test
FileSystem. So, to not introduce a delay one could update FileSystem in
libadblockplus-android later when there is no pressure.

https://codereview.adblockplus.org/29449592/diff/29454687/lib/init.js
File lib/init.js (right):

https://codereview.adblockplus.org/29449592/diff/29454687/lib/init.js#newcode37
lib/init.js:37: if (Prefs.initialized)
Could you please move all these changes in JS files (init.js + prefs.js) into a
separate commit? I see the point we have to have such changes now, but they seem
to be valid, pretty useful and self contained.

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
File src/FileSystemJsObject.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:48: [jsEngine, callback]
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...

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:53: result.SetProperty("content", content);
`content` should be moved, but getting into account std::vector, I guess, it
will be copied for present anyway.

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:57: callback.Call(params);
We can simply call `callback.Call(result);` without `params`.

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:85: params.push_back(errorValue);
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.

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h
File test/BaseJsTest.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:27: class Sync
There is a pretty similar class in src/FilterEngine.cpp. And although I'm not a
fan of it (e.g. it's totally exception unfriendly but we cannot use
std::promise/std::future for present), I would propose to move it into utils.

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:34: void Wait(int sec = 20)
For the class in utils I think it makes sense to use uint32_t in milliseconds
here and -1 for infinite waiting.

For tests either to have a separate class with a fixed default low value or
maybe a global constant. In case of separate class I prefer wrapping instead of
inheriting.

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:40: void Set(const std::string& error_ = std::string())
unfortunately we are not using anything like m_ prefix what I personally find
very inconvenient, but here I would recommend to use `this->error = error` and
there won't need to have "_".
Anyway, it seems this error is used only in one place, do you mind to simply
define it outside that class?

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:223: std::thread([this, path, callback]
I actually planned to gradually convert to a special Noop implementation and not
introduce threads, that's because I said that tests can be changed later. I was
also afraid that adding threads here can introduce data race in some tests (one
need to carefully go though all of them), though it seems not the case.

OK, let's leave it as is.

https://codereview.adblockplus.org/29449592/diff/29454687/test/DefaultFileSys...
File test/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/DefaultFileSys...
test/DefaultFileSystem.cpp:40: ASSERT_TRUE(error.empty());
What about EXPECT_TRUE when it's acceptable here and below?

https://codereview.adblockplus.org/29449592/diff/29454687/test/DefaultFileSys...
test/DefaultFileSystem.cpp:121: });
OK :)

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
File test/FileSystemJsObject.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
test/FileSystemJsObject.cpp:48: callback(std::move(std::string()),
std::move is not necessary here

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
test/FileSystemJsObject.cpp:49: std::string("Unable to read " + path));
Actually, here and below one can simply pass "some string" without explicit
construction of std::string.

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
test/FileSystemJsObject.cpp:52: callback(std::move(std::string(contentToRead)),
"");
std::move is not necessary here

https://codereview.adblockplus.org/29449592/diff/29454687/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/FilterEngine.c...
test/FilterEngine.cpp:603: int previousTimesCalled = timesCalled;
But its value is still one. Could you move it into a separate commit?

Maybe only add the comment without new variable previousTimesCalled?

[hub, 2017-06-16 21:52:56]

Patch updated.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&,
const std::string&)> ReadCallback;
On 2017/06/16 15:05:54, sergei wrote:
> I think that the data type for the read data should be std::vector<uint8_t>
> instead of std::string.

I don't think it is better since further down in the JavaScript interface we
pass string objects.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:99: std::shared_ptr<std::istream> data,
On 2017/06/16 15:05:54, sergei wrote:
> Instead of `std::shared_ptr<std::istream>` it would be better to use
> std::vector<uint8_t>.

I'd rather use std::string for the same reason as above: the JavaScript
interface is all String objects.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:144: typedef std::shared_ptr<IFileSystem>
FileSystemPtr;
On 2017/06/16 15:05:54, sergei wrote:
> OK now, but in future I think it should be std::unique_ptr.

Acknowledged.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:270: FileSystemPtr GetFileSystem() const;
On 2017/06/16 15:05:54, sergei wrote:
> I think we can remove the getter but still have the setter which accepts sync
> `FileSystem`. This backward compatibility is important to ease the update in
> other projects and also in tests (though with the proposed changes in
> LazyFileSystem it's not needed). For instance, we often need to quickly update
> libadblockplus in libadblockplus-android but have no time to change and test
> FileSystem. So, to not introduce a delay one could update FileSystem in
> libadblockplus-android later when there is no pressure.

I need this accessor because it is use in FileSystemJsObject. But I should
possibly change it.

And I'll restore the setter for the Sync implementation.

https://codereview.adblockplus.org/29449592/diff/29454687/lib/init.js
File lib/init.js (right):

https://codereview.adblockplus.org/29449592/diff/29454687/lib/init.js#newcode37
lib/init.js:37: if (Prefs.initialized)
On 2017/06/16 15:05:54, sergei wrote:
> Could you please move all these changes in JS files (init.js + prefs.js) into
a
> separate commit? I see the point we have to have such changes now, but they
seem
> to be valid, pretty useful and self contained.

Done.

https://codereview.adblockplus.org/29467599

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
File src/FileSystemJsObject.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:48: [jsEngine, callback]
On 2017/06/16 15:05:55, sergei wrote:
> 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...

Done.

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:53: result.SetProperty("content", content);
On 2017/06/16 15:05:55, sergei wrote:
> `content` should be moved, but getting into account std::vector, I guess, it
> will be copied for present anyway.

Acknowledged.

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:57: callback.Call(params);
On 2017/06/16 15:05:55, sergei wrote:
> We can simply call `callback.Call(result);` without `params`.

Done.

https://codereview.adblockplus.org/29449592/diff/29454687/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:85: params.push_back(errorValue);
On 2017/06/16 15:05:55, sergei wrote:
> 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.

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

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h
File test/BaseJsTest.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:27: class Sync
On 2017/06/16 15:05:55, sergei wrote:
> There is a pretty similar class in src/FilterEngine.cpp. And although I'm not
a
> fan of it (e.g. it's totally exception unfriendly but we cannot use
> std::promise/std::future for present), I would propose to move it into utils.

That's where it comes from.

The problem with Utils.h is that we can't really include it in test because it
includes some v8 headers that seems to not be allowed in test.

Also I'm ok moving this somewhere where it is useful.

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:34: void Wait(int sec = 20)
On 2017/06/16 15:05:55, sergei wrote:
> For the class in utils I think it makes sense to use uint32_t in milliseconds
> here and -1 for infinite waiting.
> 
> For tests either to have a separate class with a fixed default low value or
> maybe a global constant. In case of separate class I prefer wrapping instead
of
> inheriting.

At that point I'd that have Wait() to have a std::chrono::duration as an
argument. That way we get to use whatever.

Also uint32_t and -1 one as value, not pretty. I'll use
std::chrono::duration::zero() as "no timeout"

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:40: void Set(const std::string& error_ = std::string())
On 2017/06/16 15:05:55, sergei wrote:
> unfortunately we are not using anything like m_ prefix what I personally find
> very inconvenient, but here I would recommend to use `this->error = error` and
> there won't need to have "_".
> Anyway, it seems this error is used only in one place, do you mind to simply
> define it outside that class?

This is actually for -Wshadow that sadly isn't enabled. Using "this->" doesn't
fix it.

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:223: std::thread([this, path, callback]
On 2017/06/16 15:05:55, sergei wrote:
> I actually planned to gradually convert to a special Noop implementation and
not
> introduce threads, that's because I said that tests can be changed later. I
was
> also afraid that adding threads here can introduce data race in some tests
(one
> need to carefully go though all of them), though it seems not the case.
> 
> OK, let's leave it as is.

Acknowledged.

https://codereview.adblockplus.org/29449592/diff/29454687/test/DefaultFileSys...
File test/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/DefaultFileSys...
test/DefaultFileSystem.cpp:40: ASSERT_TRUE(error.empty());
On 2017/06/16 15:05:56, sergei wrote:
> What about EXPECT_TRUE when it's acceptable here and below?

Done.

https://codereview.adblockplus.org/29449592/diff/29454687/test/DefaultFileSys...
test/DefaultFileSystem.cpp:121: });
On 2017/06/16 15:05:55, sergei wrote:
> OK :)

I know, right?

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
File test/FileSystemJsObject.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
test/FileSystemJsObject.cpp:48: callback(std::move(std::string()),
On 2017/06/16 15:05:56, sergei wrote:
> std::move is not necessary here

Done.

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
test/FileSystemJsObject.cpp:49: std::string("Unable to read " + path));
On 2017/06/16 15:05:56, sergei wrote:
> Actually, here and below one can simply pass "some string" without explicit
> construction of std::string.

Done.

https://codereview.adblockplus.org/29449592/diff/29454687/test/FileSystemJsOb...
test/FileSystemJsObject.cpp:52: callback(std::move(std::string(contentToRead)),
"");
On 2017/06/16 15:05:56, sergei wrote:
> std::move is not necessary here

Done.

https://codereview.adblockplus.org/29449592/diff/29454687/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/FilterEngine.c...
test/FilterEngine.cpp:603: int previousTimesCalled = timesCalled;
On 2017/06/16 15:05:56, sergei wrote:
> But its value is still one. Could you move it into a separate commit?
> 
> Maybe only add the comment without new variable previousTimesCalled?

I'll just keep the comment.

[sergei, 2017-07-03 09:25:56]

I have quickly gone through the changes in last patch set, it seems almost
ready. I will take a deeper look after patchset based on landed
https://codereview.adblockplus.org/29451722/.

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&,
const std::string&)> ReadCallback;
On 2017/06/16 21:52:53, hub wrote:
> On 2017/06/16 15:05:54, sergei wrote:
> > I think that the data type for the read data should be std::vector<uint8_t>
> > instead of std::string.
> 
> I don't think it is better since further down in the JavaScript interface we
> pass string objects.

I think we can have it but in general I disagree with having only string method.
For string:
- convenient to use
- less code
Against:
- changing of the interface: previously, with std::istream, it was possible to
read and write any binary data
- IFileSystem is a low level API providing with independence from a platform
specific API which works with binary data, but here we introduce conversion to a
string
- if we ever need to work with binary data from JS (what can actually happen) we
will have to add a method working with binary data to IFileSystem and one will
have to change the corresponding implementation in other projects (e.g. for
Android). If we keep the interface working with binary data now then only
changes in libadblockplus will be required and the updating of the latter will
be seamless in other projects.

In order to avoid superfluous copying of data I would rather propose to add
helper methods to Utils constructing V8 string from a binary data and extracting
a binary data from a V8 string.

Just in case, std::string basically can contain any data but I find it as
abusing.

What do you think about it?

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h
File test/BaseJsTest.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/test/BaseJsTest.h#n...
test/BaseJsTest.h:27: class Sync
On 2017/06/16 21:52:54, hub wrote:
> On 2017/06/16 15:05:55, sergei wrote:
> > There is a pretty similar class in src/FilterEngine.cpp. And although I'm
not
> a
> > fan of it (e.g. it's totally exception unfriendly but we cannot use
> > std::promise/std::future for present), I would propose to move it into
utils.
> 
> That's where it comes from.
> 
> The problem with Utils.h is that we can't really include it in test because it
> includes some v8 headers that seems to not be allowed in test.
> 
> Also I'm ok moving this somewhere where it is useful.

Acknowledged.

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:99: std::vector<v8::Global<v8::Value>> values;
Wow, it seems to be rebased on new v8. That's very cool, I would only like to
ask to submit a new patch set after landing of
https://codereview.adblockplus.org/29451722/.

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:269: FileSystemPtr GetAsyncFileSystem() const;
From
https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
On 2017/06/16 21:52:54, hub wrote:
> On 2017/06/16 15:05:54, sergei wrote:
> > I think we can remove the getter but still have the setter which accepts
sync
> > `FileSystem`. This backward compatibility is important to ease the update in
> > other projects and also in tests (though with the proposed changes in
> > LazyFileSystem it's not needed). For instance, we often need to quickly
update
> > libadblockplus in libadblockplus-android but have no time to change and test
> > FileSystem. So, to not introduce a delay one could update FileSystem in
> > libadblockplus-android later when there is no pressure.
> 
> I need this accessor because it is use in FileSystemJsObject. But I should
> possibly change it.
Could you please say in the comment to the getter that it's a private
functionality?
> 
> And I'll restore the setter for the Sync implementation.

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:274: * `DefaultFileSystemSync` instance by
default, which might be
I think the comment should be a bit adjusted. By default the engine will use the
implementation created by `CreateDefaultFileSystem()`. The latter internally is
indeed currently based on `DefaultFileSystemSync` but I think it's a detail of
internal implementation.

https://codereview.adblockplus.org/29449592/diff/29467603/src/FileSystemJsObj...
File src/FileSystemJsObject.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29467603/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:47: [weakJsEngine, callback]
`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.

https://codereview.adblockplus.org/29449592/diff/29467603/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:114: params.push_back(errorValue);
On 2017/06/16 21:52:54, hub wrote:
> On 2017/06/16 15:05:55, sergei wrote:
> > 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.
> 
> We can rework this later I think. Currently the FileSystemJsObjectTest.Write
> test assume error to be "" if none.

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`.

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h
File src/Thread.h (right):

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h#newcode53
src/Thread.h:53: }
On 2017/06/16 21:52:55, hub wrote:
> On 2017/06/16 15:05:55, sergei wrote:
> > For the class in utils I think it makes sense to use uint32_t in
milliseconds
> > here and -1 for infinite waiting.
> > 
> > For tests either to have a separate class with a fixed default low value or
> > maybe a global constant. In case of separate class I prefer wrapping instead
> of
> > inheriting.
> 
> At that point I'd that have Wait() to have a std::chrono::duration as an
> argument. That way we get to use whatever.
> 
> Also uint32_t and -1 one as value, not pretty. I'll use
> std::chrono::duration::zero() as "no timeout"

Zero seems even less appropriate here but OK, I think it should not be an issue
currently. BTW, what about returning of member `set` from `Wait`?

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h#newcode71
src/Thread.h:71: bool set;
would it be better to call it as `isSet`?

[hub, 2017-07-04 19:58:28]

Beside the read / write API comments, I have dealt with the rest.

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:99: std::vector<v8::Global<v8::Value>> values;
On 2017/07/03 09:25:54, sergei wrote:
> Wow, it seems to be rebased on new v8. That's very cool, I would only like to
> ask to submit a new patch set after landing of
> https://codereview.adblockplus.org/29451722/.

Yes I did rebase on top of your patch. I was expecting it to land already. I
will definitely wait for it and rebase on master at that point.

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:269: FileSystemPtr GetAsyncFileSystem() const;
On 2017/07/03 09:25:54, sergei wrote:
> From
>
https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
> On 2017/06/16 21:52:54, hub wrote:
> > On 2017/06/16 15:05:54, sergei wrote:
> > > I think we can remove the getter but still have the setter which accepts
> sync
> > > `FileSystem`. This backward compatibility is important to ease the update
in
> > > other projects and also in tests (though with the proposed changes in
> > > LazyFileSystem it's not needed). For instance, we often need to quickly
> update
> > > libadblockplus in libadblockplus-android but have no time to change and
test
> > > FileSystem. So, to not introduce a delay one could update FileSystem in
> > > libadblockplus-android later when there is no pressure.
> > 
> > I need this accessor because it is use in FileSystemJsObject. But I should
> > possibly change it.
> Could you please say in the comment to the getter that it's a private
> functionality?
> > 
> > And I'll restore the setter for the Sync implementation.

Acknowledged.

https://codereview.adblockplus.org/29449592/diff/29467603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:274: * `DefaultFileSystemSync` instance by
default, which might be
On 2017/07/03 09:25:54, sergei wrote:
> I think the comment should be a bit adjusted. By default the engine will use
the
> implementation created by `CreateDefaultFileSystem()`. The latter internally
is
> indeed currently based on `DefaultFileSystemSync` but I think it's a detail of
> internal implementation.

Acknowledged.

https://codereview.adblockplus.org/29449592/diff/29467603/src/FileSystemJsObj...
File src/FileSystemJsObject.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29467603/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:47: [weakJsEngine, callback]
On 2017/07/03 09:25:54, sergei wrote:
> `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.

I'll fix that here and there.

https://codereview.adblockplus.org/29449592/diff/29467603/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:114: params.push_back(errorValue);
On 2017/07/03 09:25:54, sergei wrote:
> On 2017/06/16 21:52:54, hub wrote:
> > On 2017/06/16 15:05:55, sergei wrote:
> > > 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.
> > 
> > We can rework this later I think. Currently the FileSystemJsObjectTest.Write
> > test assume error to be "" if none.
> 
> 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`.

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.

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h
File src/Thread.h (right):

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h#newcode53
src/Thread.h:53: }
On 2017/07/03 09:25:56, sergei wrote:
> On 2017/06/16 21:52:55, hub wrote:
> > On 2017/06/16 15:05:55, sergei wrote:
> > > For the class in utils I think it makes sense to use uint32_t in
> milliseconds
> > > here and -1 for infinite waiting.
> > > 
> > > For tests either to have a separate class with a fixed default low value
or
> > > maybe a global constant. In case of separate class I prefer wrapping
instead
> > of
> > > inheriting.
> > 
> > At that point I'd that have Wait() to have a std::chrono::duration as an
> > argument. That way we get to use whatever.
> > 
> > Also uint32_t and -1 one as value, not pretty. I'll use
> > std::chrono::duration::zero() as "no timeout"
> 
> Zero seems even less appropriate here but OK, I think it should not be an
issue
> currently. BTW, what about returning of member `set` from `Wait`?

I wasn't super thrilled about it either.
Maybe we can have a `WaitFor` that takes the duration and have `Wait` not
timeout.
`WaitFor` will return false if timeout occured.

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h#newcode71
src/Thread.h:71: bool set;
On 2017/07/03 09:25:55, sergei wrote:
> would it be better to call it as `isSet`?

sure

[hub, 2017-07-04 21:20:14]

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&,
const std::string&)> ReadCallback;
On 2017/07/03 09:25:53, sergei wrote:
> On 2017/06/16 21:52:53, hub wrote:
> > On 2017/06/16 15:05:54, sergei wrote:
> > > I think that the data type for the read data should be
std::vector<uint8_t>
> > > instead of std::string.
> > 
> > I don't think it is better since further down in the JavaScript interface we
> > pass string objects.
> 
> I think we can have it but in general I disagree with having only string
method.
> For string:
> - convenient to use
> - less code
> Against:
> - changing of the interface: previously, with std::istream, it was possible to
> read and write any binary data
> - IFileSystem is a low level API providing with independence from a platform
> specific API which works with binary data, but here we introduce conversion to
a
> string
> - if we ever need to work with binary data from JS (what can actually happen)
we
> will have to add a method working with binary data to IFileSystem and one will
> have to change the corresponding implementation in other projects (e.g. for
> Android). If we keep the interface working with binary data now then only
> changes in libadblockplus will be required and the updating of the latter will
> be seamless in other projects.
> 
> In order to avoid superfluous copying of data I would rather propose to add
> helper methods to Utils constructing V8 string from a binary data and
extracting
> a binary data from a V8 string.
> 
> Just in case, std::string basically can contain any data but I find it as
> abusing.
> 
> What do you think about it?

IMHO the C++ iostream interface isn't really convenient for reading binary
buffers. If we really want to change things I'd go as far as changing the "sync"
interface to only manipulate buffers. This would actually make the abstraction
much friendlier to a platform specific implementation as we'd not pass streams
around.

[sergei, 2017-07-05 10:03:22]

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29454687/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef std::function<void(std::string&&,
const std::string&)> ReadCallback;
On 2017/07/04 21:20:14, hub wrote:
> On 2017/07/03 09:25:53, sergei wrote:
> > On 2017/06/16 21:52:53, hub wrote:
> > > On 2017/06/16 15:05:54, sergei wrote:
> > > > I think that the data type for the read data should be
> std::vector<uint8_t>
> > > > instead of std::string.
> > > 
> > > I don't think it is better since further down in the JavaScript interface
we
> > > pass string objects.
> > 
> > I think we can have it but in general I disagree with having only string
> method.
> > For string:
> > - convenient to use
> > - less code
> > Against:
> > - changing of the interface: previously, with std::istream, it was possible
to
> > read and write any binary data
> > - IFileSystem is a low level API providing with independence from a platform
> > specific API which works with binary data, but here we introduce conversion
to
> a
> > string
> > - if we ever need to work with binary data from JS (what can actually
happen)
> we
> > will have to add a method working with binary data to IFileSystem and one
will
> > have to change the corresponding implementation in other projects (e.g. for
> > Android). If we keep the interface working with binary data now then only
> > changes in libadblockplus will be required and the updating of the latter
will
> > be seamless in other projects.
> > 
> > In order to avoid superfluous copying of data I would rather propose to add
> > helper methods to Utils constructing V8 string from a binary data and
> extracting
> > a binary data from a V8 string.
> > 
> > Just in case, std::string basically can contain any data but I find it as
> > abusing.
> > 
> > What do you think about it?
> 
> IMHO the C++ iostream interface isn't really convenient for reading binary
> buffers. If we really want to change things I'd go as far as changing the
"sync"
> interface to only manipulate buffers. This would actually make the abstraction
> much friendlier to a platform specific implementation as we'd not pass streams
> around.
It's not actually difficult, one has to firstly obtain a size of the data
is.seekg(0, std::ios_base::end);
auto dataSize = is.tellg();
is.seekg (0, std::ios_base::beg);
and read the data
std::vector<uint8_t> data(dataSize);
is.read(data.data(), data.size());
and it seems we need to do it only once.
Nevertheless it's not the issue we are discussing here. I'm just saying that
previously it was possible to work with binary data and I think that that
possibility should be kept in the interface without abusing std::string, thus by
using another container which hold bytes (e.g. std::vector<uint8_t>).

What concerns changing of the sync interface, I was not sure about it because
std::ofstream/std::ifstream seem already pretty good abstraction from a platform
and changing of it would likely complicate updating of libadblockplus in other
projects, so I thought we should keep sync interface without that change for
present. However currently the change starting to use it in
libadblockplus-android in review and I think we should change the sync interface
now to simplify the code in that review :). So, I simply expect the reading code
above to be in the default implementation of the sync interface (I hope it won't
affect tests a lot).

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h
File src/Thread.h (right):

https://codereview.adblockplus.org/29449592/diff/29467603/src/Thread.h#newcode53
src/Thread.h:53: }
On 2017/07/04 19:58:27, hub wrote:
> On 2017/07/03 09:25:56, sergei wrote:
> > On 2017/06/16 21:52:55, hub wrote:
> > > On 2017/06/16 15:05:55, sergei wrote:
> > > > For the class in utils I think it makes sense to use uint32_t in
> > milliseconds
> > > > here and -1 for infinite waiting.
> > > > 
> > > > For tests either to have a separate class with a fixed default low value
> or
> > > > maybe a global constant. In case of separate class I prefer wrapping
> instead
> > > of
> > > > inheriting.
> > > 
> > > At that point I'd that have Wait() to have a std::chrono::duration as an
> > > argument. That way we get to use whatever.
> > > 
> > > Also uint32_t and -1 one as value, not pretty. I'll use
> > > std::chrono::duration::zero() as "no timeout"
> > 
> > Zero seems even less appropriate here but OK, I think it should not be an
> issue
> > currently. BTW, what about returning of member `set` from `Wait`?
> 
> I wasn't super thrilled about it either.
> Maybe we can have a `WaitFor` that takes the duration and have `Wait` not
> timeout.
> `WaitFor` will return false if timeout occured.
> 

I thought about it too, it actually looks very good.

https://codereview.adblockplus.org/29449592/diff/29479644/src/FilterEngine.cpp
File src/FilterEngine.cpp (left):

https://codereview.adblockplus.org/29449592/diff/29479644/src/FilterEngine.cp...
src/FilterEngine.cpp:185: class Sync
A minor thing, but do you mind to make this change (only concerning Sync) in
another commit?

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp#ne...
src/JsEngine.cpp:332: fileSystemLegacy = val;
BTW, this `fileSystemLegacy` member is not used, so if the user sets it it will
have no effect. It seems the best option is to create async implementation with
injected `val`.

https://codereview.adblockplus.org/29449592/diff/29479644/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29479644/test/JsEngine.cpp#n...
test/JsEngine.cpp:175: ASSERT_TRUE(jsEngine->GetAsyncFileSystem());
Since it's a private functionality, I think we can remove this line from tests.

[hub, 2017-07-06 12:21:53]

Patch updated. 

Still one comment unaddressed.

https://codereview.adblockplus.org/29449592/diff/29479644/src/FilterEngine.cpp
File src/FilterEngine.cpp (left):

https://codereview.adblockplus.org/29449592/diff/29479644/src/FilterEngine.cp...
src/FilterEngine.cpp:185: class Sync
On 2017/07/05 10:03:21, sergei wrote:
> A minor thing, but do you mind to make this change (only concerning Sync) in
> another commit?

this require some patch surgery to make it worth it.

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp#ne...
src/JsEngine.cpp:332: fileSystemLegacy = val;
On 2017/07/05 10:03:21, sergei wrote:
> BTW, this `fileSystemLegacy` member is not used, so if the user sets it it
will
> have no effect. It seems the best option is to create async implementation
with
> injected `val`.

We don't have a visibility of what filesystem has been created so we can't
simply recreate it. 

The few alternative I see are:
1. Add a method to IFileSystem to do this. We might need to address a few issues
in our own test though.
2. remove this method, but this would break API

One of these should happen.

https://codereview.adblockplus.org/29449592/diff/29479644/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29479644/test/JsEngine.cpp#n...
test/JsEngine.cpp:175: ASSERT_TRUE(jsEngine->GetAsyncFileSystem());
On 2017/07/05 10:03:21, sergei wrote:
> Since it's a private functionality, I think we can remove this line from
tests.

Done.

[sergei, 2017-07-06 14:14:06]

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp#ne...
src/JsEngine.cpp:332: fileSystemLegacy = val;
On 2017/07/06 12:21:52, hub wrote:
> On 2017/07/05 10:03:21, sergei wrote:
> > BTW, this `fileSystemLegacy` member is not used, so if the user sets it it
> will
> > have no effect. It seems the best option is to create async implementation
> with
> > injected `val`.
> 
> We don't have a visibility of what filesystem has been created so we can't
> simply recreate it. 
> 
> The few alternative I see are:
> 1. Add a method to IFileSystem to do this. We might need to address a few
issues
> in our own test though.
> 2. remove this method, but this would break API
> 
> One of these should happen.

I thought about removing of legacy member and setting 
fileSystem.reset(new DefaultFileSystem(val));

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/FileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:52: std::ostream& data) = 0;
What do you think about making it in a separate commit?
I think that maybe we should also switch to buffers even here.

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef
std::function<void(std::vector<char>&&,
Since it's for bytes, I would rather prefer an unsigned type like uint8_t or
`unsigned char` than char which takes values from -128 to 127.
BTW, what about having a typedef for the buffer?

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp#new...
src/JsValue.cpp:130: std::vector<char> AdblockPlus::JsValue::AsBuffer() const
Since there is a buffer type in JS, what about calling the methods slightly
differently to avoid confusion, e.g. `GetStringBuffer` and `SetStringProperty`?

[hub, 2017-07-06 14:33:32]

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/FileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:52: std::ostream& data) = 0;
On 2017/07/06 14:14:04, sergei wrote:
> What do you think about making it in a separate commit?
> I think that maybe we should also switch to buffers even here.

This is needed because we write to that stream buffer before passing it here.
There is no write in std::basic_istream<>.

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef
std::function<void(std::vector<char>&&,
On 2017/07/06 14:14:05, sergei wrote:
> Since it's for bytes, I would rather prefer an unsigned type like uint8_t or
> `unsigned char` than char which takes values from -128 to 127.
> BTW, what about having a typedef for the buffer?

But iostream and making it harder. If I use std::vector<unsigned char> I need to
use basic_istream<unsigned char> which requires to specialize a
std::char_traits<> for it, etc. Unless I cast, etc.

I thought this was the best way to keep it simple.

I can typedef the buffer though.

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp#new...
src/JsValue.cpp:130: std::vector<char> AdblockPlus::JsValue::AsBuffer() const
On 2017/07/06 14:14:05, sergei wrote:
> Since there is a buffer type in JS, what about calling the methods slightly
> differently to avoid confusion, e.g. `GetStringBuffer` and
`SetStringProperty`?

I'm ok with AsStringBuffer() like we have AsString().

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp#new...
src/JsValue.cpp:212: void AdblockPlus::JsValue::SetProperty(const std::string&
name, const std::vector<char>& val)
I'll rename it to SetStringProperty then.

[sergei, 2017-07-06 14:53:35]

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/FileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:52: std::ostream& data) = 0;
On 2017/07/06 14:33:31, hub wrote:
> On 2017/07/06 14:14:04, sergei wrote:
> > What do you think about making it in a separate commit?
> > I think that maybe we should also switch to buffers even here.
> 
> This is needed because we write to that stream buffer before passing it here.
> There is no write in std::basic_istream<>.
I see, but if we changed either std::istream or std::ostream to a buffer type
now, I think it would simplify the code a bit. And I think we should actually do
it in a separate commit, it seems to be a self contained change.

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef
std::function<void(std::vector<char>&&,
On 2017/07/06 14:33:31, hub wrote:
> On 2017/07/06 14:14:05, sergei wrote:
> > Since it's for bytes, I would rather prefer an unsigned type like uint8_t or
> > `unsigned char` than char which takes values from -128 to 127.
> > BTW, what about having a typedef for the buffer?
> 
> But iostream and making it harder. If I use std::vector<unsigned char> I need
to
> use basic_istream<unsigned char> which requires to specialize a
> std::char_traits<> for it, etc. Unless I cast, etc.
> 
> I thought this was the best way to keep it simple.
> 
> I can typedef the buffer though.
I think we should simply cast to `const char*` or `char*` when it's needed, e.g.
if we work with C++ streams in the default implementation of the sync interface
then we can cast `static_cast<const std::ostream::char_type*>(data.data());` to
pass it to std::ostream::write where data is the vector of bytes and similar for
v8 conversions if it's necessary there.

[hub, 2017-07-06 21:24:30]

I will need to rebase this patch on top of
https://codereview.adblockplus.org/29481704

Eventually

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29479644/src/JsEngine.cpp#ne...
src/JsEngine.cpp:332: fileSystemLegacy = val;
On 2017/07/06 14:14:04, sergei wrote:
> On 2017/07/06 12:21:52, hub wrote:
> > On 2017/07/05 10:03:21, sergei wrote:
> > > BTW, this `fileSystemLegacy` member is not used, so if the user sets it it
> > will
> > > have no effect. It seems the best option is to create async implementation
> > with
> > > injected `val`.
> > 
> > We don't have a visibility of what filesystem has been created so we can't
> > simply recreate it. 
> > 
> > The few alternative I see are:
> > 1. Add a method to IFileSystem to do this. We might need to address a few
> issues
> > in our own test though.
> > 2. remove this method, but this would break API
> > 
> > One of these should happen.
> 
> I thought about removing of legacy member and setting 
> fileSystem.reset(new DefaultFileSystem(val));

Done.

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/FileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/FileSystem.h:52: std::ostream& data) = 0;
On 2017/07/06 14:53:33, sergei wrote:
> On 2017/07/06 14:33:31, hub wrote:
> > On 2017/07/06 14:14:04, sergei wrote:
> > > What do you think about making it in a separate commit?
> > > I think that maybe we should also switch to buffers even here.
> > 
> > This is needed because we write to that stream buffer before passing it
here.
> > There is no write in std::basic_istream<>.
> I see, but if we changed either std::istream or std::ostream to a buffer type
> now, I think it would simplify the code a bit. And I think we should actually
do
> it in a separate commit, it seems to be a self contained change.

See https://codereview.adblockplus.org/29481704

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
File include/AdblockPlus/IFileSystem.h (right):

https://codereview.adblockplus.org/29449592/diff/29481630/include/AdblockPlus...
include/AdblockPlus/IFileSystem.h:82: typedef
std::function<void(std::vector<char>&&,
On 2017/07/06 14:53:33, sergei wrote:
> On 2017/07/06 14:33:31, hub wrote:
> > On 2017/07/06 14:14:05, sergei wrote:
> > > Since it's for bytes, I would rather prefer an unsigned type like uint8_t
or
> > > `unsigned char` than char which takes values from -128 to 127.
> > > BTW, what about having a typedef for the buffer?
> > 
> > But iostream and making it harder. If I use std::vector<unsigned char> I
need
> to
> > use basic_istream<unsigned char> which requires to specialize a
> > std::char_traits<> for it, etc. Unless I cast, etc.
> > 
> > I thought this was the best way to keep it simple.
> > 
> > I can typedef the buffer though.
> I think we should simply cast to `const char*` or `char*` when it's needed,
e.g.
> if we work with C++ streams in the default implementation of the sync
interface
> then we can cast `static_cast<const std::ostream::char_type*>(data.data());`
to
> pass it to std::ostream::write where data is the vector of bytes and similar
for
> v8 conversions if it's necessary there.

fair enough.

[hub, 2017-07-06 22:43:03]

Now it requires https://codereview.adblockplus.org/29481704

[sergei, 2017-07-07 13:29:28]

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp#new...
src/JsValue.cpp:212: void AdblockPlus::JsValue::SetProperty(const std::string&
name, const std::vector<char>& val)
On 2017/07/06 14:33:32, hub wrote:
> I'll rename it to SetStringProperty then.

Could you please rename it?

[hub, 2017-07-07 13:39:20]

updated patch

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29481630/src/JsValue.cpp#new...
src/JsValue.cpp:212: void AdblockPlus::JsValue::SetProperty(const std::string&
name, const std::vector<char>& val)
On 2017/07/07 13:29:27, sergei wrote:
> On 2017/07/06 14:33:32, hub wrote:
> > I'll rename it to SetStringProperty then.
> 
> Could you please rename it?

Sorry, I had the change shelved by mistake. Current patch has it.

[sergei, 2017-07-07 13:41:02]

LGTM

https://codereview.adblockplus.org/29449592/diff/29481725/src/DefaultFileSyst...
File src/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/29449592/diff/29481725/src/DefaultFileSyst...
src/DefaultFileSystem.cpp:72: DefaultFileSystemSync::Read(const std::string&
path) const
It's actually not our coding style, but I think it's fine. I would even like to
have such coding style (return value on a separate line for a function
implementation).