Remove FilterStorage.moveSubscription

[mjethani]

Background

It seems that the FilterStorage.moveSubscription method is not used at all anywhere in the browser extension, but because of it we must maintain a separate FilterStorage.subscriptions array just so the subscriptions can be ordered. If this functionality is not being used from any of the clients of Adblock Plus core, it should be removed. This would allow us to get rid of FilterStorage.subscriptions and just work with FilterStorage.knownSubscriptions (less book-keeping).

What to change

In lib/filterStorage.js:

• Remove the FilterStorage.subscriptions array and use the values of the FilterStorage.knownSubscriptions ordered map instead
• Add a new FilterStorage.subscriptions() generator function that yields the values in the FilterStorage.knownSubscriptions ordered map
• Add a new FilterStorage.subscriptionCount property (getter function) that returns the size of the FilterStorage.knownSubscriptions ordered map

Everywhere else:

• Replace all usage of the FilterStorage.subscriptions array with the new FilterStorage.subscriptions() function and the new FilterStorage.subscriptionCount property

Finally, remove FilterStorage.moveSubscription from lib/filterStorage.js and all tests; also stop listening for the subscription.moved event in lib/filterListener.js.

Integration notes

After change set ​3c14ad288359 the subscriptions property of FilterStorage has been replaced by a generator method (yields Subscription objects of knownSubscriptions) and adds a subscriptionCount property.

In adblockpluschrome there would have to be changes in lib/hitLogger.js, lib/indexedDBBackup.js, and lib/subscriptionInit.js.

In adblockplusui the changes would be in messageResponder.js, and possibly elsewhere.

Note that the subscriptions property still exists, it is just a generator function now, which means code like FilterStorage.subscriptions.length != 0 will still work silently but it will still be a bug.

Hints for testers

This change touches every part of the FilterStorage module, so it should tested thoroughly.

Try this:

1. Subscribe to a filter list. Expected: Filters from the list should work.
2. Disable the filter list. Expected: Filters from the list should no longer work.
3. Re-enable the filter list. Expected: Filters from the list should work again.
4. Unsubscribe from (delete) the filter list. Expected: Filters from the list should no longer work.
5. Re-subscribe to the same filter list. Expected: Filters from the list should work again.
6. Disable the filter list. Expected: Filters from the list should no longer work.
7. Unsubscribe from (delete) the filter list. Expected: Filters from the list should still no longer work.
8. Re-subscribe to the same filter list. Expected: Filters from the list should work again, i.e. the list should not be disabled by default (in other words, it should not remember its last enabled/disabled state).

Through all of the steps above, any other filters from any other list subscribed to and enabled should always work.

Look for any errors/exceptions in the background page console.

[mjethani]

@greiner @sebastian @sergz @Anton is it OK to remove this functionality from core?

[jsonesen]

Is this something I can start on?

[mjethani]

@jsonesen I think we'll need feedback from @sebastian and @greiner before deciding to remove this function. Some features are planned for the future that may need this functionality.

[greiner]

We don't use this functionality in the UI at the moment. While it might come in handy to allow users to manually order filter groups in the future, I'm sure we'd be able to find a different approach to accomplish this. Therefore I'd be fine with removing FilterStorage.moveSubscription().

[mjethani]

It looks like there's no objection to removing this method and the related data structure, in which case we'll go ahead and remove it.

[sebastian]

It seems if we remove FilterStorage.moveSubscription(), we should also stop listening for subscription.moved in lib/filterListener.js (and elsewhere, in case we do).

[jsonesen]

It seems that the INIParser in lib/iniParser/js also has a list of subscriptions similar to FilterStorage. Should this and all of its uses be removed? or Replaced? I ask since FilterStorage uses it to load stored subscriptions.

[mjethani]

Jon, if you're referring to the subscriptions property of the INIParser class, let's leave it as an array. Adding an item to an array is O(1) and we really need it there. The duplicates get filtered out when we populate the local knownSubscriptions object in importData.

[mjethani]

This change is in review, and it occurred to me that one of the implications of this change is that a patterns.ini containing multiple entries for a subscription will only be read as having a single entry. The only thing that this may have any implications for is the UI, because even with multiple entries, each entry would have all the filters for the subscription (so two entries A, B, C and D, E, F would be merged into the same Subscription object A, B, C, D, E, F), which means this already had no implications for ad blocking.

Because of #6858, we will still end up with multiple SpecialSubscription objects serialized and deserialized, but each one has a unique URL so they are not the same subscription.

So again, the only implications are for UI.

Question to Thomas, Arthur, Sebastian:

• Does the UI support, or has plans to support, adding the same subscription more than once? (I don't see why, but still asking)
• Did the UI ever explicitly support (not just because it just worked that way) adding the same subscription more than once?
• Does the UI do any explicit check to avoid adding the same subscription more than once?

This is a different question than whether subscriptions should be movable within the list of subscriptions.

The definition of "same subscription" here is two subscriptions with the same URL.

[mjethani]

Replying to mjethani:

• Does the UI support, or has plans to support, adding the same subscription more than once? (I don't see why, but still asking)

So I looked at the current behavior, it seems you can try to add the same subscription again but "nothing happens". Suggestion for UI: at least when a subscription is added, the item in the list could perhaps be highlighted briefly (something like a ​yellow fade).

• Did the UI ever explicitly support (not just because it just worked that way) adding the same subscription more than once?

This is not so important actually.

• Does the UI do any explicit check to avoid adding the same subscription more than once?

The answer appears to be no.

So the change here is only going to affect how an existing patterns.ini is going to be read, which also probably includes a change in how it will be displayed in the UI (duplicate entries will not appear now).

It's for the best. I'll just make a note in the issue description about this.

[greiner]

Replying to mjethani:

• Does the UI support, or has plans to support, adding the same subscription more than once? (I don't see why, but still asking)

I can't think of a reason why either. We use the URL as a unique identifier for subscriptions since the communication between the extension and the UI is serialized. Therefore having a second subscription that uses the same URL will essentially lead to undefined behavior.

• Did the UI ever explicitly support (not just because it just worked that way) adding the same subscription more than once?

Not that I'm aware of. Presumably only Wladimir would know.

• Does the UI do any explicit check to avoid adding the same subscription more than once?

No. If the UI gets made aware of another subscription with the same URL as one that it already encountered, it will treat it as both being the same.

So I looked at the current behavior, it seems you can try to add the same subscription again but "nothing happens". Suggestion for UI: at least when a subscription is added, the item in the list could perhaps be highlighted briefly (something like a ​yellow fade).

Feel free to create a ticket to suggest that because more detailed information on why it would be useful and how you'd expect it to behave would be helpful.

[mjethani]

Replying to greiner:

Replying to mjethani:
[...]

Thanks for the clarifications.

So I looked at the current behavior, it seems you can try to add the same subscription again but "nothing happens". Suggestion for UI: at least when a subscription is added, the item in the list could perhaps be highlighted briefly (something like a ​yellow fade).

Feel free to create a ticket to suggest that because more detailed information on why it would be useful and how you'd expect it to behave would be helpful.

Sure, let me go ahead and finally create that GitLab account I've been meaning to :)

[abpbot]

A commit referencing this issue has landed:
​Issue 6856 - Remove FilterStorage.moveSubscription

[mjethani]

Jon, could you please add an "Integration notes" section in the description (you can see #6916 for reference)? We need to go through both adblockpluschrome and adblockplusui and point out any changes they might need to make.

[greiner]

For reference, I've created #7027 for UI-related changes.

[abpbot]

A commit referencing this issue has landed:
​Issue 6856 - Remove FilterStorage.moveSubscription