Addressed review comments

Apart from changes in new code, this also withdraws
changes in existing code not related with this feature.

Author: kzlomek

include/AdblockPlus/FilterEngine.h

@@ -274,6 +274,12 @@ namespace AdblockPlus
      */
     typedef std::function<void(const FilterEnginePtr&)> OnCreatedCallback;
 
+    /**
+     * Callback type for evaluating JS expression.
+     * The parameter is the JS file name containing the expression.
+     */
+    typedef std::function<void(const std::string&)> EvaluateCallback;
+
     /**
      * Asynchronously constructs FilterEngine.
      * @param jsEngine `JsEngine` instance used to run JavaScript code
@@ -283,7 +289,7 @@ namespace AdblockPlus
      * @param parameters optional creation parameters.
      */
     static void CreateAsync(const JsEnginePtr& jsEngine,
-      const JsEngine::EvaluateCallback& evaluateCallback,
+      const EvaluateCallback& evaluateCallback,
       const OnCreatedCallback& onCreated,
       const CreationParameters& parameters = CreationParameters());
 

include/AdblockPlus/JsEngine.h

@@ -143,12 +143,6 @@ namespace AdblockPlus
     JsValue Evaluate(const std::string& source,
         const std::string& filename = "");
 
-    /**
-     * Callback type for evaluating JS expression.
-     * The parameter is the JS file name containing the expression.
-     */
-    typedef std::function<void(const std::string&)> EvaluateCallback;
-
     /**
      * Initiates a garbage collection.
      */

include/AdblockPlus/Platform.h

@@ -30,6 +30,7 @@
 #include <future>
 #include <set>
 #include <string>
+#include <functional>
 
 namespace AdblockPlus
 {
@@ -133,15 +134,14 @@ namespace AdblockPlus
     TimerPtr timer;
     FileSystemPtr fileSystem;
     WebRequestPtr webRequest;
-    JsEngine::EvaluateCallback evaluateCallback;
   private:
     // used for creation and deletion of modules.
     std::mutex modulesMutex;
     std::shared_ptr<JsEngine> jsEngine;
     std::shared_future<FilterEnginePtr> filterEngine;
     std::shared_ptr<Updater> updater;
     std::set<std::string> evaluatedJsSources;
-    JsEngine::EvaluateCallback GetEvaluateCallback();
+    std::function<void(const std::string&)> GetEvaluateCallback();
   };
 
   /**

include/AdblockPlus/Updater.h

@@ -31,8 +31,6 @@ namespace AdblockPlus
   class Updater
   {
   public:
-    explicit Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& callback);
-
     /**
      * Callback type invoked when an update becomes available.
      * The parameter is the download URL of the update.
@@ -57,6 +55,12 @@ namespace AdblockPlus
      */
     void RemoveUpdateAvailableCallback();
 
+    /**
+     * Callback type for evaluating JS expression.
+     * The parameter is the JS file name containing the expression.
+     */
+    typedef std::function<void(const std::string&)> EvaluateCallback;
+
     /**
      * Forces an immediate update check.
      * `Updater` will automatically check for updates in regular intervals,
@@ -85,6 +89,8 @@ namespace AdblockPlus
      */
     void SetPref(const std::string& pref, const JsValue& value);
 
+    explicit Updater(const JsEnginePtr& jsEngine, const EvaluateCallback& callback);
+
   private:
     JsEnginePtr jsEngine;
     int updateCheckId;

lib/apiUpdater.js

@@ -19,7 +19,6 @@
 
 let API_UPDATER = (() =>
 {
-  const {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
   const {Prefs} = require("prefs");
   const {checkForUpdates} = require("updater");
 

lib/compat.js

@@ -333,6 +333,13 @@ XMLHttpRequest.prototype =
       for (let i = 0; i < list.length; i++)
         list[i].call(this, event);
     };
+
+    if (this._url.includes("update.json"))
+    {
+      window._webRequest.GET(this._url, this._requestHeaders, onGetDone);
+      return;
+    }
+
     // HACK (#5066): the code checking whether the connection is
     // allowed is temporary, the actual check should be in the core
     // when we make a decision whether to update a subscription with

lib/prefs.js

@@ -146,10 +146,11 @@ let Prefs = exports.Prefs = {
   }
 };
 
-// Update the default prefs with what was preconfigured
-for (let key in _preconfiguredPrefs)
-  if (preconfigurable.indexOf(key) != -1)
-    defaults[key] = _preconfiguredPrefs[key];
+if (typeof _preconfiguredPrefs !== "undefined")
+  // Update the default prefs with what was preconfigured
+  for (let key in _preconfiguredPrefs)
+    if (preconfigurable.indexOf(key) != -1)
+      defaults[key] = _preconfiguredPrefs[key];
 
 // Define defaults
 for (let key in defaults)

src/FilterEngine.cpp

@@ -23,16 +23,17 @@
 #include <thread>
 
 #include <AdblockPlus.h>
-#include "Utils.h"
 #include "JsContext.h"
 #include "Thread.h"
 #include <mutex>
 #include <condition_variable>
 
 using namespace AdblockPlus;
 
-namespace {
-    static std::string filterEngineJsFiles[] = {
+namespace
+{
+    const std::string filterEngineJsFiles[] =
+    {
       "compat.js",
       "info.js",
       "io.js",
@@ -221,7 +222,7 @@ FilterEngine::FilterEngine(const JsEnginePtr& jsEngine)
 }
 
 void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine,
-  const JsEngine::EvaluateCallback& evaluateCallback,
+  const EvaluateCallback& evaluateCallback,
   const FilterEngine::OnCreatedCallback& onCreated,
   const FilterEngine::CreationParameters& params)
 {
@@ -262,7 +263,7 @@ void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine,
       isSubscriptionDownloadAllowedCallback(params[0].IsString() ? &allowedConnectionType : nullptr, callJsCallback);
     });
   }
-
+  
   jsEngine->SetEventCallback("_init", [jsEngine, filterEngine, onCreated](JsValueList&& params)
   {
     filterEngine->firstRun = params.size() && params[0].AsBool();
@@ -280,15 +281,20 @@ void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine,
       filterEngine->GetJsEngine().NotifyLowMemory();
   });
 
+  // Lock the JS engine while we are loading scripts, no timeouts should fire
+  // until we are done.
+  const JsContext context(*jsEngine);
   // Set the preconfigured prefs
   auto preconfiguredPrefsObject = jsEngine->NewObject();
   for (const auto& pref : params.preconfiguredPrefs)
+  {
     preconfiguredPrefsObject.SetProperty(pref.first, pref.second);
+  }
   jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject);
 
   // Load adblockplus scripts
