Refactoring task #6

kzlomek · 2018-07-23T08:36:01Z

No description provided.

Created Updater class for handling application updates and moved respective API from FilterEngine to Updater component. For now Updater class does not care about asynchronicity (if any) in JS files which are used for update checks logic. TODO: Move Updater class to dedicated .h and .cpp files. Abstracted jsSources from FilterEngine. Now every component (like FilterEngine and Updater) created by Platfrom gets EvaluateCallback and evaluates own JS files needed for internal logic. EvaluateCallback keeps track of evaluated files to avoid duplicated evaluations.

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.

Added cpp methods for Prefs JS API. Removed not needed JS API. Added _isSubscriptionDownloadAllowed event callback to unblock downloads in JS code. Cleaned up UpdateCheckTest a bit removing FilterEngine because with the change above those tests don't require anymore FilterEnigne object to pass.

abby-sergz

It looks pretty good, there are just some peculiarities.

abby-sergz · 2018-08-01T11:05:00Z

include/AdblockPlus/JsEngine.h

+     * Callback type for evaluating JS expression.
+     * The parameter is the JS file name containing the expression.
+     */
+    typedef std::function<void(const std::string&)> EvaluateCallback;


It's actually unrelated to JsEngine, it's rather an interface of FilterEngine and Updater, therefore IMO it would be better to define it there, thus two separate typedefs in both FilterEngine and Updater.

abby-sergz · 2018-08-01T11:09:24Z

src/FilterEngine.cpp

 #include "JsContext.h"
 #include "Thread.h"
 #include <mutex>
 #include <condition_variable>

 using namespace AdblockPlus;

