704406 - Add support for showing and hiding restart doorhangers in Add-on Manager

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

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

When enabling, disabling and uninstalling non-restartless add-ons in the Add-on Manager, we need to manage the "restart" doorhanger. We need to show and hide it dynamically as the user manipulates the add-on.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

In addition to implementing showRestart and hideRestart, this patch also:

- does some minor refactoring in aboutAddons.js to avoid repeated "detailItem.addon", and

- fixes a simple bug that broke NativeWindow.doorhanger.hide.

Comment 2

[:Margaret Leibovic]

Comment on attachment 608918 [details] [diff] [review]
patch

>diff --git a/mobile/android/chrome/content/aboutAddons.js b/mobile/android/chrome/content/aboutAddons.js

>   setEnabled: function setEnabled(aValue, aAddon) {
>     let detailItem = document.querySelector("#addons-details > .addon-item");
>     let addon = aAddon || detailItem.addon;
>     if (!addon)
>       return;
> 
>+    let listItem = this._getElementForAddon(addon.id);

Can listItem be null? ...

>       if ((addon.pendingOperations & AddonManager.PENDING_ENABLE) ||
>           (addon.pendingOperations & AddonManager.PENDING_DISABLE)) {
>         this.showRestart();
>-      } else if (addon == detailItem.addon &&
>-            detailItem.getAttribute("opType") == "needs-disable" ||
>-            detailItem.getAttribute("opType") == "needs-enable") {
>+      } else if (/needs-(enable|disable)/.test(listItem.getAttribute("opType"))) {

... if so, this could cause an error...

>     // Sync to the list item
>-    let listItem = this._getElementForAddon(addon.id);
>     if (listItem) {

... if not, we could get rid of this check :)

>     } else {
>-      detailItem.addon.uninstall();
>-      let opType = this._getOpTypeForOperations(detailItem.addon.pendingOperations);
>+      addon.uninstall();
>+      let opType = this._getOpTypeForOperations(addon.pendingOperations);

Nit: While you're cleaning this up, you could stick this declaration inside the if statement below it, since it's not used elsewhere.

Comment 4

[Matt Brubeck (:mbrubeck)]

Landed with review comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26df3b9b439f

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/26df3b9b439f

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 608918 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Add-on manager does not prompt to restart when a restart is necessary.

Testing completed (on m-c, etc.): Landed on m-c March 27.

Risk to taking this patch (and alternatives if risky): Patch is mobile-only and low-risk.

String changes made by this patch: None.

Comment 7

[Paul Feher]

Verified/fixed on:
Nightly Fennec 14.0a1 (2012-03-29)
Device: Samsung Nexus S
OS: Android 2.3.6

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 608918 [details] [diff] [review]
patch

[Triage Comment]
Low risk, mobile-only Fennec blocker. Approved for Aurora 13.

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63c5274f27cb