Issue 11018003: Filter changed callback (Closed)
|
Created:
June 18, 2013, 11:42 a.m. by Andrey Novikov Modified:
Sept. 17, 2013, 10:57 a.m. Visibility:
Public. |
DescriptionFilter changed callback
Patch Set 1 #
Total comments: 19
Patch Set 2 : Filter changed callback #
Total comments: 4
Patch Set 3 : Filter changed callback #Patch Set 4 : Filter changed callback #
MessagesTotal messages: 14
http://codereview.adblockplus.org/11018003/diff/1/Makefile File Makefile (right): http://codereview.adblockplus.org/11018003/diff/1/Makefile#newcode24 Makefile:24: $(MAKE) -C third_party/v8 android_arm.debug Disregard these file changes, it's for debugging purposes. http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... File lib/filterUpdateRegistration.js (right): http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... lib/filterUpdateRegistration.js:23: switch (action) I didn't change the logic of the current Android application. May be it has to be modified in some way to be more uniform? http://codereview.adblockplus.org/11018003/diff/1/libadblockplus.gyp File libadblockplus.gyp (right): http://codereview.adblockplus.org/11018003/diff/1/libadblockplus.gyp#newcode48 libadblockplus.gyp:48: 'ldflags': [ This is not the part of callback staff but is a useful change anyway. http://codereview.adblockplus.org/11018003/diff/1/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/11018003/diff/1/src/FilterEngine.cpp#newcode23 src/FilterEngine.cpp:23: #include <AdblockPlus/LogSystem.h> Forgot to remove this :( http://codereview.adblockplus.org/11018003/diff/1/src/FilterEngine.cpp#newcod... src/FilterEngine.cpp:295: AdblockPlus::LogSystemPtr log = jsEngine->GetLogSystem(); Forgot to remove this :(
http://codereview.adblockplus.org/11018003/diff/1/Makefile File Makefile (right): http://codereview.adblockplus.org/11018003/diff/1/Makefile#newcode24 Makefile:24: $(MAKE) -C third_party/v8 android_arm.debug I hope you won't check them in then :) http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:72: typedef std::tr1::function<void(const std::string&, const std::string&, const int64_t)> FilterChangeCallback; int64_t doesn't need to be const. http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:89: void SetFilterChangeCallback(FilterChangeCallback callback = 0); 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. http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... File lib/filterUpdateRegistration.js (right): http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... lib/filterUpdateRegistration.js:23: switch (action) On 2013/06/18 11:48:28, Andrey Novikov wrote: > I didn't change the logic of the current Android application. May be it has to > be modified in some way to be more uniform? Yes, this definitely needs to be more generic - this is a general-purpose library. One way to would be sending all status changes to the C++ code. That's likely unnecessary overhead however, there can be lots of messages. The other option is passing in a filter - what messages should be sent to the caller. You will need to add a function to api.js - C++ doesn't run any complicated JS code. The list of messages definitely shouldn't be hardcoded here. http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... lib/filterUpdateRegistration.js:41: } This processing should be done in the C++ code, the code here should just send the parameters unchanged. http://codereview.adblockplus.org/11018003/diff/1/libadblockplus.gyp File libadblockplus.gyp (right): http://codereview.adblockplus.org/11018003/diff/1/libadblockplus.gyp#newcode48 libadblockplus.gyp:48: 'ldflags': [ On 2013/06/18 11:48:28, Andrey Novikov wrote: > This is not the part of callback staff but is a useful change anyway. It should be android_arm.release however. http://codereview.adblockplus.org/11018003/diff/1/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/11018003/diff/1/src/FilterEngine.cpp#newcod... src/FilterEngine.cpp:284: std::string eventName = "filterChange"; The event name isn't dynamic, no real point passing it to the callback.
Where I didn't reply, I agree with Wladimir. http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:72: typedef std::tr1::function<void(const std::string&, const std::string&, const int64_t)> FilterChangeCallback; On 2013/06/18 15:02:15, Wladimir Palant wrote: > int64_t doesn't need to be const. 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. http://codereview.adblockplus.org/11018003/diff/1/libadblockplus.gyp File libadblockplus.gyp (right): http://codereview.adblockplus.org/11018003/diff/1/libadblockplus.gyp#newcode52 libadblockplus.gyp:52: '-lv8_base', I'm surprised this works, it's not really common to specify static libraries with -l. Think I prefer how it was before, but no strong opinion.
http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:72: typedef std::tr1::function<void(const std::string&, const std::string&, const int64_t)> FilterChangeCallback; 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.
http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/11018003/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:72: typedef std::tr1::function<void(const std::string&, const std::string&, const int64_t)> FilterChangeCallback; On 2013/06/19 11:27:09, Wladimir Palant wrote: > 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. 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.
http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... File lib/filterUpdateRegistration.js (right): http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... lib/filterUpdateRegistration.js:23: switch (action) So, I see no reason to complicate things with some filter, C code can deal with filtering by itself. Yes?
http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... File lib/filterUpdateRegistration.js (right): http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... lib/filterUpdateRegistration.js:23: switch (action) On 2013/06/25 08:08:02, Andrey Novikov wrote: > So, I see no reason to complicate things with some filter, C code can deal with > filtering by itself. Yes? IMO yes. It's simpler, and I doubt there'll be any noteworthy overhead if we just forward all events.
http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... File lib/filterUpdateRegistration.js (right): http://codereview.adblockplus.org/11018003/diff/1/lib/filterUpdateRegistratio... lib/filterUpdateRegistration.js:23: switch (action) On 2013/06/25 08:21:58, Felix H. Dahlke wrote: > IMO yes. It's simpler, and I doubt there'll be any noteworthy overhead if we > just forward all events. JS to C++ calls always mean overhead and we will be producing lots of them here. Still, probably not worth it unless we see that this indeed has a performance impact.
http://codereview.adblockplus.org/11018003/diff/14001/lib/filterUpdateRegistr... File lib/filterUpdateRegistration.js (right): http://codereview.adblockplus.org/11018003/diff/14001/lib/filterUpdateRegistr... lib/filterUpdateRegistration.js:23: _triggerEvent("filterChange", action, subscription.url); Actually, we currently have three types of notifications: * General notifications, come without any parameters * Subscription notifications, have the subscription as parameter * Filter notifications, have a filter as parameter While you are only interested in subscription notifications, we agreed on not performing any filtering in the library. This means that you should rename the second parameter here from "subscription" into "item" and just pass it on to _triggerEvent(), even if it is undefined. http://codereview.adblockplus.org/11018003/diff/14001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/11018003/diff/14001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:284: this, "filterChange", callback, std::tr1::placeholders::_1)); Still not point passing "filterChange" to the callback, this parameter isn't even used ;) Also, does this even compile with the eventName variable being undefined? It should be "filterChange" instead. http://codereview.adblockplus.org/11018003/diff/14001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:289: jsEngine->RemoveEventCallback(eventName); Does this even compile with the eventName variable being undefined? It should be "filterChange" instead. http://codereview.adblockplus.org/11018003/diff/14001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:295: std::string url(params.size() >= 2 && !params[1]->IsNull() ? params[1]->AsString() : ""); Please keep the second parameter generic, something like: JsValuePtr item(params.size() >= 2 ? params[1] : jsEngine->NewValue(false)); The callback can then call new Filter(item) or new Subscription(item) if necessary.
LGTM
LGTM
Can you close this, Andrey? 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
