Issue 5450 - introduce the Platform class

include/AdblockPlus/JsEngine.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 27 matching lines @@
   class Isolate;
   class Value;
   class Context;
   template<typename T> class FunctionCallbackInfo;
   typedef void(*FunctionCallback)(const FunctionCallbackInfo<v8::Value>& info);
 }
 
 namespace AdblockPlus
 {
   class JsEngine;
+  class Platform;
 
   /**
    * Shared smart pointer to a `JsEngine` instance.
    */
   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.
    */
   class ScopedV8Isolate
   {
   public:
     ScopedV8Isolate();
     ~ScopedV8Isolate();
     v8::Isolate* Get()
     {
@@ skipping 37 matching lines @@
      */
     class JsWeakValuesID
     {
       friend class JsEngine;
       JsWeakValuesLists::const_iterator iterator;
     };
 
     /**
      * 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
      *        general purpose event handling mechanism.
      * @param callback Event callback function.
      */
     void SetEventCallback(const std::string& eventName, const EventCallback& callback);
 
     /**
      * Removes the callback function for an event.
@@ skipping 109 matching lines @@
 
     /**
      * Converts v8 arguments to `JsValue` objects.
      * @param arguments `v8::FunctionCallbackInfo` object containing the arguments to
      *        convert.
      * @return List of arguments converted to `const JsValue` objects.
      */
     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.
      */
     void SetGlobalProperty(const std::string& name, const AdblockPlus::JsValue& value);
 
     /**
      * Returns a pointer to associated v8::Isolate.
      */
     v8::Isolate* GetIsolate()
     {
       return isolate.Get();
     }
 
     /**
      * Notifies JS engine about critically low memory what should cause a
      * 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;
   };
 }
 
 #endif