Issue 6477 - separate done and error callbacks in IFileSystem::Read

[sergei, 2018-03-23 11:27:19]

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

https://codereview.adblockplus.org/29731562/diff/29731589/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:35: namespace ReadCallback
the changes here are:
- create the namespace `ReadCallback`
- move the previously called `ReadCallback` function into the namespace and call
it V8Callback.
- add WeakData which is removing the stored JS values from JsEngine. It's a
first attempt to address #6517, however here it's not generic and it's almost
the same what was here before. As soon as there are several (3-5) such WeakData
structures they will be generalized and committed as #6517.
- now the JS callback function requires 3 arguments, previously it required 2.
- the call of fileSystem.Read is adjusted in accordance with the changes of the
interface.

https://codereview.adblockplus.org/29731562/diff/29731589/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:105: namespace ReadFromFileCallback
the changes here are:
- create the namespace `ReadFromFileCallback`
- move the previously called `ReadFromFileCallback` function into the namespace
and call it V8Callback.
- move other related helper functions to the namespace too.
- add WeakData which is removing the stored JS values from JsEngine. See above
regarding #6517.
- now the JS callback function requires 4 arguments, previously it required 3.
- the call of fileSystem.Read is adjusted in accordance with the changes of the
interface.

https://codereview.adblockplus.org/29731562/diff/29731589/src/FileSystemJsObj...
src/FileSystemJsObject.cpp:191: throw JsError(tryCatch.Exception(),
tryCatch.Message());
Please pay attention to this change. Previously it was

if (tryCatch.HasCaught())	
{	
 
jsValues[1].Call(jsEngine->NewValue(JsError::ExceptionToString(tryCatch.Exception(),
tryCatch.Message())));	
  return;
}

now it's

if (tryCatch.HasCaught())
  throw JsError(tryCatch.Exception(), tryCatch.Message());

I find it deserving a comment here because it looks like the changing of the
approach, namely it seems that we switched to the exception-based execution
flow, however it's not completely correct. One should be ready that an exception
can be thrown by any method of either JsValue or JsEngine, in particular, when
one calls `jsValues[1].Call();` (see in the old code) it throws the same
exception, here we throw the exception manually because `v8::Function` is not
wrapped by JsValue, so IMO it's not a switching to the exception-based flow,
it's rather a unifying of the approach.

https://codereview.adblockplus.org/29731562/diff/29731589/test/BaseJsTest.h
File test/BaseJsTest.h (right):

https://codereview.adblockplus.org/29731562/diff/29731589/test/BaseJsTest.h#n...
test/BaseJsTest.h:166: {
Strictly speaking, now in accordance with the interface one should catch
exceptions and call errorCallback if there is some, e.g. see
https://codereview.adblockplus.org/29731562/diff/29731589/test/FileSystemJsOb...,
MockFileSystem::Read.

I left it without try-catch because it's not supposed that anything results in
an exception here, so it seems rather a dead-code, so IMO it should be rather
simple and one should find out what is happening earlier (because of less
conditions) in tests. Please let me know if you think that every implementation
of IFileSystem::Read, including all tests, should contain that try-catch, namely
calling errorCallback if there is an exception.

[hub, 2018-03-23 14:55:52]

LGTM