557849 - Make Addon.updateAutomatically persist

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is that the global preference to allow automatic updates?

Anything we should test manually on that end?

Comment 2

[Dave Townsend [:mossop]]

This is the per-add-on way to opt-out of automatic updates. It's probably easiest for the background part of this functionality to be tested with an automated test, but we'll probably need some litmus coverage once the UI for checking for updates is finalised.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This makes the updateAutomatically property persist into the database and adds tests that it remains the same across add-on updates and makes the background update check only install updates when allowed to.

Comment 4

[Dave Townsend [:mossop]]

Landed on branch: http://hg.mozilla.org/projects/addonsmgr/rev/6bc12660bce0

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 439460 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm
>--- a/toolkit/mozapps/extensions/AddonManager.jsm
>+++ b/toolkit/mozapps/extensions/AddonManager.jsm
>@@ -272,25 +272,30 @@ var AddonManagerInternal = {
>   backgroundUpdateCheck: function AMI_backgroundUpdateCheck() {
>     if (!Services.prefs.getBoolPref(PREF_EM_UPDATE_ENABLED))
>       return;
> 
>     let scope = {};
>     Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", scope);
>     scope.LightweightThemeManager.updateCurrentTheme();
> 
>-    this.getAddonsByTypes(null, function getAddonsCallback(aAddons) {
>+    this.getAllAddons(function getAddonsCallback(aAddons) {
>       aAddons.forEach(function BUC_forEachCallback(aAddon) {
>-        if (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE) {
>-          aAddon.findUpdates({
>-            onUpdateAvailable: function BUC_onUpdateAvailable(aAddon, aInstall) {
>+        // Search for updates for all add-ons to allow compatibility information
>+        // to apply
Is this what you really mean here? Do you mean that this will check all add-ons for updates and if there is updated compatibility info it will also be applied? I don't suggest that wording but something along those lines if that is what you mean.

>+        aAddon.findUpdates({
>+          onUpdateAvailable: function BUC_onUpdateAvailable(aAddon, aInstall) {
>+            // Start installing updates when the add-on can be updated and
>+            // automatic updates are enabled.
>+            if (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE &&
>+                aAddon.updateAutomatically) {
nit: I think updateAutomatically is better named as autoUpdate. What do you think?

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (From update of attachment 439460 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm
> >--- a/toolkit/mozapps/extensions/AddonManager.jsm
> >+++ b/toolkit/mozapps/extensions/AddonManager.jsm
> >@@ -272,25 +272,30 @@ var AddonManagerInternal = {
> >   backgroundUpdateCheck: function AMI_backgroundUpdateCheck() {
> >     if (!Services.prefs.getBoolPref(PREF_EM_UPDATE_ENABLED))
> >       return;
> > 
> >     let scope = {};
> >     Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", scope);
> >     scope.LightweightThemeManager.updateCurrentTheme();
> > 
> >-    this.getAddonsByTypes(null, function getAddonsCallback(aAddons) {
> >+    this.getAllAddons(function getAddonsCallback(aAddons) {
> >       aAddons.forEach(function BUC_forEachCallback(aAddon) {
> >-        if (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE) {
> >-          aAddon.findUpdates({
> >-            onUpdateAvailable: function BUC_onUpdateAvailable(aAddon, aInstall) {
> >+        // Search for updates for all add-ons to allow compatibility information
> >+        // to apply
> Is this what you really mean here? Do you mean that this will check all add-ons
> for updates and if there is updated compatibility info it will also be applied?
> I don't suggest that wording but something along those lines if that is what
> you mean.

Basically "Check all add-ons for updates so that any compatibility updates will be applied"

> 
> >+        aAddon.findUpdates({
> >+          onUpdateAvailable: function BUC_onUpdateAvailable(aAddon, aInstall) {
> >+            // Start installing updates when the add-on can be updated and
> >+            // automatic updates are enabled.
> >+            if (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE &&
> >+                aAddon.updateAutomatically) {
> nit: I think updateAutomatically is better named as autoUpdate. What do you
> think?

I'm not sure there is much between the two apart from length. I guess neither truly conveys what this setting is doing but for that we'd need something like applyAutomaticallyDetectedUpdates.

Comment 7

[Dave Townsend [:mossop]]

Attachment: follow up fix

There is a follow up fix here for the test which I accidentally made intermittent.

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 439460 [details] [diff] [review] [details])
>...
> > >+        aAddon.findUpdates({
> > >+          onUpdateAvailable: function BUC_onUpdateAvailable(aAddon, aInstall) {
> > >+            // Start installing updates when the add-on can be updated and
> > >+            // automatic updates are enabled.
> > >+            if (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE &&
> > >+                aAddon.updateAutomatically) {
> > nit: I think updateAutomatically is better named as autoUpdate. What do you
> > think?
> 
> I'm not sure there is much between the two apart from length. I guess neither
> truly conveys what this setting is doing but for that we'd need something like
> applyAutomaticallyDetectedUpdates.
As suggested in person how about applyBackgroundUpdates

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 439460 [details] [diff] [review]
patch rev 1

Looks good! r=me with nits fixed

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 439645 [details] [diff] [review]
follow up fix

Looks good and thanks for the walkthrough of the changes

Comment 11

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/1fb9690044ad

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That slipped through. Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2) Gecko/20100720 Firefox/4.0b2

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Litmus test has been added:
https://litmus.mozilla.org/show_test.cgi?id=15178