-extern std::string jsSources[];
+namespace {
+    static std::string filterEngineJsFiles[] = {


it's not necessary to make it static but it should be const.

abby-sergz · 2018-08-01T11:10:16Z

src/FilterEngine.cpp

 #include "JsContext.h"
 #include "Thread.h"
 #include <mutex>
 #include <condition_variable>

 using namespace AdblockPlus;

-extern std::string jsSources[];
+namespace {


The coding style in this project implies that { are located on a new line, could you please move it there?

abby-sergz · 2018-08-01T11:11:44Z

src/FilterEngine.cpp

  // Set the preconfigured prefs
  auto preconfiguredPrefsObject = jsEngine->NewObject();
  for (const auto& pref : params.preconfiguredPrefs)
-  {


This change seem unrelated, could you please revert it back?

abby-sergz · 2018-08-01T11:12:00Z

src/FilterEngine.cpp

@@ -228,7 +262,7 @@ void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine,
      isSubscriptionDownloadAllowedCallback(params[0].IsString() ? &allowedConnectionType : nullptr, callJsCallback);
    });
  }
-


This change seem unrelated, could you please revert it back?

abby-sergz · 2018-08-01T11:56:19Z

src/Platform.cpp

+        return; //NO-OP, file was already evaluated
+
+      // Lock the JS engine while we are loading scripts
+      const JsContext context(*jsEngine);


The lock is not necessary here.

Please check my response to your comment about removing "const JsContext context(*jsEngine);" from FilterEngine::CreateAsync().

My previous comment is not valid anymore.

abby-sergz · 2018-08-01T11:57:53Z

src/Updater.cpp

+  : jsEngine(jsEngine), updateCheckId(0)
+{
+  // Hack to enable downloads from JS (see compat.js)
+  jsEngine->SetEventCallback("_isSubscriptionDownloadAllowed", [this](JsValueList&& params)


this code is unrelated to the Updater, could you please not bring it here?

Indeed, it is a bad hack. For the time being, to enable Updater downloads is JS code, that is to not rely on _isSubscriptionDownloadAllowed, I propose to add following execution path to send() method in compat.js just before _isSubscriptionDownloadAllowed() is executed:
if (this._url.includes("update.json"))
{
window._webRequest.GET(this._url, this._requestHeaders, onGetDone);
return;
}
Do you agree?

Right, it's required. I agree, let's add that check to compat.js.

abby-sergz · 2018-08-01T12:03:22Z

src/Updater.cpp

+
+  // Set empty preconfigured prefs
+  auto preconfiguredPrefsObject = jsEngine->NewObject();
+  jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject);


This code is actually unrelated, however, since prefs.js is required and prefs.js expects that object, I would propose to change prefs.js. The latter should rather check whether _preconfiguredPrefs exists. Additionally, it can happen that someone firstly instantiated Updater and then is instantiating FilterEngine, but prefs are already initialized. IMO we may not fully address it here but fix it later, under another ticket.

I'll remove this code from the Updater class and add a check to prefs.js to iterate through _preconfiguredPrefs only if this object exists. Is that enough for the scope of this task? Later on _preconfiguredPrefs could be split so FilterEngine and Updater would use dedicated objects.

It's absolutely enough. I think later we should rather extract Prefs the similar way as Updater and make the former as a dependency for FilterEngine and Updater.

abby-sergz · 2018-08-01T12:05:11Z

lib/apiUpdater.js

+
+let API_UPDATER = (() =>
+{
+  const {Services} = Cu.import("resource://gre/modules/Services.jsm", {});


It seems Services is not used here, could you please remove it?

abby-sergz · 2018-08-01T12:10:02Z

include/AdblockPlus/Platform.h

@@ -125,11 +133,15 @@ namespace AdblockPlus
    TimerPtr timer;
    FileSystemPtr fileSystem;
    WebRequestPtr webRequest;
+    JsEngine::EvaluateCallback evaluateCallback;


It seems unused, could you please remove it?

kzlomek · 2018-08-02T16:36:26Z

Thanks @abby-sergz, I've added my responses to clarify some things before I'll push new a commit which addresses review remarks.

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

kzlomek · 2018-08-05T09:34:52Z

@abby-sergz , my commit from yesterday should address all review comments.

abby-sergz

Could you please add a couple of tests instantiating both FilterEngine and Updater in different orders and I think it will be over.

abby-sergz · 2018-08-06T08:38:10Z

src/Platform.cpp

@@ -91,6 +117,16 @@ FilterEngine& Platform::GetFilterEngine()
  return *std::shared_future<FilterEnginePtr>(filterEngine).get();
 }

+Updater& Platform::GetUpdater()
+{


I would like to have creation of Updater and evaluatedJsSources to be thread safe, but perhaps it's actually fine for present because I suspect that all these creations will be re-engineered soon anyway.

Good point, it crossed my mind when I started writing this code. For the Updater thread safety, since Updater does not require async callback from JS code for creation, I propose to change constructor to:

Updater& Platform::GetUpdater() { { std::lock_guard<std::mutex> lock(modulesMutex); if (updater) return *updater; } GetJsEngine(); // ensures that JsEngine is instantiated { std::lock_guard<std::mutex> lock(modulesMutex); if (!updater) updater = std::make_shared<Updater>(jsEngine, GetEvaluateCallback()); return *updater; } }

And I'll add a dedicated mutex to serialize access to evaluatedJsSources.

kzlomek · 2018-08-06T09:13:56Z

@abby-sergz , regarding requested tests do you want to check in those tests something specific to confirm that FilterEngine and Updater are operable, for example by calling Set(Get)Pref() on both of them?

abby-sergz · 2018-08-06T09:22:38Z

Good point, yeah, it would be great. Just call what you think makes sense. пн, 6 авг. 2018 г. в 11:13, kzlomek <[email protected]>:

…

@abby-sergz <https://github.com/abby-sergz> , regarding requested tests do you want to check in those tests something specific to confirm that FilterEngine and Updater are operable, for example by calling Set(Get)Pref() on both of them? —

Now creation of Updater is thread safe. Added synchronization for evaluatedJsSources set. Added more unit tests.

kzlomek · 2018-08-08T07:13:48Z

I've pushed commit "Fixed thread safety and added more tests" and then force pushed amendments in this commit with fixes in indentation and to resolve bug in the testes. I don't plan to update this commit so please review.

abby-sergz · 2018-08-09T11:24:10Z

test/UpdaterAndFilterEngineCreation.cpp

+
+    static const size_t COUNT = 100;
+    const std::string PROP_NAME = "patternsbackupinterval";
+    std::vector<std::thread> threadsVector;


Basically, one can use AdblockPlus::AsyncExecutor from include/AdblockPlus/AsyncExecutor.h. It also should not be a member, just allocate it before the loop (below) and call asyncExecutor.Dispatch(the-same-labda).

OK, I'll check it and use AsyncExecutor.

abby-sergz · 2018-08-09T11:33:26Z

test/UpdaterAndFilterEngineCreation.cpp

+    static const size_t COUNT = 100;
+    const std::string PROP_NAME = "patternsbackupinterval";
+    std::vector<std::thread> threadsVector;
+    Updater* updaterAddrArray[COUNT];


what do you think about using of std::array here?

Yes, good tip.

abby-sergz · 2018-08-09T11:36:53Z

test/UpdaterAndFilterEngineCreation.cpp

+      for (size_t idx = 0; idx < COUNT; ++idx)
+        threadsVector.push_back(
+          std::thread([this, idx, isUpdater, isFilterEngine]() -> void {
+            Updater* updaterAddr = nullptr;


Do we need these variables? It seems one can just assign an item in the array.

I used them just to make the assignment in one place at the end of this method. But I can remove them.

abby-sergz · 2018-08-09T11:37:52Z

test/UpdaterAndFilterEngineCreation.cpp

+TEST_F(UpdaterAndFilterEngineCreationTest, TestFilterEngineSingleInstance)
+{
+    CallGetFilterEngineSimultaneously();
+    FilterEngine* filterEngineAddr = filterAddrArray[0];


Strictly speaking it would be good to test just in case that the value of the first element is not nullptr since we expect it anyway.

Good point.

abby-sergz · 2018-08-09T11:39:46Z

test/UpdaterAndFilterEngineCreation.cpp

+TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrder1)
+{
+    Updater& updater = platform->GetUpdater();
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));


Just wonder, does it not work without that sleep?

No, sleep is not needed.

abby-sergz

I think it's over, just a tiny thing is left there.

abby-sergz · 2018-08-10T12:32:25Z

test/UpdaterAndFilterEngineCreation.cpp

+{
+    CallGetFilterEngineSimultaneously();
+    FilterEngine* filterEngineAddr = filterAddrArray[0];
+    ASSERT_NE(filterEngineAddr, nullptr);


Sorry for not noticing it before. Even if it's a nullptr, we still can continue the execution of the test. It's a pretty subtle and perhaps even subjective thing, but could you please change all these ASSERT_* to EXPECT_*. It can seem inconsistent from other tests but we are gradually applying this approach.

Usually EXPECT_* are preferred, as they allow more than one failure to be reported in a test. However, you should use ASSERT_* if it doesn't make sense to continue when the assertion in question fails.

Therefore we usually use ASSERT_ in such cases when we know that the normal execution should not be possible, e.g. of a seg-fault because of dereferencing of a nullptr (in general case it can of course work without a seg-fault).

BTW, normally the order of arguments is that the expected value is going first and the actual value is the second one. Could you please change that order too if it does not cause compilation errors?

OK, I know this appoach - I'll change those nullptr checks from ASSERT to EXPECT and will update the order, I'll push new commit in couple of minutes.

kzlomek added 4 commits July 19, 2018 08:55

Code cleanup

7d6bdda

abby-sergz suggested changes Aug 1, 2018

View reviewed changes

Addressed review comments

49f0171

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

abby-sergz reviewed Aug 6, 2018

View reviewed changes

kzlomek force-pushed the refactoring_task branch 4 times, most recently from 7246e0a to 98318ef Compare August 8, 2018 06:14

Fixed thread safety and added more tests

511c949

Now creation of Updater is thread safe. Added synchronization for evaluatedJsSources set. Added more unit tests.

kzlomek force-pushed the refactoring_task branch from 98318ef to 511c949 Compare August 8, 2018 06:53

abby-sergz reviewed Aug 9, 2018

View reviewed changes

Reworked last commit applying review comments

1ae9556

abby-sergz reviewed Aug 10, 2018

View reviewed changes

Updated UpdaterAndFilterEngineCreationTest to match coding convention

956d620

kzlomek force-pushed the refactoring_task branch from 0db12e3 to 956d620 Compare August 10, 2018 13:43

Refactoring task #6

Are you sure you want to change the base?

Refactoring task #6

Conversation

kzlomek commented Jul 23, 2018

abby-sergz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzlomek commented Aug 2, 2018

kzlomek commented Aug 5, 2018

abby-sergz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzlomek commented Aug 6, 2018

abby-sergz commented Aug 6, 2018 via email • edited Loading

kzlomek commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abby-sergz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abby-sergz commented Aug 6, 2018 via email •

edited

Loading