Issue 7094 - Encapsulate management of subscription filters

[Manish Jethani, 2018-11-17 21:41:09]

Patch Set 3

Patch Set 1 was different, Patch Set 2 including the concept of "lightweight"
filters which I have now removed (see the diff) but will reintroduce in the
appropriate follow-up patch, and now I have also added a hasFilter() method to
use in https://codereview.adblockplus.org/29935568/

https://codereview.adblockplus.org/29934588/diff/29945555/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29934588/diff/29945555/lib/filterStorage.j...
lib/filterStorage.js:207: connectSubscriptionFilters(subscription, filters);
The reason we're passing the filters array to the method here is because we
don't know if the subscription will hold on to the filter objects. Right now it
does hold on to them, but in subsequent patches we'll make some of the objects
"ephemeral" so the subscription will simply keep the text and discard the
object.

https://codereview.adblockplus.org/29934588/diff/29945555/lib/filterStorage.j...
lib/filterStorage.js:262: let subscriptions = (
Note: Both removeFilter() and moveFilter() can probably rewritten in a better
way, but I want to minimize the changes in this patch. Much of the code here
should actually be in the Subscription class, now that we're encapsulating
everything, but it is not strictly necessary for what we're trying to accomplish
with these changes.

https://codereview.adblockplus.org/29934588/diff/29945555/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29934588/diff/29945555/lib/subscriptionCla...
lib/subscriptionClasses.js:194: return this._filterText.indexOf(filter.text,
fromIndex);
Note: We look up a filter always by the text, not by the filter object, which
can be temporary and so the references can keep changing.

https://codereview.adblockplus.org/29934588/diff/29945555/lib/subscriptionCla...
lib/subscriptionClasses.js:202: hasFilter(filter)
We will use this in this patch: https://codereview.adblockplus.org/29935568/

https://codereview.adblockplus.org/29934588/diff/29945555/test/synchronizer.js
File test/synchronizer.js (right):

https://codereview.adblockplus.org/29934588/diff/29945555/test/synchronizer.j...
test/synchronizer.js:144: test.deepEqual([...subscription.filterText()], ["foo",
"!bar", "@@bas", "#bam"], "Resulting subscription filters");
Note: No need to create filter object, we just compare by text here. It's easier
to read.

[Manish Jethani, 2018-11-17 22:16:12]

Patch Set 4: Pass old filters to disconnectSubscriptionFilters

[Manish Jethani, 2018-11-17 23:41:12]

Patch Set 5: Add tests
Patch Set 6: Update tests

[Manish Jethani, 2018-11-18 02:25:51]

Patch Set 7: Lazy initialize and clean up filter text lookup

https://codereview.adblockplus.org/29934588/diff/29945605/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29934588/diff/29945605/lib/filterStorage.j...
lib/filterStorage.js:213: subscription.clearCaches();
Typically a subscription will be updated with multiple new filters. In this
case, when the "subscription.updated" event is handled by filterListener.js, it
will end up populating the _filterTextLookup property of the Subscription
instance. After the event has been handled, we clear the lookup here.

https://codereview.adblockplus.org/29934588/diff/29945605/lib/filterStorage.j...
lib/filterStorage.js:399: clearSubscriptionCaches();
This is not necessary right now but better to do it. It will clear the
"ephemeral" Filter instances from memory, when we finally get to it.

https://codereview.adblockplus.org/29934588/diff/29945605/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29934588/diff/29945605/lib/subscriptionCla...
lib/subscriptionClasses.js:202: if (!this._filterTextLookup)
We could not afford to have this lookup in memory all the time, it alone takes
up ~3 MB. Instead we initialize it when needed. Afterwards, it can be cleared up
by calling the clearCaches() method.

https://codereview.adblockplus.org/29934588/diff/29945605/test/subscriptionCl...
File test/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29934588/diff/29945605/test/subscriptionCl...
test/subscriptionClasses.js:81: subscription.clearCaches();
Test again after clearing caches.

[Manish Jethani, 2018-11-18 02:28:10]

Patch Set 8: Update JSDoc

[Manish Jethani, 2018-11-18 22:23:12]

Patch Set 9: Remove hasFilter and related code

hasFilter() is not needed until we do the next patch, this one is already quite
big, so I'll just remove it for now.

[hub, 2018-11-19 17:47:50]

https://codereview.adblockplus.org/29934588/diff/29946561/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29934588/diff/29946561/lib/subscriptionCla...
lib/subscriptionClasses.js:69: _filterText: null,
shouldn't this be named `_filtersText` since it is many filters?

Also would be consistent with `_filters`

[Manish Jethani, 2018-11-20 00:19:39]

https://codereview.adblockplus.org/29934588/diff/29946561/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29934588/diff/29946561/lib/subscriptionCla...
lib/subscriptionClasses.js:69: _filterText: null,
On 2018/11/19 17:47:49, hub wrote:
> shouldn't this be named `_filtersText` since it is many filters?
> 
> Also would be consistent with `_filters`

I thought about calling it _filtersText but that sounds wrong in English. Maybe
_filterTextList or _filterTextArray? Though I find it clear enough to just call
it _filterText, since text is both singular and plural and it maps nicely to the
generator function filterText()

[hub, 2018-11-20 19:30:19]

LGTM

https://codereview.adblockplus.org/29934588/diff/29946561/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29934588/diff/29946561/lib/subscriptionCla...
lib/subscriptionClasses.js:69: _filterText: null,
On 2018/11/20 00:19:39, Manish Jethani wrote:
> On 2018/11/19 17:47:49, hub wrote:
> > shouldn't this be named `_filtersText` since it is many filters?
> > 
> > Also would be consistent with `_filters`
> 
> I thought about calling it _filtersText but that sounds wrong in English.
Maybe
> _filterTextList or _filterTextArray? Though I find it clear enough to just
call
> it _filterText, since text is both singular and plural and it maps nicely to
the
> generator function filterText()

my bad. let's leave it at that.