Filter changed callback

include/AdblockPlus/FilterEngine.h

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 51 matching lines @@
     Subscription(JsValuePtr value);
   };
 
   typedef std::tr1::shared_ptr<Filter> FilterPtr;
   typedef std::tr1::shared_ptr<Subscription> SubscriptionPtr;
 
   class FilterEngine
   {
   public:
     typedef std::tr1::function<void(const std::string&)> UpdaterCallback;
-    typedef std::tr1::function<void(const std::string&, const std::string&, const int64_t)> FilterChangeCallback;

[Wladimir Palant, 2013/06/18 15:02:15]

int64_t doesn't need to be const.

[Felix Dahlke, 2013/06/19 10:28:38]

It's a matter of taste here. I like to const everything I can, but avoid that in
code where that is not common. In libadblockplus it's rather mixed.

[Wladimir Palant, 2013/06/19 11:27:09]

This essentially disallows the callback to change the parameter value even
though that change won't have any effect on the caller - we shouldn't mandate it
even if we consider changing parameter values bad style.

[Felix Dahlke, 2013/06/19 11:35:34]

That's the trouble with const parameters, have to change the public signature.
Some consider it good style, others don't. I'm in the former camp :) Not worth
picking nits over though.

+    typedef std::tr1::function<void(const std::string&, const JsValuePtr)> FilterChangeCallback;
 
     explicit FilterEngine(JsEnginePtr jsEngine);
     JsEnginePtr GetJsEngine() const { return jsEngine; }
     bool IsFirstRun() const;
     FilterPtr GetFilter(const std::string& text);
     SubscriptionPtr GetSubscription(const std::string& url);
     std::vector<FilterPtr> GetListedFilters() const;
     std::vector<SubscriptionPtr> GetListedSubscriptions() const;
     std::vector<SubscriptionPtr> FetchAvailableSubscriptions() const;
     FilterPtr Matches(const std::string& url,
         const std::string& contentType,
         const std::string& documentUrl) const;
     std::vector<std::string> GetElementHidingSelectors(const std::string& domain) const;
     JsValuePtr GetPref(const std::string& pref) const;
     void SetPref(const std::string& pref, JsValuePtr value);
     void ForceUpdateCheck(UpdaterCallback callback = 0);
-    void SetFilterChangeCallback(FilterChangeCallback callback = 0);

[Wladimir Palant, 2013/06/18 15:02:15]

Having a default for the callback makes no sense - setting the callback is the
very purpose of this function (unlike ForceUpdateCheck above).

If removing is the point then a separate RemoveFilterChangeCallback() function
is better.

+    void SetFilterChangeCallback(FilterChangeCallback callback);
+    void RemoveFilterChangeCallback();
 
   private:
     JsEnginePtr jsEngine;
     bool initialized;
     bool firstRun;
     int updateCheckId;
 
     void InitDone(JsValueList& params);
     void UpdateCheckDone(const std::string& eventName, UpdaterCallback callback, JsValueList& params);
-    void FilterChanged(const std::string& eventName, FilterChangeCallback callback, JsValueList& params);
+    void FilterChanged(FilterChangeCallback callback, JsValueList& params);
   };
 }
 
 #endif