FilterEngine API improvements

include/AdblockPlus/FilterEngine.h

 #ifndef ADBLOCKPLUS_FILTER_ENGINE_H
 #define ADBLOCKPLUS_FILTER_ENGINE_H
 
 #include <vector>
 #include <map>
 #include <string>
+#include <tr1/memory>
 
 namespace AdblockPlus
 {
   class JsEngine;
   class FilterEngine;
 
-  class JSObject
+  class JsObject
   {
   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));
     }
 
     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);
+    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();
+    JsObject();
 #endif
   };
 
-  class Filter : public JSObject
+  class Filter : public JsObject,
+                 public std::tr1::enable_shared_from_this<Filter>
   {
     friend class FilterEngine;
 
   public:
+    enum Type {TYPE_BLOCKING, TYPE_EXCEPTION,
+               TYPE_ELEMHIDE, TYPE_ELEMHIDE_EXCEPTION,
+               TYPE_COMMENT, TYPE_INVALID};
+
     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
+  class Subscription : public JsObject,
+                       public std::tr1::enable_shared_from_this<Subscription>
   {
     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*>&);
+  typedef std::tr1::shared_ptr<Filter> FilterPtr;
+  typedef std::tr1::shared_ptr<Subscription> SubscriptionPtr;
+  typedef void (*SubscriptionsCallback)(const std::vector<SubscriptionPtr>&);
 
   class FilterEngine
   {
     friend class Filter;
     friend class Subscription;
   public:
     explicit FilterEngine(JsEngine& jsEngine);
     Filter& GetFilter(const std::string& text);
     Subscription& GetSubscription(const std::string& url);
-    const std::vector<Filter*>& GetListedFilters() const;
-    const std::vector<Subscription*>& GetListedSubscriptions() const;
+    const std::vector<FilterPtr>& GetListedFilters() const;
+    const std::vector<SubscriptionPtr>& 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;
+    FilterPtr Matches(const std::string& url,
+        const std::string& contentType,
+        const std::string& documentUrl);
+    std::vector<std::string> GetElementHidingSelectors(const std::string& domain) const;
 
   private:
     JsEngine& jsEngine;
 #if FILTER_ENGINE_STUBS
-    std::map<std::string, Filter*> knownFilters;
-    std::vector<Filter*> listedFilters;
-    std::map<std::string, Subscription*> knownSubscriptions;
-    std::vector<Subscription*> listedSubscriptions;
+    std::map<std::string, FilterPtr> knownFilters;
+    std::vector<FilterPtr> listedFilters;
+    std::map<std::string, SubscriptionPtr> knownSubscriptions;
+    std::vector<SubscriptionPtr> listedSubscriptions;
 #endif
   };
 }
 
 #endif