590347 - Expose the softblock notification in preference to the incompatible notification when compatibility checking is disabled

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We want to only show one of these messages at a time. When compatibility checking is enabled the incompatible message is more important since it blocks the user from using the add-on. When compatibility checking is disabled the softblock message is probably more important since we have information that the add-on may be dangerous to use.

Comment 1

[Dave Townsend [:mossop]]

This is true for the details and list views

Comment 2

[Dave Townsend [:mossop]]

Doesn't affect mainstream users so we shouldn't block the release on it.

Comment 3

[Sachin Hosmani [:sachin]]

What are the softblock and incompatible notifications?
How can I reproduce them?

I was able to find where the compatibility check preference listener lies, but I don't understand what these notifications are.

Thanks

Comment 4

[Dave Townsend [:mossop]]

Attachment: Incompatible notification screenshot

(In reply to Sachin Hosmani from comment #3)
> What are the softblock and incompatible notifications?
> How can I reproduce them?
> 
> I was able to find where the compatibility check preference listener lies,
> but I don't understand what these notifications are.

This is an example of the incompatible notification, it appears in the list or detail view of add-ons. The softblock notification looks similar though with different text/colours. You can simulate softblocking an add-on with a custom blocklist file (see https://wiki.mozilla.org/Extension_Blocklisting:Testing). An incompatible add-on can be created with just an install.rdf with an incompatible min/maxVersion range in it.

Comment 5

[Sachin Hosmani [:sachin]]

Attachment: Exposes the softblock notification in preference to the incompatible notification when compatibility checking is disabled

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 742704 [details] [diff] [review]
Exposes the softblock notification in preference to the incompatible notification when compatibility checking is disabled

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

What files are you patching here, the line numbers don't match up to mozilla-central.

This looks good but I'd really like to see a test for it. browser_list.js and browser_details.js should show examples of testing these things. You may find it easier to write a separate test file to just test this case rather than squeeze it into those though as you have to manipulate compatibility checking.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2783,5 @@
>          errorLink.value = gStrings.ext.GetStringFromName("details.notification.blocked.link");
>          errorLink.href = this._addon.blocklistURL;
>          errorLink.hidden = false;
> +      } else if (!this._addon.isCompatible && (AddonManager.checkCompatibility ||
> +        !(this._addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED))) {

Just change the == to != and drop the leading !

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1186,5 @@
>                this._errorLink.value = gStrings.ext.GetStringFromName("notification.blocked.link");
>                this._errorLink.href = this.mAddon.blocklistURL;
>                this._errorLink.hidden = false;
> +            } else if ((!isUpgrade && !this.mAddon.isCompatible) && (AddonManager.checkCompatibility ||
> +               !(this.mAddon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED))) {

Ditto

Comment 7

[Sachin Hosmani [:sachin]]

> What files are you patching here, the line numbers don't match up to
> mozilla-central.

I should have pulled before creating the patch.

And yes, I'm working on the rest.

Comment 8

[Sachin Hosmani [:sachin]]

Attachment: Includes a test

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 747529 [details] [diff] [review]
Includes a test

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

Looks great, thanks!

Comment 10

[Sachin Hosmani [:sachin]]

Dave, are there any issues left? Can it be pushed?

Comment 11

[Dave Townsend [:mossop]]

Ah yes sorry I didn't realise you couldn't checkin. Flagging it for someone to grab or I'll take care of it later on today.

Comment 12

[Dave Townsend [:mossop]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e52cb6e6bac

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e52cb6e6bac