Last Comment Bug 704406 - Add support for showing and hiding restart doorhangers in Add-on Manager
: Add support for showing and hiding restart doorhangers in Add-on Manager
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: Firefox 14
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
: 738429 (view as bug list)
Depends on:
Blocks: aom 738429
  Show dependency treegraph
 
Reported: 2011-11-21 21:08 PST by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2016-07-29 14:20 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
+
11+


Attachments
patch (10.14 KB, patch)
2012-03-23 16:59 PDT, Matt Brubeck (:mbrubeck)
margaret.leibovic: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-11-21 21:08:50 PST
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) 2012-03-23 16:59:35 PDT
Created attachment 608918 [details] [diff] [review]
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 2012-03-26 10:45:26 PDT
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 3 Matt Brubeck (:mbrubeck) 2012-03-26 12:31:07 PDT
*** Bug 738429 has been marked as a duplicate of this bug. ***
Comment 4 Matt Brubeck (:mbrubeck) 2012-03-26 12:32:50 PDT
Landed with review comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26df3b9b439f
Comment 5 Marco Bonardo [::mak] 2012-03-27 05:28:13 PDT
https://hg.mozilla.org/mozilla-central/rev/26df3b9b439f
Comment 6 Matt Brubeck (:mbrubeck) 2012-03-28 15:21:40 PDT
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 2012-03-30 02:18:25 PDT
Verified/fixed on:
Nightly Fennec 14.0a1 (2012-03-29)
Device: Samsung Nexus S
OS: Android 2.3.6
Comment 8 Alex Keybl [:akeybl] 2012-04-02 09:53:27 PDT
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) 2012-04-02 11:44:39 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/63c5274f27cb

Note You need to log in before you can comment on or make changes to this bug.