-  for(size_t i = 0; i < ArraySize(filterEngineJsFiles); ++i)
-    evaluateCallback(filterEngineJsFiles[i]);
+  for (const auto& filterEngineJsFile: filterEngineJsFiles)
+    evaluateCallback(filterEngineJsFile);
 
 }
 

src/Platform.cpp

@@ -19,7 +19,6 @@
 #include <AdblockPlus/FilterEngine.h>
 #include <AdblockPlus/DefaultLogSystem.h>
 #include <AdblockPlus/AsyncExecutor.h>
-#include "JsContext.h"
 #include "DefaultTimer.h"
 #include "DefaultWebRequest.h"
 #include "DefaultFileSystem.h"
@@ -68,16 +67,14 @@ JsEngine& Platform::GetJsEngine()
   return *jsEngine;
 }
 
-JsEngine::EvaluateCallback Platform::GetEvaluateCallback()
+std::function<void(const std::string&)> Platform::GetEvaluateCallback()
 {
     // GetEvaluateCallback() method assumes that jsEngine is already created
     return [this](const std::string& filename)
     {
       if (evaluatedJsSources.find(filename) != evaluatedJsSources.end())
         return; //NO-OP, file was already evaluated
 
-      // Lock the JS engine while we are loading scripts
-      const JsContext context(*jsEngine);
       for (int i = 0; !jsSources[i].empty(); i += 2)
         if (jsSources[i] == filename)
         {

src/Updater.cpp

@@ -16,13 +16,14 @@
  */
 
 #include <AdblockPlus.h>
-#include "Utils.h"
-#include <cassert>
+#include "JsContext.h"
 
 using namespace AdblockPlus;
 
-namespace {
-    static std::string updaterJsFiles[] = {
+namespace
+{
+    const std::string updaterJsFiles[] =
+    {
       "compat.js",
       "info.js",
       "prefs.js",
@@ -36,28 +37,14 @@ namespace {
     };
 }
 
-Updater::Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& evaluateCallback)
+Updater::Updater(const JsEnginePtr& jsEngine, const EvaluateCallback& evaluateCallback)
   : jsEngine(jsEngine), updateCheckId(0)
 {
-  // Hack to enable downloads from JS (see compat.js)
-  jsEngine->SetEventCallback("_isSubscriptionDownloadAllowed", [this](JsValueList&& params)
-  {
-    // param[0] - nullable string Prefs.allowed_connection_type
-    // param[1] - function(Boolean)
-    bool areArgumentsValid = params.size() == 2 && (params[0].IsNull() || params[0].IsString()) && params[1].IsFunction();
-    assert(areArgumentsValid && "Invalid argument: there should be two args and the second one should be a function");
-    if (!areArgumentsValid)
-      return;
-    params[1].Call(this->jsEngine->NewValue(true));
-  });
-
-  // Set empty preconfigured prefs
-  auto preconfiguredPrefsObject = jsEngine->NewObject();
-  jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject);
-
-  // Load adblockplus scripts
-  for(size_t i = 0; i < ArraySize(updaterJsFiles); ++i)
-    evaluateCallback(updaterJsFiles[i]);
+  // Lock the JS engine while we are loading scripts, no timeouts should fire
+  // until we are done.
+  const JsContext context(*jsEngine);
+  for (const auto& updaterJsFile: updaterJsFiles)
+    evaluateCallback(updaterJsFile);
 }
 
 void Updater::SetUpdateAvailableCallback(const Updater::UpdateAvailableCallback& callback)