Opened 3 years ago

Closed 3 years ago

#4612 closed change (fixed)

Add functionality enabling AA subscription into libadblockplus

Reported by: sergz Assignee: sergz
Priority: P2 Milestone:
Module: Libadblockplus Keywords:
Cc: asmirnov, fhd, oleksandr Blocked By:
Blocking: #4604 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29363607/

Description

Background

This issue arose during discussion of #4604.

Why in libadblockplus and not in core

I think that adblockpluscore is a generic core without any presumable subscriptions, that's why subscription URLs and the adding logic are duplicated in each extension and in libadblockplus. Next, libadblockplus chooses and adds the default language subscription and it has URL of acceptable ads subscription in its pref.js, so it seems legit to add AA subscription in libadblockplus on the first run when a default subscription corresponding to the current locale is being added.

Why

It should prevent the users of libadblockplus from implementing the same functionality each time and solves some known issues, like possible race conditions for instance.

How

However, I would like to have it optional, disabled by default and enabled through a constructor argument of FilterEngine to avoid affecting of tests. Currently they are already at certain degree affected by default language subscription, though.

What to change

  • add adding of AA subscriptions in init.js where we choose and add default subscription
  • add conditions which enable mentioned above functionality in accordance with corresponding settings
  • define what settings should be passed through preconfiguredPrefs argument of FilterEngine constructor and define default values in prefs.js
  • check which tests can be simplified and make required adjustments of all affected tests.

Change History (10)

comment:1 Changed 3 years ago by sergz

  • Owner set to sergz

comment:2 Changed 3 years ago by fhd

Yeah, makes sense to have this in libadblockplus rather than duplicate the behaviour in all clients.

But while at it, I actually think we should deal with the automatic language subscription selection, too, in the same way. But I think the default should be automatic subscription selection, if people want different behaviour they can change it, and we can change it in the tests where we need to.

And also, while at it: It would be handy to have API for enabling/disabling AA, too.

comment:3 Changed 3 years ago by sergz

Sorry, it was unclear. That's actually what I meant, beside AA stuff, to have a property which disables both functionalities by default to avoid any side-effects in tests. But maybe you are right, for users it would be better to have everything enabled by default.

Adding API to manage AA seems to be a bit more complicated, maybe another issue?

  • FilterEngine::SetAAEnabled(bool)
  • FilterEngine::IsAAEnabled()
  • Subscription::IsAA() it should allow to find AA subscription among items returned by FilterEngine::GetListedSubscriptions
  • FilterEngine::GetAAURL() it seems not an ideal way but already better than to ask user of libadblockplus to read hard-coded property subscriptions_exceptionsurl.

comment:4 Changed 3 years ago by fhd

Yeah, can be a separate issue. But this is one of the things we keep duplicating that bothers me :P (I doubt we have a use case for GetAAURL, I think all we use that URL for is to check if AA is enabled etc, which would be covered by the new API functions.)

comment:5 Changed 3 years ago by sergz

If AA is not in the list of subscriptions then there is no way to show to user the URL and make a link to AA on an options page.

comment:6 Changed 3 years ago by fhd

Hm, true I guess. Then we might not need IsAA though, but both wouldn't hurt.

comment:7 Changed 3 years ago by sergz

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 Changed 3 years ago by asmirnov

  • Blocking 4604 added

comment:10 Changed 3 years ago by sergz

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.