Issue 5183 - Provide async interface for FileSystem

include/AdblockPlus/IFileSystem.h

Index: include/AdblockPlus/IFileSystem.h
===================================================================
copy from include/AdblockPlus/FileSystem.h
copy to include/AdblockPlus/IFileSystem.h
--- a/include/AdblockPlus/FileSystem.h
+++ b/include/AdblockPlus/IFileSystem.h
@@ -10,30 +10,31 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef ADBLOCK_PLUS_FILE_SYSTEM_H
-#define ADBLOCK_PLUS_FILE_SYSTEM_H
+#ifndef ADBLOCK_PLUS_IFILE_SYSTEM_H
+#define ADBLOCK_PLUS_IFILE_SYSTEM_H
 
 #include <istream>
 #include <stdint.h>
 #include <string>
 #include <memory>
+#include <vector>
 
 namespace AdblockPlus
 {
   /**
    * File system interface.
    */
-  class FileSystem
+  class IFileSystem
   {
   public:
     /**
      * Result of a stat operation, i.e.\ information about a file.
      */
     struct StatResult
     {
       StatResult()
@@ -60,62 +61,87 @@
       bool isFile;
 
       /**
        * POSIX time of the last modification.
        */
       int64_t lastModified;
     };
 
-    virtual ~FileSystem() {}
+    virtual ~IFileSystem() {}
+
+    /**
+     * Default callback type for asynchronous filesystem calls.
+     * @param An error string. Empty is success.
+     */
+    typedef std::function<void(const std::string&)> Callback;
+
+    /**
+     * Callback type for the asynchronous Read call.
+     * @param Output char array with file content.
+     * @param An error string. Empty if success.
+     */
+    typedef std::function<void(std::string&&, const std::string&)> ReadCallback;

[sergei, 2017/06/16 15:05:54]

I think that the data type for the read data should be std::vector<uint8_t>
instead of std::string.

[hub, 2017/06/16 21:52:53]

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

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

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?

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

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:20]

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

 
     /**
      * Reads from a file.
      * @param path File path.
-     * @return Input stream with the file's contents.
+     * @param callback The function called on completion with the input data.
      */
-    virtual std::shared_ptr<std::istream>
-      Read(const std::string& path) const = 0;
+    virtual void Read(const std::string& path,
+                      const ReadCallback& callback) const = 0;
 
     /**
      * Writes to a file.
      * @param path File path.
      * @param data Input stream with the data to write.
+     * @param callback The function called on completion.
      */
     virtual void Write(const std::string& path,
-                       std::istream& data) = 0;
+                       std::shared_ptr<std::istream> data,

[sergei, 2017/06/16 15:05:54]

Instead of `std::shared_ptr<std::istream>` it would be better to use
std::vector<uint8_t>.

[hub, 2017/06/16 21:52:54]

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

+                       const Callback& callback) = 0;
 
     /**
      * Moves a file (i.e.\ renames it).
      * @param fromPath Current path to the file.
      * @param toPath New path to the file.
+     * @param callback The function called on completion.
      */
-    virtual void Move(const std::string& fromPath,
-                      const std::string& toPath) = 0;
+    virtual void Move(const std::string& fromPath, const std::string& toPath,
+                      const Callback& callback) = 0;
 
     /**
      * Removes a file.
      * @param path File path.
+     * @param callback The function called on completion.
      */
-    virtual void Remove(const std::string& path) = 0;
+    virtual void Remove(const std::string& path, const Callback& callback) = 0;
+
+    /**
+     * Callback type for the asynchronous Stat call.
+     * @param the StatResult data.
+     * @param an error string. Empty if no error.
+     */
+    typedef std::function<void(const StatResult&, const std::string&)> StatCallback;
 
     /**
      * Retrieves information about a file.
      * @param path File path.
-     * @return File information.
+     * @param callback The function called on completion.
      */
-    virtual StatResult Stat(const std::string& path) const = 0;
+    virtual void Stat(const std::string& path,
+                      const StatCallback& callback) const = 0;
 
     /**
      * Returns the absolute path to a file.
      * @param path File path (can be relative or absolute).
      * @return Absolute file path.
      */
     virtual std::string Resolve(const std::string& path) const = 0;
   };
 
   /**
-   * Shared smart pointer to a `FileSystem` instance.
+   * Shared smart pointer to a `IFileSystem` instance.
    */
-  typedef std::shared_ptr<FileSystem> FileSystemPtr;
+  typedef std::shared_ptr<IFileSystem> FileSystemPtr;

[sergei, 2017/06/16 15:05:54]

OK now, but in future I think it should be std::unique_ptr.

[hub, 2017/06/16 21:52:53]

Acknowledged.

 }
 
 #endif