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);\
+            FilterNotifier.triggerListeners(readString($0), filter, $3, $4);\
+          }, &action, this, mType, 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);
+  explicit ActiveFilter(Type type, 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.

 };