From 49f017119996ea894a8e08c5be3a2b1f2d4d9272 Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Sat, 4 Aug 2018 15:41:53 +0200 Subject: [PATCH] Addressed review comments Apart from changes in new code, this also withdraws changes in existing code not related with this feature. --- include/AdblockPlus/FilterEngine.h | 8 ++++++- include/AdblockPlus/JsEngine.h | 6 ----- include/AdblockPlus/Platform.h | 4 ++-- include/AdblockPlus/Updater.h | 10 +++++++-- lib/apiUpdater.js | 1 - lib/compat.js | 7 ++++++ lib/prefs.js | 9 ++++---- src/FilterEngine.cpp | 20 +++++++++++------ src/Platform.cpp | 5 +---- src/Updater.cpp | 35 ++++++++++-------------------- src/Utils.h | 2 -- 11 files changed, 54 insertions(+), 53 deletions(-) diff --git a/include/AdblockPlus/FilterEngine.h b/include/AdblockPlus/FilterEngine.h index 91156bcf..c7d4af48 100644 --- a/include/AdblockPlus/FilterEngine.h +++ b/include/AdblockPlus/FilterEngine.h @@ -274,6 +274,12 @@ namespace AdblockPlus */ typedef std::function OnCreatedCallback; + /** + * Callback type for evaluating JS expression. + * The parameter is the JS file name containing the expression. + */ + typedef std::function 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()); diff --git a/include/AdblockPlus/JsEngine.h b/include/AdblockPlus/JsEngine.h index cab146b1..833e0d32 100644 --- a/include/AdblockPlus/JsEngine.h +++ b/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 EvaluateCallback; - /** * Initiates a garbage collection. */ diff --git a/include/AdblockPlus/Platform.h b/include/AdblockPlus/Platform.h index c0718960..977ba24e 100644 --- a/include/AdblockPlus/Platform.h +++ b/include/AdblockPlus/Platform.h @@ -30,6 +30,7 @@ #include #include #include +#include namespace AdblockPlus { @@ -133,7 +134,6 @@ namespace AdblockPlus TimerPtr timer; FileSystemPtr fileSystem; WebRequestPtr webRequest; - JsEngine::EvaluateCallback evaluateCallback; private: // used for creation and deletion of modules. std::mutex modulesMutex; @@ -141,7 +141,7 @@ namespace AdblockPlus std::shared_future filterEngine; std::shared_ptr updater; std::set evaluatedJsSources; - JsEngine::EvaluateCallback GetEvaluateCallback(); + std::function GetEvaluateCallback(); }; /** diff --git a/include/AdblockPlus/Updater.h b/include/AdblockPlus/Updater.h index d8f0f17a..6b9593ad 100644 --- a/include/AdblockPlus/Updater.h +++ b/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 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; diff --git a/lib/apiUpdater.js b/lib/apiUpdater.js index c329af75..ba2c3d47 100644 --- a/lib/apiUpdater.js +++ b/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"); diff --git a/lib/compat.js b/lib/compat.js index a433b3b5..4c6f0606 100644 --- a/lib/compat.js +++ b/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 diff --git a/lib/prefs.js b/lib/prefs.js index b9ac2c46..fa6fd584 100644 --- a/lib/prefs.js +++ b/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) diff --git a/src/FilterEngine.cpp b/src/FilterEngine.cpp index 5220e090..ca3522ca 100644 --- a/src/FilterEngine.cpp +++ b/src/FilterEngine.cpp @@ -23,7 +23,6 @@ #include #include -#include "Utils.h" #include "JsContext.h" #include "Thread.h" #include @@ -31,8 +30,10 @@ 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); } diff --git a/src/Platform.cpp b/src/Platform.cpp index 9c2dcbe3..f24af79f 100644 --- a/src/Platform.cpp +++ b/src/Platform.cpp @@ -19,7 +19,6 @@ #include #include #include -#include "JsContext.h" #include "DefaultTimer.h" #include "DefaultWebRequest.h" #include "DefaultFileSystem.h" @@ -68,7 +67,7 @@ JsEngine& Platform::GetJsEngine() return *jsEngine; } -JsEngine::EvaluateCallback Platform::GetEvaluateCallback() +std::function Platform::GetEvaluateCallback() { // GetEvaluateCallback() method assumes that jsEngine is already created return [this](const std::string& filename) @@ -76,8 +75,6 @@ JsEngine::EvaluateCallback Platform::GetEvaluateCallback() 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) { diff --git a/src/Updater.cpp b/src/Updater.cpp index 9d96188e..c64b0881 100644 --- a/src/Updater.cpp +++ b/src/Updater.cpp @@ -16,13 +16,14 @@ */ #include -#include "Utils.h" -#include +#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) diff --git a/src/Utils.h b/src/Utils.h index a1fa90dc..5cd71a09 100644 --- a/src/Utils.h +++ b/src/Utils.h @@ -29,8 +29,6 @@ #include "JsError.h" -#define ArraySize(a) (sizeof(a) / sizeof(a[0])) - namespace AdblockPlus { namespace Utils