Issue 5137 - [emscripten] Added basic filter storage implementation

compiled/bindings/generator.cpp

Index: compiled/bindings/generator.cpp
===================================================================
--- a/compiled/bindings/generator.cpp
+++ b/compiled/bindings/generator.cpp
@@ -19,16 +19,44 @@
 
 #include <emscripten.h>
 
 #include "generator.h"
 
 namespace
 {
   std::vector<bindings_internal::ClassInfo> classes;
+  std::vector<bindings_internal::NamespaceInfo> namespaces;
+
+  void printProperties(const bindings_internal::PropertyList& properties)
+  {
+    for (const auto& property : properties)
+    {
+      if (property.jsValue.empty())
+      {
+        printf("get %s%s,\n", property.name.c_str(),
+               wrapCall(property.getter, false).c_str());
+        if (!property.setter.empty())
+        {
+          printf("set %s%s,\n", property.name.c_str(),
+                 wrapCall(property.setter, false).c_str());
+        }
+      }
+      else
+        printf("%s: %s,\n", property.name.c_str(), property.jsValue.c_str());
+    }
+  }
+
+  void printMethods(const bindings_internal::MethodList& methods,
+                    bool instanceOnly = true)
+  {
+    for (const auto& method : methods)
+      if (!instanceOnly || method.call.instance_function)
+        printf("%s: %s,\n", method.name.c_str(), wrapCall(method.call).c_str());
+  }
 }
 
 namespace bindings_internal
 {
   FunctionInfo::FunctionInfo()
   {
     name[0] = '\0';
   }
@@ -250,16 +278,61 @@ namespace bindings_internal
       throw std::runtime_error("More than one subclass differentiator defined for class " + classInfo->name);
 
     DifferentiatorInfo differentiatorInfo;
     differentiatorInfo.offset = offset;
     differentiatorInfo.mapping = mapping;
     classInfo->subclass_differentiator = differentiatorInfo;
   }
 
+  NamespaceInfo* find_namespace(const char* namespaceName)
+  {
+    for (auto& namespaceInfo : namespaces)
+      if (namespaceInfo.name == namespaceName)
+        return &namespaceInfo;
+    return nullptr;
+  }
+
+  void register_namespace(const char* name)
+  {
+    if (find_namespace(name))
+      throw std::runtime_error(std::string("Duplicate definition for namespace ") + name);
+
+    NamespaceInfo namespaceInfo;
+    namespaceInfo.name = name;
+    namespaces.push_back(namespaceInfo);
+  }
+
+  void register_namespace_property(const char* namespaceName, const char* name,
+      const FunctionInfo& getter, const FunctionInfo& setter)
+  {
+    NamespaceInfo* namespaceInfo = find_namespace(namespaceName);
+    if (!namespaceInfo)
+      throw std::runtime_error(std::string("Property defined on unknown namespace: ") + name);
+
+    PropertyInfo propertyInfo;
+    propertyInfo.name = name;
+    propertyInfo.getter = getter;
+    propertyInfo.setter = setter;
+    namespaceInfo->properties.push_back(propertyInfo);
+  }
+
+  void register_namespace_method(const char* namespaceName, const char* name,
+      const FunctionInfo& call)
+  {
+    NamespaceInfo* namespaceInfo = find_namespace(namespaceName);
+    if (!namespaceInfo)
+      throw std::runtime_error(std::string("Method defined on unknown namespace: ") + name);
+
+    MethodInfo methodInfo;
+    methodInfo.name = name;
+    methodInfo.call = call;
+    namespaceInfo->methods.push_back(methodInfo);
+  }

[Wladimir Palant, 2017/05/01 14:47:25]

I'm not really happy with the complexity required to expose namespaces. We
should probably discuss changes to the bindings definition API - maybe the code
can get considerably simpler if we go away from the embind-style APIs.

[sergei, 2017/05/08 10:54:39]

I would like to include in that discussion the design, in particular global
variables like knownFilters, FilterNotifier and subscriptions (in
FilterStorage.cpp), not loosely coupling like Listed property of subscription,
and global things mentioned above like FilterNotifier and in some other places.
So far, I find these things OK but only because it closely repeats the original
JS version, so the number of bugs should be less, but later it would be better
to reconsider such things.

In particular here, FilterStorage should be a class and we will get rid of the
namespace. However I would like to have the whole library in a namespace, if
it's a bit tricky then we could use for instance ABP prefix for everything.

[Wladimir Palant, 2017/05/08 12:55:41]

I considered making FilterStorage a singleton class. The downside here is that
calling instance methods from JavaScript is considerably less efficient while
not offering any benefit here whatsoever.

[sergei, 2017/08/24 13:32:04]

