Opened on 08/29/2014 at 09:34:10 AM

Closed on 10/25/2018 at 09:48:01 AM

#1285 closed change (fixed)

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: 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)


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.

Attachments (0)

Change History (20)

comment:1 Changed on 08/29/2014 at 09:36:11 AM by fhd

  • Priority changed from P3 to P2

comment:2 Changed on 08/29/2014 at 09:45:24 PM 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 on 08/29/2014 at 09:48:11 PM by trev

comment:3 Changed on 09/02/2014 at 05:54:58 AM 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 on 09/02/2014 at 01:36:46 PM 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 on 09/02/2014 at 01:41:15 PM by fhd

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

comment:6 Changed on 09/02/2014 at 01:49:52 PM by fhd

  • Description modified (diff)

comment:7 Changed on 09/02/2014 at 02:55:50 PM 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 on 09/03/2014 at 12:56:12 PM by fhd

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

comment:9 Changed on 09/03/2014 at 12:56:31 PM by fhd

  • Blocking 1319 added

comment:10 Changed on 09/03/2014 at 12:57:44 PM 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 on 02/20/2015 at 02:35:27 PM by simona

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

comment:12 Changed on 02/20/2015 at 02:38:06 PM by simona

  • Blocking 1605 added

comment:13 Changed on 03/27/2015 at 02:49:22 PM by oleksandr

  • Blocking 542 added

comment:14 Changed on 06/03/2015 at 02:53:51 PM by oleksandr

  • Owner set to oleksandr

comment:15 Changed on 07/07/2015 at 10:42:54 PM by oleksandr

  • Tester set to Unknown

The functionality to support this was implemented differently that is described here. See I think this issue can be closed.

comment:16 Changed on 07/07/2015 at 10:44:23 PM by oleksandr

  • Blocking 1605 removed

comment:17 Changed on 07/09/2015 at 09:48:02 AM 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 on 08/12/2015 at 01:20:37 AM by oleksandr

  • Owner oleksandr deleted

comment:19 Changed on 12/21/2017 at 11:25:42 AM by fhd

  • Cc trev removed

comment:20 Changed on 10/25/2018 at 09:48:01 AM by sergz

  • Resolution set to fixed
  • Status changed from new to closed

Done as a part of refactoring.

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
Note: See TracTickets for help on using tickets.