WIP: FilterEngine and Updater JS api separation

Moved Updater class to dedicated header and source files.

Created apiUpdater.js with a separate API for Updater component.

Issues to fix:

1. Pass Prefs to Updater component
- Now Updater sets just an empty Prefs object

2. Review content of apiUpdater.js and updaterJsFiles[] array
- Check that all what's needed is set there and nothing is redundant
- Does Updater component need to take care of asynchronicity in JS
files?

3. Fix UpdateCheckTests
- They are now passing just because FilterEngine object is created
while those tests should just rely only on Updater object.

Author: kzlomek

include/AdblockPlus.h

@@ -32,6 +32,7 @@
 
 #include <AdblockPlus/AppInfo.h>
 #include <AdblockPlus/FilterEngine.h>
+#include <AdblockPlus/Updater.h>
 #include <AdblockPlus/LogSystem.h>
 #include <AdblockPlus/JsEngine.h>
 #include <AdblockPlus/JsValue.h>

include/AdblockPlus/FilterEngine.h

@@ -534,57 +534,6 @@ namespace AdblockPlus
       ContentTypeMask contentTypeMask,
       const std::vector<std::string>& documentUrls) const;
   };
-
-  /**
-   * Component of libadblockplus responsible for Update checks for the application.
-   */
-  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.
-     */
-    typedef std::function<void(const std::string&)> UpdateAvailableCallback;
-
-    /**
-     * Callback type invoked when a manually triggered update check finishes.
-     * The parameter is an optional error message.
-     */
-    typedef std::function<void(const std::string&)> UpdateCheckDoneCallback;
-
-    /**
-     * Sets the callback invoked when an application update becomes available.
-     * @param callback Callback to invoke.
-     */
-    void SetUpdateAvailableCallback(const UpdateAvailableCallback& callback);
-
-    /**
-     * Removes the callback invoked when an application update becomes
-     * available.
-     */
-    void RemoveUpdateAvailableCallback();
-
-    /**
-     * Forces an immediate update check.
-     * `Updater` will automatically check for updates in regular intervals,
-     * so applications should only call this when the user triggers an update
-     * check manually.
-     * @param callback Optional callback to invoke when the update check is
-     *        finished. The string parameter will be empty when the update check
-     *        succeeded, or contain an error message if it failed.
-     *        Note that the callback will be invoked whether updates are
-     *        available or not - to react to updates being available, use
-     *        `Updater::SetUpdateAvailableCallback()`.
-     */
-    void ForceUpdateCheck(const UpdateCheckDoneCallback& callback = UpdateCheckDoneCallback());
-
-  private:
-    JsEnginePtr jsEngine;
-    int updateCheckId;
-  };
 }
 
 #endif

include/AdblockPlus/Platform.h

@@ -25,6 +25,7 @@
 #include "AppInfo.h"
 #include "Scheduler.h"
 #include "FilterEngine.h"
+#include "Updater.h"
 #include <mutex>
 #include <future>
 #include <set>

include/AdblockPlus/Updater.h

@@ -0,0 +1,79 @@
+/*
+ * 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
+ * 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_UPDATER_H
+#define ADBLOCK_PLUS_UPDATER_H
+
+#include <functional>
+#include <string>
+#include <AdblockPlus/JsEngine.h>
+
+namespace AdblockPlus
+{
+  /**
+   * Component of libadblockplus responsible for Update checks for the application.
+   */
+  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.
+     */
+    typedef std::function<void(const std::string&)> UpdateAvailableCallback;
+
+    /**
+     * Callback type invoked when a manually triggered update check finishes.
+     * The parameter is an optional error message.
+     */
+    typedef std::function<void(const std::string&)> UpdateCheckDoneCallback;
+
+    /**
+     * Sets the callback invoked when an application update becomes available.
+     * @param callback Callback to invoke.
+     */
+    void SetUpdateAvailableCallback(const UpdateAvailableCallback& callback);
+
+    /**
+     * Removes the callback invoked when an application update becomes
+     * available.
+     */
+    void RemoveUpdateAvailableCallback();
+
+    /**
+     * Forces an immediate update check.
+     * `Updater` will automatically check for updates in regular intervals,
+     * so applications should only call this when the user triggers an update
+     * check manually.
+     * @param callback Optional callback to invoke when the update check is
+     *        finished. The string parameter will be empty when the update check
+     *        succeeded, or contain an error message if it failed.
+     *        Note that the callback will be invoked whether updates are
+     *        available or not - to react to updates being available, use
+     *        `Updater::SetUpdateAvailableCallback()`.
+     */
+    void ForceUpdateCheck(const UpdateCheckDoneCallback& callback = UpdateCheckDoneCallback());
+
+  private:
+    JsEnginePtr jsEngine;
+    int updateCheckId;
+  };
+}
+
+#endif

lib/api.js

