496287 - App update incompatible list should not warn about disabled/blocklisted items

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

When you install 3.5 over the top of 3.0 you will get warned about add-ons that are not compatible with 3.5. This happens even if the add-ons are already user disabled or blocklisted. We shouldn't warn about those cases.

Comment 1

[Dave Townsend [:mossop]]

So the point of this dialog is to tell the user that some items in the list are not compatible with the new version of Firefox they are using. The wording is:

"The following add-ons are not compatible with this version of Firefox and have been disabled".

Based on that wording I think we shouldn't list items here where the item was disabled in the previous version of Firefox (whether at the user's choice, or because of the blocklist, or because of compatibility or any other reason).

We should be able to infer this during startup by checking the value of appDisabled in the datasource before we update it.

Comment 2

[Dave Townsend [:mossop]]

Edge case. What should happen if while running Firefox 3.0.x the user clicked disable on an item, or the blocklist updated and set an item to be disabled, but no restart happened. Instead the user shutdown Firefox 3.0.x and then starts Firefox 3.5.

Should the list warn about the item that was waiting to be disabled?

Same question for the case where the item was disabled and it was waiting to be enabled.

Comment 3

[Dave Townsend [:mossop]]

Attachment: screenshot

for reference

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

I think it make most sense that if an item was previously disable, or waiting to be disabled then it should be hidden. If it was waiting to be enabled then the user should be warned about it.

This patch does a quick scan through the installed items to identify those that are disabled or pending disable and remembers their IDs. It passes that list to the incompatibility dialog which ignores them after the initial compatibility sync.

This is I think quite a low impact change but I don't think we can automate tests for it. I have prepared some manual testcases to verify the behaviour: https://people.mozilla.com/~dtownsend/testcases/bug496287/

Comment 5

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

Why can't you just change the call to getIncompatibleItemList so that false is passed for the includeDisabled param to get what you want instead of all of these changes?

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> Why can't you just change the call to getIncompatibleItemList so that false is
> passed for the includeDisabled param to get what you want instead of all of
> these changes?

Because (and this is awesome) that would then not include items that were disabled because they are incompatible with the application, so basically it would return nothing. Changing that would be an API change

Comment 7

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

Sounds like a great user experience to be told nothing is incompatible ;)

I finally remembered why it was done this way. If the app is dies or is killed for what ever reason during the incompatible check and the incompatible add-ons weren't disabled prior to this phase they could easily screw up the app. Hence why all the work prior to showing the wizard.

Comment 8

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

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

Just a couple of nits... I haven't tested the patch.

>diff --git a/toolkit/mozapps/extensions/content/update.js b/toolkit/mozapps/extensions/content/update.js
>--- a/toolkit/mozapps/extensions/content/update.js
>+++ b/toolkit/mozapps/extensions/content/update.js
>@@ -45,29 +45,33 @@ const nsIAUCL = Components.interfaces.ns
> const PREF_UPDATE_EXTENSIONS_ENABLED            = "extensions.update.enabled";
> const PREF_XPINSTALL_ENABLED                    = "xpinstall.enabled";
> 
> var gUpdateWizard = {
>   // When synchronizing app compatibility info this contains all installed
>   // add-ons. When checking for compatible versions this contains only
>   // incompatible add-ons.
>   items: [],
>+  // Contains a list of IDs of items that weren't enabled in the previous
>+  // version of the application.
nit: this could be a bit more concise. Perhaps something like "Contains a list of items that were disabled prior to the application upgrade."

>+  inactiveItems: [],
nit: I'm tempted to say this should be named disabledItemIDs but it is really items that were disabled or were going to be disabled prior to the app upgrade. How about inactiveItemIDs?

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>@@ -3613,36 +3613,48 @@ ExtensionManager.prototype = {
>     [isDirty,] = this._ensureDatasetIntegrity();
> 
>     if (this._checkForFileChanges())
>       isDirty = true;
> 
>     if (PendingOperations.size != 0)
>       isDirty = true;
> 
>+    var ds = this.datasource;
>+    var inactiveItems = [];
nit: inactiveItemIDs

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Addresses the nits.

Looking for approval to land this on trunk and branch if possible. It is a low risk patch that can approve the upgrade experience for users by not unnecessarily warning them about add-ons that are incompatible with the new version of Firefox. There isn't a way to automatically test this but manual test cases are provided.

Comment 10

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

Dave, this may be another edgecase but does this handle the case where an add-on was disabled and has OP_NEEDS_ENABLE? If it doesn't perhaps the disabled checks you added in nsExtensionManager.js.in needs to include a check that it doesn't also have OP_NEEDS_ENABLE.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> Dave, this may be another edgecase but does this handle the case where an
> add-on was disabled and has OP_NEEDS_ENABLE? If it doesn't perhaps the disabled
> checks you added in nsExtensionManager.js.in needs to include a check that it
> doesn't also have OP_NEEDS_ENABLE.

If it has OP_NEEDS_ENABLE then the add-on won't end up in inactiveItemIDs. I did this on purpose with the rationale that if the user was expecting an item to be enabled the next time they start the app then warning them that it is now incompatible is the appropriate thing to do.

Comment 12

[Dave Townsend [:mossop]]

Pushed to trunk with a=beltzner over IRC

http://hg.mozilla.org/mozilla-central/rev/cacf7953fcac

Comment 13

[Dave Townsend [:mossop]]

Cannot automate the tests, but litmus tests could be generated from the testcases at https://people.mozilla.com/~dtownsend/testcases/bug496287/

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 381856 [details] [diff] [review]
patch rev 2

Too late in the cycle now, maybe for 1.9.1.1

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 381856 [details] [diff] [review]
patch rev 2

This is a very safe patch that removes an annoyance from the upgrade process probably making things easier for most minor updates. There aren't any automated tests but a full set of steps for manually testing are available.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 381856 [details] [diff] [review]
patch rev 2

a1912=beltzner, Mossop can you please land before you head off on vacation.

Comment 17

[Dave Townsend [:mossop]]

Fixed on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/346f22da1410

Comment 18

[juan becerra [:juanb]]

Verified using:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)

Installed Fx 3.0.12 with Tab Sidebar version 2.0, which I disabled. Then I installed Fx 3.5.2 on top. When I started the browser the dialog illustrated in comment #3 did not appear. If I install 3.5 the dialog appears.

Comment 19

[Tony Chung [:tchung]]

Ran the same tests as comment 18, but with the trunk builds.  Saw the same results as juan reported.  Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090805 Minefield/3.6a1pre

Comment 20

[Aakash Desai [:aakashd]]

Testcases added to litmus for regression testing:

https://litmus.mozilla.org/show_test.cgi?id=9507
https://litmus.mozilla.org/show_test.cgi?id=9508