Closed Bug 704406 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
firefox13 --- fixed
firefox14 --- verified
blocking-fennec1.0 --- +
fennec 11+ ---

People

(Reporter: mfinkle, Assigned: mbrubeck)

References

Details

Attachments

(1 file)

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.
OS: Linux → Android
Hardware: x86 → ARM
Assignee: nobody → mark.finkle
Priority: -- → P3
tracking-fennec: --- → 11+
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
Assignee: mark.finkle → mbrubeck
Blocks: 738429
Attached patch patchSplinter Review
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.
Attachment #608918 - Flags: review?(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.
Attachment #608918 - Flags: review?(margaret.leibovic) → review+
Duplicate of this bug: 738429
Landed with review comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26df3b9b439f
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/26df3b9b439f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Attachment #608918 - Flags: approval-mozilla-aurora?
Verified/fixed on:
Nightly Fennec 14.0a1 (2012-03-29)
Device: Samsung Nexus S
OS: Android 2.3.6
Status: RESOLVED → VERIFIED
Comment on attachment 608918 [details] [diff] [review]
patch

[Triage Comment]
Low risk, mobile-only Fennec blocker. Approved for Aurora 13.
Attachment #608918 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.