@@ -28,7 +28,6 @@ let API = (() =>
   const {ElemHide} = require("elemHide");
   const {Synchronizer} = require("synchronizer");
   const {Prefs} = require("prefs");
-  const {checkForUpdates} = require("updater");
   const {Notification} = require("notification");
 
   return {
@@ -188,6 +187,7 @@ let API = (() =>
     {
       Notification.markAsShown(id);
     },
+
     checkFilterMatch(url, contentTypeMask, documentUrl)
     {
       let requestHost = extractHostFromURL(url);
@@ -213,11 +213,6 @@ let API = (() =>
       Prefs[pref] = value;
     },
 
-    forceUpdateCheck(eventName)
-    {
-      checkForUpdates(eventName ? _triggerEvent.bind(null, eventName) : null);
-    },
-
     getHostFromUrl(url)
     {
       return extractHostFromURL(url);

lib/apiUpdater.js

@@ -0,0 +1,52 @@
+/*
+ * 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
+ * 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/>.
+ */
+
+"use strict";
+
+let API_UPDATER = (() =>
+{
+  const {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
+  const {Prefs} = require("prefs");
+  const {checkForUpdates} = require("updater");
+
+  return {
+    getPref(pref)
+    {
+      return Prefs[pref];
+    },
+
+    setPref(pref, value)
+    {
+      Prefs[pref] = value;
+    },
+
+    forceUpdateCheck(eventName)
+    {
+      checkForUpdates(eventName ? _triggerEvent.bind(null, eventName) : null);
+    },
+
+    getHostFromUrl(url)
+    {
+      return extractHostFromURL(url);
+    },
+
+    compareVersions(v1, v2)
+    {
+      return Services.vc.compare(v1, v2);
+    }
+  };
+})();

libadblockplus.gyp

@@ -44,6 +44,7 @@
       'src/DefaultWebRequest.cpp',
       'src/FileSystemJsObject.cpp',
       'src/FilterEngine.cpp',
+      'src/Updater.cpp',
       'src/GlobalJsObject.cpp',
       'src/JsContext.cpp',
       'src/JsEngine.cpp',
@@ -159,6 +160,7 @@
         ],
         'load_after_files': [
           'lib/api.js',
+          'lib/apiUpdater.js',
           'lib/publicSuffixList.js',
           'lib/punycode.js',
           'lib/basedomain.js',

src/FilterEngine.cpp

@@ -23,23 +23,15 @@
 #include <thread>
 
 #include <AdblockPlus.h>
+#include "Utils.h"
 #include "JsContext.h"
 #include "Thread.h"
 #include <mutex>
 #include <condition_variable>
 
-#define ArraySize(a) (sizeof(a) / sizeof(a[0]))
-
 using namespace AdblockPlus;
 
 namespace {
-
-    /*
-     * TODO: Clarify JS dependencies for FilterEngine and Updater
-     * so both are using only required JS files.
-     * Now both tables are the same with full list of JS files.
-     */
-
     static std::string filterEngineJsFiles[] = {
       "compat.js",
       "info.js",
@@ -65,39 +57,6 @@ namespace {
       "synchronizer.js",
       "filterUpdateRegistration.js",
       "subscriptions.xml",
-      "updater.js",
-      "api.js",
-      "publicSuffixList.js",
-      "punycode.js",
-      "basedomain.js"
-    };
-
-    static std::string updaterJsFiles[] = {
-      "compat.js",
-      "info.js",
-      "io.js",
-      "prefs.js",
-      "utils.js",
-      "elemHideHitRegistration.js",
-      "events.js",
-      "coreUtils.js",
-      "filterNotifier.js",
-      "init.js",
-      "common.js",
-      "filterClasses.js",
-      "subscriptionClasses.js",
-      "filterStorage.js",
-      "elemHide.js",
-      "elemHideEmulation.js",
-      "matcher.js",
-      "filterListener.js",
-      "downloader.js",
-      "notification.js",
-      "notificationShowRegistration.js",
-      "synchronizer.js",
-      "filterUpdateRegistration.js",
-      "subscriptions.xml",
-      "updater.js",
       "api.js",
       "publicSuffixList.js",
       "punycode.js",
@@ -640,44 +599,3 @@ FilterPtr FilterEngine::GetWhitelistingFilter(const std::string& url,
   while (urlIterator != documentUrls.end());
   return FilterPtr();
 }
-
-
-Updater::Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& evaluateCallback)
-  : jsEngine(jsEngine), updateCheckId(0)
-{
-  // Load adblockplus scripts
-  for(size_t i = 0; i < ArraySize(updaterJsFiles); ++i)
-    evaluateCallback(updaterJsFiles[i]);
-}
-
-void Updater::SetUpdateAvailableCallback(const Updater::UpdateAvailableCallback& callback)
-{
-  jsEngine->SetEventCallback("updateAvailable", [this, callback](JsValueList&& params)
-  {
-    if (params.size() >= 1 && !params[0].IsNull())
-      callback(params[0].AsString());
-  });
-}
-
-void Updater::RemoveUpdateAvailableCallback()
-{
-  jsEngine->RemoveEventCallback("updateAvailable");
-}
-
-void Updater::ForceUpdateCheck(const Updater::UpdateCheckDoneCallback& callback)
-{
-  JsValue func = jsEngine->Evaluate("API.forceUpdateCheck");
-  JsValueList params;
-  if (callback)
-  {
-    std::string eventName = "_updateCheckDone" + std::to_string(++updateCheckId);
-    jsEngine->SetEventCallback(eventName, [this, eventName, callback](JsValueList&& params)
-    {
-      std::string error(params.size() >= 1 && !params[0].IsNull() ? params[0].AsString() : "");
-      callback(error);
-      jsEngine->RemoveEventCallback(eventName);
-    });
-    params.push_back(jsEngine->NewValue(eventName));
-  }
-  func.Call(params);
-}