Issue 5019 - Fixes anti adblock notification not being added on first run

Issue 5019

[Sebastian Noack, 2017-03-21 10:25:34]

LGTM as far as I understand the logic (see my comment on the issue). But this
should be reviewed as well by Thomas.

[Thomas Greiner, 2017-03-21 11:14:19]

That makes me wonder why the same logic is implemented in two different ways.

i.e.
if (subscription.lastDownload && subscription.disabled)
if (url in FilterStorage.knownSubscriptions && !subscription.disabled)

The change in this review seems to make sense but why not just make this
consistent to avoid further issues with inconsistent behavior in the future?
After all it doesn't seem necessary to check whether the filter list is in
`FilterStorage.knownSubscriptions` but it is necessary to check whether the
filter list was successfully downloaded.

[Sebastian Noack, 2017-03-21 11:34:20]

On 2017/03/21 11:14:19, Thomas Greiner wrote:
> That makes me wonder why the same logic is implemented in two different ways.
> 
> i.e.
> if (subscription.lastDownload && subscription.disabled)
> if (url in FilterStorage.knownSubscriptions && !subscription.disabled)
> 
> The change in this review seems to make sense but why not just make this
> consistent to avoid further issues with inconsistent behavior in the future?
> After all it doesn't seem necessary to check whether the filter list is in
> `FilterStorage.knownSubscriptions` but it is necessary to check whether the
> filter list was successfully downloaded.

As far as I remember (please correct me if wrong)
FilterStorage.knownSubscriptions is a persistent cache that leaks every
subscription that ever have been known at any moment during the run time of
Adblock Plus. So it seems the check for |url in
FilterStorage.knownSubscriptions| will always return true here, hence is
redundant?

However, the same handler is called when the subscription is updated, disabled
or removed. But when the subscription is removed, downloadCount and
downloadStatus might still indicate that it has been downloaded. So we have to
additionally check whether it is subscribed. I suppose this was the original
intention of the |url in FilterStorage.knownSubscriptions| check, which however
as far as I understand, never worked as intended?

[Thomas Greiner, 2017-03-21 12:11:59]

On 2017/03/21 11:34:20, Sebastian Noack wrote:
> As far as I remember (please correct me if wrong)
> FilterStorage.knownSubscriptions is a persistent cache that leaks every
> subscription that ever have been known at any moment during the run time of
> Adblock Plus.

`filterClasses.knownFilters` is a cache of all filters we have encountered.
`FilterStorage.knownSubscriptions`, however, is a list of all filter lists that
the user has currently installed.

> However, the same handler is called when the subscription is updated, disabled
> or removed. But when the subscription is removed, downloadCount and
> downloadStatus might still indicate that it has been downloaded. So we have to
> additionally check whether it is subscribed.

Good point. For adding the notification we only need to know whether it's
downloaded and disabled but for removing it we need to know whether it's
installed and disabled.

LGTM