Issue 5450 - introduce the Platform class

include/AdblockPlus/JsEngine.h

Index: include/AdblockPlus/JsEngine.h
diff --git a/include/AdblockPlus/JsEngine.h b/include/AdblockPlus/JsEngine.h
index fbce51f93de65480b392e552ce13f317c7cc3373..4820ffc647c6ca7d535c3af02dafefd4bf8eb8d8 100644
--- a/include/AdblockPlus/JsEngine.h
+++ b/include/AdblockPlus/JsEngine.h
@@ -45,6 +45,7 @@ namespace v8
 namespace AdblockPlus
 {
   class JsEngine;
+  class Platform;
 
   /**
    * Shared smart pointer to a `JsEngine` instance.
@@ -52,26 +53,6 @@ namespace AdblockPlus
   typedef std::shared_ptr<JsEngine> JsEnginePtr;
 
   /**
-   * A factory to construct DefaultTimer.
-   */
-  TimerPtr CreateDefaultTimer();
-
-  /**
-   * A factory to construct DefaultFileSystem.
-   */
-  FileSystemPtr CreateDefaultFileSystem(const Scheduler& scheduler);
-
-  /**
-   * A factory to construct DefaultWebRequest.
-   */
-  WebRequestPtr CreateDefaultWebRequest(const Scheduler& scheduler);
-
-  /**
-   * A factory to construct LogSystem.
-   */
-  LogSystemPtr CreateDefaultLogSystem();
-
-  /**
    * Scope based isolate manager. Creates a new isolate instance on
    * constructing and disposes it on destructing.
    */
@@ -129,20 +110,12 @@ namespace AdblockPlus
     /**
      * Creates a new JavaScript engine instance.
      *
-     * When a parameter value is nullptr the corresponding default
-     * implementation is chosen.
-     *
      * @param appInfo Information about the app.
-     * @param timer Implementation of timer.
-     * @param fileSystem Implementation of filesystem.
-     * @param webRequest Implementation of web request.
-     * @param logSystem Implementation of log system.
+     * @param platform AdblockPlus platform providing with necessary
+     *        dependencies.
      * @return New `JsEngine` instance.
      */
-    static JsEnginePtr New(const AppInfo& appInfo = AppInfo(),
-      TimerPtr timer = nullptr, FileSystemPtr fileSystem = nullptr,
-      WebRequestPtr webRequest = nullptr, LogSystemPtr logSystem = nullptr);
-
+    static JsEnginePtr New(const AppInfo& appInfo, Platform& platform);
     /**
      * Registers the callback function for an event.
      * @param eventName Event name. Note that this can be any string - it's a
@@ -272,18 +245,6 @@ namespace AdblockPlus
     JsValueList ConvertArguments(const v8::FunctionCallbackInfo<v8::Value>& arguments);
 
     /**
-     * Private functionality.
-     * @return The asynchronous IFileSystem implementation.
-     */
-    FileSystemPtr GetAsyncFileSystem() const;
-
-    /**
-     * Private functionality.
-     * @return The LogSystem implementation.
-     */
-    LogSystem& GetLogSystem();
-
-    /**
      * Sets a global property that can be accessed by all the scripts.
      * @param name Name of the property to set.
      * @param value Value of the property to set.
@@ -303,26 +264,31 @@ namespace AdblockPlus
      * garbage collection.
      */
     void NotifyLowMemory();
+
+    /**
+     * Private functionality.
+     */
+    Platform& GetPlatform()
+    {
+      return platform;
+    }
   private:
     void CallTimerTask(const JsWeakValuesID& timerParamsID);
 
-    explicit JsEngine(TimerPtr timer, FileSystemPtr fileSystem, WebRequestPtr webRequest, LogSystemPtr logSystem);
+    explicit JsEngine(Platform& platform);
 
     JsValue GetGlobalObject();
 
     /// Isolate must be disposed only after disposing of all objects which are
     /// using it.
     ScopedV8Isolate isolate;
+    Platform& platform;

[hub, 2017/07/28 14:29:03]

The problem I see here is that there is no guarantee that platform (a reference)
will never be dangling since there is no guarantee that the lifetime of platform
be longer than the JsEngine.

[sergei, 2017/07/28 15:10:29]

Right but it should be the responsibility of a user, the Platform class should
be destroyed only after releasing of all references to all modules (JsEngine,
FilterEngine, etc) from a user side. In order to better communicate it through
API I think we could return "JsEngine&" from `GetJsEngine` (a reference) instead
of a shared_ptr and maybe don't allow to freely construct JsEngine (only
Platform should be able to do it), but I think it should be done after moving of
FilterEngine into Platform.
Generally speaking the Platform class should become a parent class for its
modules. One of the reasons for it is that it's impossible to have a "self
waiting" class with internal asynchronous operations
(https://issues.adblockplus.org/ticket/5198) and pass a shared_ptr to this class
around, so either this Platform will wait for finishing of currently running
async operations or a user has to call some TearDown method. I think that it
would be better to put the call of such TearDown method into the destructor of
the Platform class, so a user should not remember about it and thus cannot
forget it. Yet one reasons is that actually there is already yet one similar
case (https://issues.adblockplus.org/ticket/4693) and that Platform class should
ensure that no async operation is running and safely destroy all modules.

BTW, the graceful shutting down seems requires to think a bit more
(https://issues.adblockplus.org/ticket/5118) because now I see that my comment
to #5118 is actually incorrect :).

[hub, 2017/07/28 18:16:38]

No. It is the responsibility of the API to make it sane and prevent that kind of
mistake.
This could help, yes, as it would keep the ownership of JsEngine into Platform.

[sergei, 2017/07/28 19:16:50]

But we cannot do it right now because FilterEngine is not moved yet and requires
a shared_ptr. I have added it into the issue description to don't forget about
it.

 
-    FileSystemPtr fileSystem;
-    LogSystemPtr logSystem;
     std::unique_ptr<v8::Global<v8::Context>> context;
     EventMap eventCallbacks;
     std::mutex eventCallbacksMutex;
     JsWeakValuesLists jsWeakValuesLists;
     std::mutex jsWeakValuesListsMutex;
-    TimerPtr timer;
-    WebRequestPtr webRequest;
   };
 }