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

test/BaseJsTest.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 90 matching lines @@
   void operator()(LogLevel logLevel, const std::string& message,
         const std::string& source)
   {
     throw std::runtime_error("Unexpected error: " + message);
   }
 };
 
 class ThrowingFileSystem : public AdblockPlus::IFileSystem
 {
 public:
-  void Read(const std::string& fileName, const ReadCallback& callback) const override
+  void Read(const std::string& fileName, const ReadCallback& callback, const Callback& errorCallback) const override
   {
     throw std::runtime_error("Not implemented");
   }
 
   void Write(const std::string& fileName, const IOBuffer& data,
     const Callback& callback) override
   {
     throw std::runtime_error("Not implemented");
   }
 
@@ skipping 31 matching lines @@
   static void ExecuteImmediately(const Task& task)
   {
     if (task)
       task();
   }
   explicit LazyFileSystem(const Scheduler& scheduler = LazyFileSystem::ExecuteImmediately)
     : scheduler(scheduler)
   {
   }
 
-  void Read(const std::string& fileName, const ReadCallback& callback) const override
+  void Read(const std::string& fileName, const ReadCallback& callback, const Callback& errorCallback) const override
   {
-    scheduler([fileName, callback]
+    scheduler([fileName, callback, errorCallback]
     {

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

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.

       if (fileName == "patterns.ini")
       {
         std::string dummyData = "# Adblock Plus preferences\n[Subscription]\nurl=~user~0000";
-        callback(IOBuffer(dummyData.cbegin(), dummyData.cend()), "");
+        callback(IOBuffer(dummyData.cbegin(), dummyData.cend()));
       }
       else if (fileName == "prefs.json")
       {
         std::string dummyData = "{}";
-        callback(IOBuffer(dummyData.cbegin(), dummyData.cend()), "");
+        callback(IOBuffer(dummyData.cbegin(), dummyData.cend()));
       }
+      else
+        errorCallback("File not found, " + fileName);
     });
   }
 
   void Write(const std::string& fileName, const IOBuffer& data,
              const Callback& callback) override
   {
   }
 
 
   void Move(const std::string& fromFileName, const std::string& toFileName,
@@ skipping 19 matching lines @@
   }
 public:
   Scheduler scheduler;
 };
 
 class InMemoryFileSystem : public LazyFileSystem
 {
   std::map<std::string, IOBuffer> files;
 public:
   using LazyFileSystem::LazyFileSystem;
-  void Read(const std::string& fileName, const ReadCallback& callback) const override
+  void Read(const std::string& fileName, const ReadCallback& callback, const Callback& errorCallback) const override
   {
-    scheduler([this, fileName, callback]()
+    scheduler([this, fileName, callback, errorCallback]()
     {
       auto ii_file = files.find(fileName);
-      if (ii_file == files.end())
-      {
-        callback(IOBuffer(), "File not found, " + fileName);
-        return;
-      }
-      callback(IOBuffer(ii_file->second), "");
+      if (ii_file != files.end())
+        callback(IOBuffer(ii_file->second));
+      else
+        errorCallback("File not found, " + fileName);
     });
   }
 
   void Write(const std::string& fileName, const IOBuffer& data,
     const Callback& callback) override
   {
     scheduler([this, fileName, data, callback]()
     {
       files[fileName] = data;
       callback("");
@@ skipping 104 matching lines @@
   }
 
   void TearDown() override
   {
     if (platform)
       platform.reset();
   }
 };
 
 #endif