FilterEngine API improvements

include/AdblockPlus/FilterEngine.h

Index: include/AdblockPlus/FilterEngine.h
===================================================================
--- a/include/AdblockPlus/FilterEngine.h
+++ b/include/AdblockPlus/FilterEngine.h
@@ -1,39 +1,109 @@
 #ifndef ADBLOCKPLUS_FILTER_ENGINE_H
 #define ADBLOCKPLUS_FILTER_ENGINE_H
 
 #include <vector>
+#include <map>
 #include <string>
 
 namespace AdblockPlus
 {
   class JsEngine;
+  class FilterEngine;
 
-  struct Subscription
+  class JSObject
   {
-    std::string url;
-    std::string title;
+  public:
+    std::string GetProperty(const std::string& name, const std::string& defaultValue) const;
+    int GetProperty(const std::string& name, int defaultValue) const;
+    bool GetProperty(const std::string& name, bool defaultValue) const;
+    inline std::string GetProperty(const std::string& name, const char* defaultValue) const
+    {
+      return GetProperty(name, std::string(defaultValue));
+    }
 
-    Subscription(const std::string& url, const std::string& title);
+    void SetProperty(const std::string& name, const std::string& value);
+    void SetProperty(const std::string& name, int value);
+    void SetProperty(const std::string& name, bool value);
+    inline void SetProperty(const std::string& name, const char* value)
+    {
+      SetProperty(name, std::string(value));
+    }
+
+  protected:
+#if FILTER_ENGINE_STUBS
+    JSObject(FilterEngine& filterEngine);
+
+    FilterEngine& filterEngine;
+    std::map<std::string, std::string> stringProperties;
+    std::map<std::string, int> intProperties;
+    std::map<std::string, bool> boolProperties;
+#else
+    JSObject();
+#endif
   };
 
+  class Filter : public JSObject
+  {
+    friend class FilterEngine;
+
+  public:
+    bool IsListed() const;
+    void AddToList();
+    void RemoveFromList();
+
+  private:
+#if FILTER_ENGINE_STUBS
+    Filter(FilterEngine& filterEngine, const std::string& text);
+#else
+    Filter();
+#endif
+  };
+
+  class Subscription : public JSObject
+  {
+    friend class FilterEngine;
+
+  public:
+    bool IsListed() const;
+    void AddToList();
+    void RemoveFromList();
+    void UpdateFilters();
+
+  private:
+#if FILTER_ENGINE_STUBS
+    Subscription(FilterEngine& filterEngine, const std::string& url);
+#else
+    Subscription();
+#endif
+  };
+
+  typedef void (*SubscriptionsCallback)(const std::vector<Subscription*>&);
+
   class FilterEngine
   {
+    friend class Filter;
+    friend class Subscription;
   public:
     explicit FilterEngine(JsEngine& jsEngine);
-    void AddSubscription(Subscription subscription);
-    void RemoveSubscription(const Subscription& subscription);
-    const Subscription* FindSubscription(const std::string& url) const;
-    const std::vector<Subscription>& GetSubscriptions() const;
-    void UpdateSubscriptionFilters(const Subscription& subscription);
-    std::vector<Subscription> FetchAvailableSubscriptions();
-    bool Matches(const std::string& url,
-                 const std::string& contentType) const;
+    Filter& GetFilter(const std::string& text);
+    Subscription& GetSubscription(const std::string& url);
+    const std::vector<Filter*>& GetListedFilters() const;
+    const std::vector<Subscription*>& GetListedSubscriptions() const;
+    void FetchAvailableSubscriptions(SubscriptionsCallback callback);
+    Filter* Matches(const std::string& url,
+                    const std::string& contentType,
+                    const std::string& documentUrl);

[Oleksandr, 2013/04/05 14:09:07]

Might be just me, but based on this signature I might accidentally do a delete
on a return value after I'm done with it. Based on the implementation it doesn't
seem like I should though. Maybe it would be better if the function received the
pointer to a Filter object and would modify it? Same for subscriptions.

[Felix Dahlke, 2013/04/05 14:20:59]

How would that be any less ambigous?

IMO, we should preferably return an auto_ptr to a copy of the filter object. If
that doesn't make sense, I'll vote for sticking with a pointer until we have
shared_ptr (I'll personally implement the damn thing if we don't agree on using
any of TR1, Boost or C++11 :D)

[Wladimir Palant, 2013/04/05 14:31:45]

Actually, the idea was that filters and subscriptions wouldn't be copied around
- there is a unique Filter/Subscription instance for each JS object. Or did I
misunderstand your suggestion?

[Felix Dahlke, 2013/04/05 14:43:55]

Then a pointer should be fine.

You have a point in the mail Oleksandr (didn't show up in the comments), an
input parameter would somewhat imply that the caller has to delete it, although
I'd never feel safe without looking it up in the docs.

But it seems the caller shouldn't delete it, so a shared_ptr or weak_ptr (or C
pointer for now) would be better. And if it should, why not just return an
auto_ptr?

[Wladimir Palant, 2013/04/08 13:51:47]

I am using shared_ptr instead of raw pointers now.

     std::vector<std::string> GetElementHidingRules() const;
 
   private:
     JsEngine& jsEngine;
-    std::vector<Subscription> subscriptions;
+#if FILTER_ENGINE_STUBS
+    std::map<std::string, Filter*> knownFilters;
+    std::vector<Filter*> listedFilters;
+    std::map<std::string, Subscription*> knownSubscriptions;
+    std::vector<Subscription*> listedSubscriptions;
+#endif
   };
 }
 
 #endif