Opened 3 years ago

Last modified 2 years ago

#1285 new change

Disable automatic update checks by default and add API for enabling them

Reported by: fhd Assignee:
Priority: P2 Milestone:
Module: Libadblockplus Keywords: large-scale-deployments
Cc: trev, simona Blocked By:
Blocking: #542, #1319 Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by fhd)

Background

Libadblockplus is currently running regular update checks for the application (with data from AppInfo) in the background. We've added this for practical reasons: Since all of our own Libadblockplus clients (Android and IE) need update functionality. However, other Libadblockplus clients do not necessarily want to use this update mechanism, so it should be enabled explicitly.

What to change

  1. Remove the update_url_release and devbuild_update_url prefs we set in Libadblockplus.
  2. Remove AppInfo::developmentBuild - Libadblockplus doesn't need to know anymore.
  3. Add a setter for the update URL to FilterEngine. Update checks should only happen if an URL has been set here, i.e. not by default.

Change History (18)

comment:1 Changed 3 years ago by fhd

  • Priority changed from P3 to P2

comment:2 Changed 3 years ago by trev

IMHO, a simpler option is to have a flag in AppInfo.

Edit: Thinking a bit more about it - probably not, that structure is being passed to JsEngine and is supposed to be generic. However, the application can remove update_url_release and update_url_devbuild preferences to indicate that it doesn't want updates.

Last edited 3 years ago by trev (previous) (diff)

comment:3 Changed 3 years ago by fhd

How about this then:

  1. We remove the default update URLs from lib/prefs.js
  2. We add explicit setters for both update URLs

That way, updates will be disabled by default, and clients can enable release and devbuild update checks as they see fit.

That'll do for now. The remaining problem (more of a <=P3 thing) is that, IMO, FilterEngine just shouldn't be responsible for application update checks to begin with - they have nothing to do with "filters". I suppose it would be fine again if we rename it to "AdblockPlusEngine" or "AbpEngine" or something along those lines - but that wouldn't make it any less convoluted.

I think it would be cleaner to have a separate Updater component. That's non-trivial because it would use the same code FilterEngine uses under the hood, we'll probably need a shared base class for anything that wants to interact with the ABP code. But it would be pretty clean. What do you think, Wladimir?

comment:4 Changed 3 years ago by trev

We add explicit setters for both update URLs

Then we don't need two update URLs - if the application sets them anyway then it can do the devbuild/release check itself, and we don't need to store the URLs in prefs either. I guess having the application explicitly enable automatic updates is indeed a better approach...

I think it would be cleaner to have a separate Updater component.

This is indeed non-trivial. I don't think that a shared base class is a good idea. We rather need an internal Loader component that will be used by both FilterEngine and Updater. It should be able to detect whether the JS files have been loaded already (e.g. by checking whether the API variable exists) - if not, it should load them. This should allow FilterEngine and Updater to be simple API wrappers.

comment:5 Changed 3 years ago by fhd

Yeah, sounds good. Would you agree that having Updater and FilterEngine as separate components would be cleaner though?

comment:6 Changed 3 years ago by fhd

  • Description modified (diff)

comment:7 Changed 3 years ago by trev

Yes, ideally we would have separate objects encapsulate different sets of functionality - FilterEngine is starting to get crowded. A separate Updater component would be a start.

comment:8 Changed 3 years ago by fhd

Good good, here goes the follow-up: #1319.

comment:9 Changed 3 years ago by fhd

  • Blocking 1319 added

comment:10 Changed 3 years ago by fhd

  • Summary changed from Make it possible to disable automatic update checks to Disable automatic update checks by default and add API for enabling them

comment:11 Changed 2 years ago by simona

  • Cc simona added
  • Keywords large-scale-deployments added

comment:12 Changed 2 years ago by simona

  • Blocking 1605 added

comment:13 Changed 2 years ago by oleksandr

  • Blocking 542 added

comment:14 Changed 2 years ago by oleksandr

  • Owner set to oleksandr

comment:15 Changed 2 years ago by oleksandr

  • Tester set to Unknown

The functionality to support this was implemented differently that is described here. See https://codereview.adblockplus.org/5653480979038208/ I think this issue can be closed.

comment:16 Changed 2 years ago by oleksandr

  • Blocking 1605 removed

comment:17 Changed 2 years ago by trev

No, not really the same thing. This issue is about having applications using libadblockplus opt into update checks - only when the application can actually deal with updates. This isn't about user configuration.

comment:18 Changed 2 years ago by oleksandr

  • Owner oleksandr deleted
Note: See TracTickets for help on using tickets.