Add API calls and stubs for filter matching and subscription management

[Wladimir Palant, 2013-04-03 13:42:41]

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:11: std::string title;
We'll need to expose many more subscription properties like last download time.
I wonder whether it's better to just wrap a JS object here and provide a
GetProperty() method (and a corresponding SetPropery() method to set things like
title). It will also make converting from the C++ object to the JS object
simpler.

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:22: const Subscription*
FindSubscription(const std::string& url) const;
This should be called GetSubscription() - ABP code will generally create a new
subscription object for a URL if there isn't one yet, it allows doing something
with the subscription before actually adding it.

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:25: std::vector<Subscription>
FetchAvailableSubscriptions();
This is usually an asynchronous action (via XMLHttpRequest), we probably want to
work with callbacks here.

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:27: const std::string& contentType) const;
We need to pass documentUrl as well here at the very least - this is required to
handle whitelisting and domain-specific filters correctly.

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:34: }
General note: maybe Subscription should be a class that only FilterEngine can
create, via GetSubscription(). It would have a private constructor and get the
JS engine as a parameter there. Things like UpdateFilters() could be methods of
the Subscription class then, also AddToList() and RemoveFromList() - so you
wouldn't have to list each method working with subscriptions in FilterEngine.

[Felix Dahlke, 2013-04-04 05:10:51]

All valid points, some further detail below.

Generally, my intention was to keep this as simple as possible and add things as
we need them, because I was a bit worried I might overengineer this in the wrong
direction otherwise.

Since Oleksandr has begun to integrate it and you're back now, we can do things
properly. Actually, Wladimir, would you mind taking this over? I'm still in the
middle of other things and I'd rather not be a bottleneck.

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:11: std::string title;
On 2013/04/03 13:42:41, Wladimir Palant wrote:
> We'll need to expose many more subscription properties like last download
time.
> I wonder whether it's better to just wrap a JS object here and provide a
> GetProperty() method (and a corresponding SetPropery() method to set things
like
> title). It will also make converting from the C++ object to the JS object
> simpler.

Sure, sounds reasonable. My intention was to go with the bare minimum we need on
the UI for now and expand as needed, but a general GetPropery mechanism would
allow for that as well.

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:22: const Subscription*
FindSubscription(const std::string& url) const;
On 2013/04/03 13:42:41, Wladimir Palant wrote:
> This should be called GetSubscription() - ABP code will generally create a new
> subscription object for a URL if there isn't one yet, it allows doing
something
> with the subscription before actually adding it.

Oh, didn't notice. Sure :)

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:25: std::vector<Subscription>
FetchAvailableSubscriptions();
On 2013/04/03 13:42:41, Wladimir Palant wrote:
> This is usually an asynchronous action (via XMLHttpRequest), we probably want
to
> work with callbacks here.

Yeah, not sure why I didn't do that actually...

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:27: const std::string& contentType) const;
On 2013/04/03 13:42:41, Wladimir Palant wrote:
> We need to pass documentUrl as well here at the very least - this is required
to
> handle whitelisting and domain-specific filters correctly.

You're right, it's quite lacking. I had the idea to go with the absolute bare
minimum for now, and expand as needed (not being entirely sure yet what kind of
information we can actually get), but we can just wrap it properly right now.

http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
include/AdblockPlus/FilterEngine.h:34: }
On 2013/04/03 13:42:41, Wladimir Palant wrote:
> General note: maybe Subscription should be a class that only FilterEngine can
> create, via GetSubscription(). It would have a private constructor and get the
> JS engine as a parameter there. Things like UpdateFilters() could be methods
of
> the Subscription class then, also AddToList() and RemoveFromList() - so you
> wouldn't have to list each method working with subscriptions in FilterEngine.

Yes, that would be a nicer design. As I noted before, my intention was to keep
all this as simple as possible for now, but there's no need to now that you're
back and Oleksandr has begun to integrate it.

[Wladimir Palant, 2013-04-04 06:28:58]

On 2013/04/04 05:10:51, Felix H. Dahlke wrote:
> Since Oleksandr has begun to integrate it and you're back now, we can do
things
> properly. Actually, Wladimir, would you mind taking this over? I'm still in
the
> middle of other things and I'd rather not be a bottleneck.

Sure, I meant to work on the JS side of it anyway.

> 
>
http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
> File include/AdblockPlus/FilterEngine.h (right):
> 
>
http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
> include/AdblockPlus/FilterEngine.h:11: std::string title;
> On 2013/04/03 13:42:41, Wladimir Palant wrote:
> > We'll need to expose many more subscription properties like last download
> time.
> > I wonder whether it's better to just wrap a JS object here and provide a
> > GetProperty() method (and a corresponding SetPropery() method to set things
> like
> > title). It will also make converting from the C++ object to the JS object
> > simpler.
> 
> Sure, sounds reasonable. My intention was to go with the bare minimum we need
on
> the UI for now and expand as needed, but a general GetPropery mechanism would
> allow for that as well.
> 
>
http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
> include/AdblockPlus/FilterEngine.h:22: const Subscription*
> FindSubscription(const std::string& url) const;
> On 2013/04/03 13:42:41, Wladimir Palant wrote:
> > This should be called GetSubscription() - ABP code will generally create a
new
> > subscription object for a URL if there isn't one yet, it allows doing
> something
> > with the subscription before actually adding it.
> 
> Oh, didn't notice. Sure :)
> 
>
http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
> include/AdblockPlus/FilterEngine.h:25: std::vector<Subscription>
> FetchAvailableSubscriptions();
> On 2013/04/03 13:42:41, Wladimir Palant wrote:
> > This is usually an asynchronous action (via XMLHttpRequest), we probably
want
> to
> > work with callbacks here.
> 
> Yeah, not sure why I didn't do that actually...
> 
>
http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
> include/AdblockPlus/FilterEngine.h:27: const std::string& contentType) const;
> On 2013/04/03 13:42:41, Wladimir Palant wrote:
> > We need to pass documentUrl as well here at the very least - this is
required
> to
> > handle whitelisting and domain-specific filters correctly.
> 
> You're right, it's quite lacking. I had the idea to go with the absolute bare
> minimum for now, and expand as needed (not being entirely sure yet what kind
of
> information we can actually get), but we can just wrap it properly right now.
> 
>
http://codereview.adblockplus.org/10016005/diff/2001/include/AdblockPlus/Filt...
> include/AdblockPlus/FilterEngine.h:34: }
> On 2013/04/03 13:42:41, Wladimir Palant wrote:
> > General note: maybe Subscription should be a class that only FilterEngine
can
> > create, via GetSubscription(). It would have a private constructor and get
the
> > JS engine as a parameter there. Things like UpdateFilters() could be methods
> of
> > the Subscription class then, also AddToList() and RemoveFromList() - so you
> > wouldn't have to list each method working with subscriptions in
FilterEngine.
> 
> Yes, that would be a nicer design. As I noted before, my intention was to keep
> all this as simple as possible for now, but there's no need to now that you're
> back and Oleksandr has begun to integrate it.

[Wladimir Palant, 2013-04-04 17:09:51]

I created http://codereview.adblockplus.org/10100009/ for my changes, this
review can be closed now.