Issue 6559 - Use Map object for known subscriptions

Note: This will also require changes in adblockplusui.

[Manish Jethani, 2018-06-06 11:04:43]

Patch Set 1
Patch Set 2: Avoid lookup for disabled subscriptions
Patch Set 3: Move check for known subscription into removeSubscription

https://codereview.adblockplus.org/29800557/diff/29800568/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29800557/diff/29800568/lib/filterListener....
lib/filterListener.js:257: if (!subscription.disabled &&
Better to check if the subscription is disabled first so we avoid the lookup.

https://codereview.adblockplus.org/29800557/diff/29800568/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29800557/diff/29800568/lib/filterStorage.j...
lib/filterStorage.js:143: if
(!FilterStorage.knownSubscriptions.has(subscription.url))
IMO it makes more sense to do the check here rather than in lib/synchronizer.js

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

https://codereview.adblockplus.org/29800557/diff/29800568/lib/subscriptionCla...
lib/subscriptionClasses.js:170: let subscription =
Subscription.knownSubscriptions.get(url);
No need to call both Map.has and Map.get, only the latter is enough since values
are never null/undefined.

[kzar, 2018-06-07 13:52:26]

https://codereview.adblockplus.org/29800557/diff/29800568/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29800557/diff/29800568/lib/filterListener....
lib/filterListener.js:257: if (!subscription.disabled &&
On 2018/06/06 11:04:42, Manish Jethani wrote:
> Better to check if the subscription is disabled first so we avoid the lookup.

Acknowledged.

https://codereview.adblockplus.org/29800557/diff/29800568/lib/synchronizer.js
File lib/synchronizer.js (left):

https://codereview.adblockplus.org/29800557/diff/29800568/lib/synchronizer.js...
lib/synchronizer.js:204: if (listed)
But we already have `listed` here, but now we don't use that and perform the
lookup again in the FilterStorage code? I don't care too much either way, but I
guess I don't understand your reasoning so far.

[Manish Jethani, 2018-06-07 14:29:46]

Back to Patch Set 2

https://codereview.adblockplus.org/29800557/diff/29800568/lib/synchronizer.js
File lib/synchronizer.js (left):

https://codereview.adblockplus.org/29800557/diff/29800568/lib/synchronizer.js...
lib/synchronizer.js:204: if (listed)
On 2018/06/07 13:52:26, kzar wrote:
> But we already have `listed` here, but now we don't use that and perform the
> lookup again in the FilterStorage code? I don't care too much either way, but
I
> guess I don't understand your reasoning so far.

Yeah I have to admit the last change is not exactly related, let's leave it out
for now (so let's focus on Patch Set 2).

I did it because it made sense that both addSubscription and removeSubscription
would do the same type of check, but let's leave it out. Ideally
removeSubscription would return true if the subscription was actually removed,
which would become the value of "listed" here (so no additional lookup).

[kzar, 2018-06-07 14:35:47]

Patch Set 2 LGTM