489941 - Rework the Update Add-ons dialog

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

When Fennec (or any other extension enabled Mozilla app) starts after an update, it checks to see if the installed add-ons are still compatible. It displays a dialog box if any are not compatible, allowing the user to check the updates to those add-ons.

This dialog causes problems on Windows Mobile because of size issues. It also creates a problem where the dialog does not take top-level focus, instead hiding behind other running applications. The user then has no way of knowing that Fennec has launched and is awaiting input about an add-on update check.

We should either fix the size and focus issues or remove the dialog altogether. Handling this situation inside Fennec might be easier (thinking UI here) and we could take actions based on prefs (just check or don't check, but don't bug me with a freaking dialog)

Comment 1

[Madhava Enros [:madhava]]

Couldn't we use one of our lower-right-hand-corner alert boxes for this?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Yes, but this dialog appears before Fennec is even active/running, so we'd need to do some work to just make it skip past that point and let Fennec handle it.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

IMO, the ideal solution here would be a patch that allows a default choice. Right now, we always show the update.xul window here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#3382

Called from here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#3329

I think we could use a preference that tells the EM to just skip the update check (disabling add-ons that are not compatible) and then the application can tell the user about the situation when it loads.

Comment 4

[Dave Townsend [:mossop]]

I think for 1.9.3 we're going to be getting rid of this dialog everywhere. If you need it gone on 1.9.2 then I suspect the easiest option right now might be just to ifdef it off for mobile.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (platform)

Adds two preferences:
* Adds a preference to turn off/on the UI for incompatible add-ons found during an application update
* If the UI is off, the second preference will store the list of disabled add-on IDs

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (fennec)

Depends on the platform patch

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (fennec) - v2

Changed the strings slightly

Comment 8

[Madhava Enros [:madhava]]

Instead of "One or more add-ons are no longer compatible and were disabled" how
about the shorter:

[# of add-ons] incompatible add-ons disabled

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 400132 [details] [diff] [review]
patch (fennec) - v2

>+alertAddonsDisabled=#1 incompatible add-on was disabled;#1 incompatible add-ons were disabled

... should have a localization note pointing to https://developer.mozilla.org/en/Localization_and_Plurals. We don't really have a better way to mark up plurals at this point in time.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)

> ... should have a localization note pointing to
> https://developer.mozilla.org/en/Localization_and_Plurals. We don't really have
> a better way to mark up plurals at this point in time.

done

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed the fennec part:
https://hg.mozilla.org/mobile-browser/rev/2808948efa5c

leaving open for the platform part

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 399570 [details] [diff] [review]
patch (platform)

Couple of tweaks here, would like to see it run by me again  but I expect it to be fine at that point.

>+const PREF_EM_DISABLED_ADDONS_LIST    = "extensions.disabledAddons";
>+const PREF_EM_DISABLED_ADDONS_UI      = "extensions.disabledAddons.showUI";

I don't really like this preference name, nor the constant name. How about extensions.showMismatchUI and PREF_EM_SHOW_MISMATCH_UI.

>-    // Always check for compatibility updates when upgrading if we have add-ons
>-    // that aren't managed by the application.
>-    if (!allAppManaged)
>-      this._showMismatchWindow(inactiveItemIDs);
>+    // Determine if we should check for compatibility updates when upgrading if
>+    // we have add-ons that aren't managed by the application.
>+    if (!allAppManaged) {

Let's give Firefox a win here too. We shouldn't show the UI if all add-ons are appManaged, or if none were disabled this time or if it is the first run.

>+      // Should we show a UI or just pass the list via a pref?
>+      if (getPref("getBoolPref", PREF_EM_DISABLED_ADDONS_UI, true)) {
>+        this._showMismatchWindow(inactiveItemIDs);
>+      }
>+      else {
>+        // Remember the list of add-ons that were disbaled this time around

"disabled"

>+        // unless this was a new profile.
>+        if (!gFirstRun && disabledAddons.length > 0)
>+          gPref.setCharPref(PREF_EM_DISABLED_ADDONS_LIST, disabledAddons.join(","));
>+        else
>+          gPref.clearUserPref(PREF_EM_DISABLED_ADDONS_LIST);
>+      }
>+    }

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (platform) - v2

This should have all the requested changes.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 400524 [details] [diff] [review]
patch (platform) - v2

Looks good, thanks

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (fennec) tweaks a prefname

Update the pref name used in the fennec mobile.js prefs file. The pref was changed in the platform patch

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed platform:
http://hg.mozilla.org/mozilla-central/rev/3710814acfa9

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed last fennec tweak:
https://hg.mozilla.org/mobile-browser/rev/965ffe4133e4

Comment 18

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

I highly suspect this caused bug 542391