702920 - If compatibility changes for a user-disabled addon, the UI doesn't get notified and doesn't update

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

If compatibility of a user-disabled addon changes (eg, compatible->incompatible) while the UI is open, the UI never gets notified of the change and thus doesn't update to show the new compatibility status.

This can occur when a compatibility-related pref changes - eg, extensions.strictCompatibility.

Note that it doesn't occur when there's a compatibility update, as the UI gets notified via onCompatibilityUpdateAvailable.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I wonder if we could just abuse onCompatibilityUpdateAvailable and send that when the compatibility changes. Not sure what other side-effects that would have though.

Comment 2

[Dave Townsend [:mossop]]

How about sending "onPropertyChanged" for isCompatible or appDisabled?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, that would work.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

And as usual, this is mostly tests.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 582714 [details] [diff] [review]
Patch v1

Review of attachment 582714 [details] [diff] [review]:
-----------------------------------------------------------------

The tests here verify that the UI updates when the onPropertyChanged event is sent, it doesn't verify that that event is actually sent for real add-ons. I think this needs an additional test for that. Install a real add-on and toggle strictCompatibility or something. xpcshell might be easiest for that.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3761,5 @@
> +    if (appDisabledChanged) {
> +      AddonManagerPrivate.callAddonListeners("onPropertyChanged",
> +                                            aAddon,
> +                                            ["appDisabled"]);
> +    }

Might make sense to include notifications for the other properties too. Up to you.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #7)
> The tests here verify that the UI updates when the onPropertyChanged event
> is sent, it doesn't verify that that event is actually sent for real
> add-ons. I think this needs an additional test for that. Install a real
> add-on and toggle strictCompatibility or something. xpcshell might be
> easiest for that.

Already done, see test_onPropertyChanged_appDisabled.js in that patch.


> Might make sense to include notifications for the other properties too. Up
> to you.

Looked into doing this:
- There's no way to tell if isCompatible changed, since it does so before updateAddonDisabledState is called, and depends on so many things
- The only time softDisabled can change is in cases where onEnabling/onDisabling are also fired
- Firing onPropertyChanged for userDisabled requires far more work than it's worth

So I'll skip that until there's an actual need for any of those.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/d0b03b763e5b

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Backed out because of xpcshell oranges:

https://hg.mozilla.org/integration/fx-team/rev/3698bfa55a72

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, that was stupid... this is dependent on bug 715787, which hasn't landed yet.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe532188a808

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/fe532188a808