Issue 10100009: FilterEngine API improvements (Closed)
|
Created:
April 4, 2013, 5:04 p.m. by Wladimir Palant Modified:
April 10, 2013, 9:11 a.m. Visibility:
Public. |
DescriptionAddressed FilterEngine API issues from http://codereview.adblockplus.org/10016005/
Patch Set 1 #
Total comments: 7
Patch Set 2 : Added filter access and addressed review comments #
Total comments: 5
Patch Set 3 : Replace GetElementHidingRules by a domain-specific GetElementHidingSelectors #
Total comments: 8
Patch Set 4 : Addressed review comments #
Total comments: 3
Patch Set 5 : Changed filter type enum #
MessagesTotal messages: 18
That comment and adding whitelist managing functions would be nice. http://codereview.adblockplus.org/10100009/diff/1/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/1/src/FilterEngine.cpp#newcod... src/FilterEngine.cpp:141: const std::string& documentUrl) const How about returning the filter definition rule that caused Matches to return true? ie - "the match" itself. Might be a good idea for future, and cool for debugging purposes. Also, maybe we should have some specific return value if the documentUrl has been whitelisted? We would be able to modify the icon on UI accordingly, if we knew if it is whitelisted or not.
Some small remarks, otherwise LGTM. http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:17: const std::string GetProperty(const std::string& name) const; Why return a const value here? It's copied on return anyway, const will only dictate that the caller has to store his copy in a const, which may not always make sense. http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:32: std::map<std::string,std::string> properties; We generally put a space after a comma in template parameters (although it's not fully consistent in ABP for IE). http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:33: std::map<std::string,int> intProperties; Will we need more types than string and int? We'll need a value type that supports several types for return values from JS anyway, so we should keep this in mind.
I've added an API to access custom filters (unstructured access should do for now). The Filter object is very similar to the Subscription object and I've added a common base class for both. I've also improved the API to access properties - it's always the same method name now, the type of the default value determines the property type. There is also a way to retrieve boolean properties, that should be all we need for now. Finally, FilterEngine::Matches() returns the actual filter matching and the caller can check whether it's an exception rule. http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterE... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:17: const std::string GetProperty(const std::string& name) const; On 2013/04/05 08:50:20, Felix H. Dahlke wrote: > Why return a const value here? It's copied on return anyway, const will only > dictate that the caller has to store his copy in a const, which may not always > make sense. True of course, originally I tried to return a reference. http://codereview.adblockplus.org/10100009/diff/1/include/AdblockPlus/FilterE... include/AdblockPlus/FilterEngine.h:33: std::map<std::string,int> intProperties; On 2013/04/05 08:50:20, Felix H. Dahlke wrote: > Will we need more types than string and int? Yes, we need booleans as well. Other than that subscriptions and filters only have a few structured fields that will need special treatment later if we want to use them. http://codereview.adblockplus.org/10100009/diff/1/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/1/src/FilterEngine.cpp#newcod... src/FilterEngine.cpp:141: const std::string& documentUrl) const On 2013/04/05 07:31:51, Oleksandr wrote: > How about returning the filter definition rule that caused Matches to return > true? ie - "the match" itself. Yes, I want to do that in the next step, while implementing a Filter class - this method should return a Filter reference. You could then also check whether the result is a whitelisting filter.
I've also replaced FilterEngine::GetElementHidingRules() by FilterEngine::GetElementHidingSelectors(domain) now - the latter should call ElemHide.getSelectorsForDomain() that we use in Chrome and Android already.
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); 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. http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:200: knownFilters[trimmed] = result; I was not able to understand who and when is doing (or is meant to be doing) the deletion of this Filter* result. Can you please elaborate?
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); On 2013/04/05 14:09:07, Oleksandr wrote: > 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. 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)
On 2013/04/05 14:20:59, Felix H. Dahlke wrote: > http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... > File include/AdblockPlus/FilterEngine.h (right): > > http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... > include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); > On 2013/04/05 14:09:07, Oleksandr wrote: > > 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. > > How would that be any less ambigous? Well, for one thing, if you pass an object into a function, you are responsible for its deletion. The function wouldn't give you an object out of nowhere. > > 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)
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); 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? http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:200: knownFilters[trimmed] = result; On 2013/04/05 14:09:07, Oleksandr wrote: > I was not able to understand who and when is doing (or is meant to be doing) the > deletion of this Filter* result. Can you please elaborate? The C++ wrappers are cached for the entire life time of the process - same as their JavaScript counterparts. The size of the C++ wrapper should be negligible compared to the JS object it is wrapping so I'm not sure doing something that's more smart is actually worth it.
http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); On 2013/04/05 14:31:45, Wladimir Palant wrote: > 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? 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?
http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:200: knownFilters[trimmed] = result; So I take it as no one deletes it - it is just cleaned up by the system when the process quits, correct? I personally would just put a loop in FilterEngine's destructor that would iterate through knownFilters and delete each. On 2013/04/05 14:31:45, Wladimir Palant wrote: > On 2013/04/05 14:09:07, Oleksandr wrote: > > I was not able to understand who and when is doing (or is meant to be doing) > the > > deletion of this Filter* result. Can you please elaborate? > > The C++ wrappers are cached for the entire life time of the process - same as > their JavaScript counterparts. The size of the C++ wrapper should be negligible > compared to the JS object it is wrapping so I'm not sure doing something that's > more smart is actually worth it.
Looks pretty complete by now. http://codereview.adblockplus.org/10100009/diff/10001/include/AdblockPlus/Fil... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/10001/include/AdblockPlus/Fil... include/AdblockPlus/FilterEngine.h:13: class JSObject Should be JsObject for consistency. I've thought a bit about how we could avoid exposing the fact that it's a JS object underneath to the API, but no good idea so far. I don't think it's feasible to have a general purpose PropertyMap here and keep it in sync with the actual JS object. Using JsObject by composition doesn't sound great either, with all the overloaded GetProperty functions we'd have to forward. I did try to hide the fact that all this is JS underneath from the API to keep the abstraction high so far (with the exception of JsEngine which I was planning to keep inside FilterEngine in the future). http://codereview.adblockplus.org/10100009/diff/10001/shell/src/FiltersComman... File shell/src/FiltersCommand.cpp (right): http://codereview.adblockplus.org/10100009/diff/10001/shell/src/FiltersComman... shell/src/FiltersCommand.cpp:22: AdblockPlus::FilterEngine& filterEngine) This fits in one line, no need to wrap. http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:81: SetProperty("type", "comment"); Have you considered having an enum for the type? Would mean we'd have to touch this if we added new types of course. http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:200: knownFilters[trimmed] = result; On 2013/04/05 15:14:12, Oleksandr wrote: > So I take it as no one deletes it - it is just cleaned up by the system when the > process quits, correct? I personally would just put a loop in FilterEngine's > destructor that would iterate through knownFilters and delete each. > > On 2013/04/05 14:31:45, Wladimir Palant wrote: > > On 2013/04/05 14:09:07, Oleksandr wrote: > > > I was not able to understand who and when is doing (or is meant to be doing) > > the > > > deletion of this Filter* result. Can you please elaborate? > > > > The C++ wrappers are cached for the entire life time of the process - same as > > their JavaScript counterparts. The size of the C++ wrapper should be > negligible > > compared to the JS object it is wrapping so I'm not sure doing something > that's > > more smart is actually worth it. > I'm with Oleksandr here, would prefer it if FilterEngine cleaned up after itself.
All review comments should be addressed now. http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); I am using shared_ptr instead of raw pointers now. http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#ne... src/FilterEngine.cpp:200: knownFilters[trimmed] = result; Now that this map stores shared_ptr instances cleaning up happens automatically.
Appart from Windows specific changes needed LGTM. I will commit Windows specific stuff a bit later today. (different way of including stuff. ie: #include <tr1/memory> vs #include <memory> and stuff like that). On 2013/04/08 13:51:47, Wladimir Palant wrote: > All review comments should be addressed now. > > http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... > File include/AdblockPlus/FilterEngine.h (right): > > http://codereview.adblockplus.org/10100009/diff/6001/include/AdblockPlus/Filt... > include/AdblockPlus/FilterEngine.h:95: const std::string& documentUrl); > I am using shared_ptr instead of raw pointers now. > > http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp > File src/FilterEngine.cpp (right): > > http://codereview.adblockplus.org/10100009/diff/10001/src/FilterEngine.cpp#ne... > src/FilterEngine.cpp:200: knownFilters[trimmed] = result; > Now that this map stores shared_ptr instances cleaning up happens automatically.
http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/Fil... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/Fil... include/AdblockPlus/FilterEngine.h:7: #include <tr1/memory> As of GCC 4.6 (we can assume that on Android with a recent NDK), #include <memory> should work as well. No Windows-specific changes necessary then. http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/Fil... include/AdblockPlus/FilterEngine.h:46: enum FilterType {BLOCKING_RULE, EXCEPTION_RULE, I'd move this into the public section of Filter, so that we have Filter::Type. Also, it's somewhat conventional to prefix enum values with their type name, avoids polluting the namespace. So if we leave it here, FILTER_TYPE_BLOCKING, if we move it into Filter, TYPE_BLOCKING (i.e. Filter::TYPE_BLOCKING).
I changed the filter type enum. As to the tr1/memory include, Oleksandr is already taking care of it. http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/Fil... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/Fil... include/AdblockPlus/FilterEngine.h:7: #include <tr1/memory> On 2013/04/08 15:03:59, Felix H. Dahlke wrote: > As of GCC 4.6 (we can assume that on Android with a recent NDK), #include > <memory> should work as well. No Windows-specific changes necessary then. Not sure about the NDK but on Linux #include <memory> definitely doesn't work.
On 2013/04/09 05:56:34, Wladimir Palant wrote: > I changed the filter type enum. As to the tr1/memory include, Oleksandr is > already taking care of it. > > http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/Fil... > File include/AdblockPlus/FilterEngine.h (right): > > http://codereview.adblockplus.org/10100009/diff/23001/include/AdblockPlus/Fil... > include/AdblockPlus/FilterEngine.h:7: #include <tr1/memory> > On 2013/04/08 15:03:59, Felix H. Dahlke wrote: > > As of GCC 4.6 (we can assume that on Android with a recent NDK), #include > > <memory> should work as well. No Windows-specific changes necessary then. > > Not sure about the NDK but on Linux #include <memory> definitely doesn't work. With GCC 4.6, it should. But we may want to support earlier GCC versions anyway, so it's fine I guess.
Oh, and LGTM :) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
