Issue 4125 - [emscripten] Convert filter classes to C++

compiled/ActiveFilter.h

+#pragma once
+
+#include <emscripten.h>
+
+#include "Filter.h"
+#include "StringMap.h"
+
+#define FILTER_PROPERTY(type, name, getter, setter) \
+    private:\
+      type name;\
+    public:\
+      type EMSCRIPTEN_KEEPALIVE getter() const\
+      {\
+        return name;\
+      }\
+      void EMSCRIPTEN_KEEPALIVE setter(type value)\
+      {\
+        if (name != value)\
+        {\
+          type oldvalue = name;\
+          name = value;\
+          DependentString action(u"filter."_str #name);\
+          EM_ASM_ARGS({\
+            var filter = new (exports[Filter_mapping[$2]])($1);\
+            FilterNotifier.triggerListeners(getStringData($0), filter, $3, $4);\

[sergei, 2016/06/16 21:16:22]

We need to call filter.delete();
See also
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i....

The rule that the caller must call AddRef for a parameter is a good rule in
addition to
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...

void f(ref_counter* param)
{
  {
    untrusive_ptr(param);
  }
  // if reference counter of param was zero then now param is not nullptr but
object is already deleted.
}

[Wladimir Palant, 2016/12/06 10:47:10]

We don't. This doesn't create a filter instance, merely a new JavaScript wrapper
around an existing instance. Given that this wrapper is being passed as a
parameter to a call there is no reason why we should addref here.

+          }, &action, this, GetType(), value, oldvalue);\
+        }\
+      }
+
+class ActiveFilter : public Filter
+{
+protected:
+  typedef StringMap<bool> DomainMap;
+  typedef StringSet SitekeySet;
+  void ParseDomains(const String& domains, String::value_type separator) const;
+  void AddSitekey(const String& sitekey) const;
+  virtual DomainMap* GetDomains() const;
+  virtual SitekeySet* GetSitekeys() const;
+  mutable std::unique_ptr<DomainMap> mDomains;
+  mutable std::unique_ptr<SitekeySet> mSitekeys;
+private:
+  bool mIgnoreTrailingDot;
+public:
+  ActiveFilter(const String& text, bool ignoreTrailingDot);
+  FILTER_PROPERTY(bool, mDisabled, GetDisabled, SetDisabled);
+  FILTER_PROPERTY(unsigned int, mHitCount, GetHitCount, SetHitCount);
+  FILTER_PROPERTY(unsigned int, mLastHit, GetLastHit, SetLastHit);
+  static void ToLower(DependentString&& str);

[sergei, 2016/06/16 21:16:27]

I would prefer to have it separate from this class.

[Wladimir Palant, 2016/12/06 10:47:12]

Ok, String::tolower() then.

+  bool EMSCRIPTEN_KEEPALIVE IsActiveOnDomain(DependentString& docDomain,
+      const String& sitekey) const;
+  bool EMSCRIPTEN_KEEPALIVE IsActiveOnlyOnDomain(DependentString& docDomain) const;
+  bool EMSCRIPTEN_KEEPALIVE IsGeneric() const;
+  OwnedString EMSCRIPTEN_KEEPALIVE Serialize() const;

[sergei, 2016/06/16 21:16:23]

Could you please add "override", so it's visible that it's overridden virtual
method and compiler also verifies it.

[Wladimir Palant, 2016/12/06 10:47:17]

It's not a virtual method, hence no "override" here. This method is only called
by our JS bindings and there we map it to a specific class. No point wasting
memory on virtual methods.

[sergei, 2017/01/10 15:57:18]

Acknowledged.

+};