Singleton and global variables are actually equally bad from design point of
view. I would wait for finishing of the transformation, then take a look what
and how is actually used and then restructure in in way to get rid of these
global things without hurting the performance.

+
   std::string generateCall(const FunctionInfo& call,
       std::vector<std::string>& params)
   {
     if (call.returnType == TypeCategory::DEPENDENT_STRING ||
         call.returnType == TypeCategory::OWNED_STRING)
     {
       params.insert(params.begin(), "string");
     }
@@ -343,17 +416,17 @@ namespace bindings_internal
       prefix += argName;
 
       if (call.args[i] == TypeCategory::STRING_REF)
       {
         hasStringArgs = true;
         params.push_back(std::string("createString(") + argName + ")");
       }
       else if (call.args[i] == TypeCategory::CLASS_PTR)
-        params.push_back(argName + "._pointer");
+        params.push_back(argName + " ? " + argName + "._pointer : 0");

[Wladimir Palant, 2017/05/01 14:47:25]

Turns out we weren't supporting null for parameters expecting a class.

[sergei, 2017/05/08 10:54:39]

In addition I wonder whether we can use C++ references, we do use them for
strings already, why not to use them also for other objects.

[Wladimir Palant, 2017/05/08 12:55:41]

We could use references easily but these won't be references in JavaScript. The
bindings code will then have to make sure that no null pointers are passed in.
So far this didn't seem to be worth doing but we can certainly consider it.

[sergei, 2017/08/24 13:32:04]

Currently one has to manually test whether a pointer argument is null or not
each time in a method implementation, however many functions expect a real
object and this testing is just additional code which can be avoided. Therefore
using C++ references is safer and easier in many cases. However, if we
accidentally pass not a "real object" from JS into a function which does not
expect it, it would be better to generate the check into bindings than manually
implement it in a method implementation.

In addition I guess we should think about it a little bit more because it can
help to develop some other conventions, e.g. ref_counted objects should be
always allocated on heap and never on the stack.

[Wladimir Palant, 2017/08/31 11:32:34]

I filed https://issues.adblockplus.org/ticket/5603 on that.

       else if (call.args[i] == TypeCategory::INT64)
       {
         // 64-bit integers are passed as two integer parameters
         params.push_back(argName + " >>> 0");
         params.push_back(argName + " / 0x100000000 >>> 0");
       }
       else
         params.push_back(argName);
@@ -427,50 +500,29 @@ namespace bindings_internal
         return result;
       }
     )");
   }
 
   void printClass(const ClassInfo& cls)
   {
     // Begin class definition
-
     ClassInfo* baseClass = find_class(cls.baseClass);
     printf("exports.%s = createClass(%s, %i, {\n", cls.name.c_str(),
         (baseClass ? ("exports." + baseClass->name).c_str() : "null"),
         cls.ref_counted_offset);
 
     // Print prototype members
-
-    for (const auto& property : cls.properties)
-    {
-      if (property.jsValue.empty())
-      {
-        printf("get %s%s,\n", property.name.c_str(),
-               wrapCall(property.getter, false).c_str());
-        if (!property.setter.empty())
-        {
-          printf("set %s%s,\n", property.name.c_str(),
-                 wrapCall(property.setter, false).c_str());
-        }
-      }
-      else
-        printf("%s: %s,\n", property.name.c_str(), property.jsValue.c_str());
-    }
-
-    for (const auto& method : cls.methods)
-      if (method.call.instance_function)
-        printf("%s: %s,\n", method.name.c_str(), wrapCall(method.call).c_str());
+    printProperties(cls.properties);
+    printMethods(cls.methods);
 
     // End class definition
-
     printf("});\n");
 
     // Print static members
-
     DifferentiatorInfo differentiator = cls.subclass_differentiator;
     if (differentiator.offset != SIZE_MAX)
     {
       printf("var %s_mapping = \n", cls.name.c_str());
       puts("{");
       for (const auto& item : differentiator.mapping)
         printf("  %i: '%s',\n", item.first, item.second.c_str());
       puts("};");
@@ -495,17 +547,27 @@ namespace bindings_internal
     {
       if (!method.call.instance_function)
       {
         printf("exports.%s.%s = %s;\n", cls.name.c_str(), method.name.c_str(),
                wrapCall(method.call).c_str());
       }
     }
   }
+
+  void printNamespace(const NamespaceInfo& namespaceInfo)
+  {
+    printf("exports.%s = {\n", namespaceInfo.name.c_str());
+    printProperties(namespaceInfo.properties);
+    printMethods(namespaceInfo.methods, false);
+    puts("};");
+  }
 }
 
 void printBindings()
 {
   bindings_internal::printHelpers();
 
   for (const auto& item : classes)
     bindings_internal::printClass(item);
+  for (const auto& item : namespaces)
+    bindings_internal::printNamespace(item);
 }