Support undo uninstall for restartless and already disabled add-ons

VERIFIED FIXED in mozilla2.0b3

Status

()

defect
VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: mossop)

Tracking

Trunk
mozilla2.0b3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta3+)

Details

(Whiteboard: [AddonsRewriteTestday][rewrite])

Attachments

(3 attachments)

Its possible to undo a pending uninstall of normal addons, but restart-less addons don't currently get that luxury.
Depends on: 553499
Flags: in-testsuite?
Flags: in-litmus?
Version: unspecified → Trunk
We have to morph this bug a bit because even for old fashion extensions the removal cannot be undone. The following error appears in the Error Console:

Error: Add-on is not marked to be uninstalled
Source File: file:///Applications/Minefield.app/Contents/MacOS/modules/XPIProvider.jsm
Line: 4475
Summary: Support undo uninstall for restart-less addons → Support undo uninstall for add-ons
Whiteboard: [rewrite] → [AddonsRewriteTestday][rewrite]
Summary: Support undo uninstall for add-ons → Support undo uninstall for restartless and already disabled add-ons
Duplicate of this bug: 562853
Duplicate of this bug: 562892

Updated

9 years ago
Duplicate of this bug: 567604
Duplicate of this bug: 561263
blocking2.0: --- → ?
This is not something we're going to get to in the Firefox 4 timeframe I think.
blocking2.0: ? → -
(In reply to comment #6)
> This is not something we're going to get to in the Firefox 4 timeframe I think.

That said we should remove the link from the ui. Shall I file a separate bug for that?
(In reply to comment #7)
> (In reply to comment #6)
> > This is not something we're going to get to in the Firefox 4 timeframe I think.
> 
> That said we should remove the link from the ui. Shall I file a separate bug
> for that?

Yeah, or make it a part of bug 567120
Assignee

Updated

9 years ago
Duplicate of this bug: 575689
Assignee

Updated

9 years ago
Duplicate of this bug: 578170

Comment 11

9 years ago
Why is Bug 578170 a dupe, that is to do with the undo link not being removed after a successful undo, this bug seems to be to do with failure of undo for certain add-ons.
(In reply to comment #11)
> Why is Bug 578170 a dupe, that is to do with the undo link not being removed
> after a successful undo, this bug seems to be to do with failure of undo for
> certain add-ons.

Interesting, you can actually undo uninstalling personas? I have no idea how that works right now. Guess it isn't a dupe but this will probably impact that heavily
Assignee

Updated

9 years ago
Blocks: 578170
No longer blocks: 578170
Assignee

Updated

9 years ago
Depends on: 573149
Duplicate of this bug: 578923
Assignee

Updated

9 years ago
blocking2.0: - → beta3+
Assignee

Updated

9 years ago
Assignee: nobody → dtownsend
Duplicate of this bug: 580639
Assignee

Updated

9 years ago
Blocks: 581153
Here is the overview of how we're going to do this. The undo process for restartless add-ons is going to be purely UI based rather than handled in the backend. This means that backend providers can be as simple as possible without needing to support undoing uninstalls if they have no need to.

In the UI clicking remove on an add-on in the list will disable the add-on and show the undo option. Clicking undo will of course revert that.

Switching away from the list showing the undo item will make the uninstall permanent, as would closing the add-ons manager. After that there is no way to get add-ons back other than re-installing them.

If remove is clicked in the detail view the UI will switch back to the list view and show the undo option there.
Blocks: 563072
Assignee

Updated

9 years ago
No longer depends on: 553499
Yea, that sounds like a much quicker way of doing this. Would still like a more permanent undo record post-4.0 though (ie, unlimited undo).
As a first step here I want to remove the restriction on disabling themes. Currently the API only allows you to enable themes on the basis that you are always switching between them. That seemed sensible enough but it really just complicates some things and there is no reason not to allow disabling, disabling just switches back to the default theme. The only theme you should not be able to disable is the default.
Attachment #459873 - Flags: review?(robert.bugzilla)
Posted patch patch rev 1Splinter Review
This patch depends on the patch from bug 573149.

The mock provider is updated to support enabling, disabling and uninstalling add-ons, restartless or otherwise. There is a very long test covering every case I could think of here.

The actual fix is fairly straightforward, When removing restartless items just mark appropriately and then when hiding the list view finalise the uninstall for items that are waiting.
Attachment #459877 - Flags: review?
Assignee

Updated

9 years ago
Attachment #459877 - Flags: review? → review?(bmcbride)
Oh, there is one issue with this fix that I wanted to defer to a follow-up bug since I don't think it is blocking and it is kind of irritating to fix.

If you have two copies of the add-ons manager open and click remove in one the other will just show the add-on get disabled. When you close the first manager the add-ons will disappear from the second as it gets uninstalled at that point.
Comment on attachment 459877 [details] [diff] [review]
patch rev 1

Man, that's a lot of test code.

>-  loadView: function(aViewId) {
>+  loadView: function(aViewId, aCallback) {
  
Add aCallback to the global loadView() wrapper function too.

>       <method name="uninstall">
>         <body><![CDATA[
>-          this.mAddon.uninstall();
>-        ]]></body>
>+          // If uninstalling does not require a restart then just disable it
>+          // and show the undo UI.
>+          if (!this.opRequiresRestart("uninstall")) {
>+            this.setAttribute("wasDisabled", this.mAddon.userDisabled);
>+            this.setAttribute("restartrequired", false);
>+            this.setAttribute("status", "uninstalled");
>+            this.mAddon.userDisabled = true;
>+          }
>+          else {
>+            this.mAddon.uninstall();
>+          }
>+          ]]></body>
>       </method>

Nit: remove the newline before the "else".


(In reply to comment #19)
> Oh, there is one issue with this fix that I wanted to defer to a follow-up bug
> since I don't think it is blocking and it is kind of irritating to fix.
> 
> If you have two copies of the add-ons manager open and click remove in one the
> other will just show the add-on get disabled. When you close the first manager
> the add-ons will disappear from the second as it gets uninstalled at that
> point.

This really irks me :\ And really really REALLY makes me want a more API-level solution post-4.0 - whether it be bug 553499, or something else.

Anyway, when you file the followup bug, could you add a reference to it somewhere in the code (like in the uninstall method of the XBL binding).
Attachment #459877 - Flags: review?(bmcbride) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/8bd296358c1e
http://hg.mozilla.org/mozilla-central/rev/58175e77c460

Filed the follow-up bug as bug 582002
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Backed out due to some odd test failures.
Status: RESOLVED → REOPENED
Flags: in-testsuite+ → in-testsuite?
Resolution: FIXED → ---
This is a follow-up fix that turns off remote searching for this test since it causes problems, maybe we should do this for all tests and just turn it on for those that want it.
Attachment #460300 - Flags: review?(bmcbride)
Attachment #460300 - Flags: review?(bmcbride) → review+
(In reply to comment #23)
> maybe we should do this for all tests and just turn it on for
> those that want it.

That could be a good idea. It's likely to cause all sorts of issues and/or make tests more complicated otherwise. Followup bug fodder!
Relanded with the fix: http://hg.mozilla.org/mozilla-central/rev/59a9d183d1ec
Flags: in-testsuite? → in-testsuite+
(In reply to comment #24)
> (In reply to comment #23)
> > maybe we should do this for all tests and just turn it on for
> > those that want it.
> 
> That could be a good idea. It's likely to cause all sorts of issues and/or make
> tests more complicated otherwise. Followup bug fodder!

Filed bug 582058
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 582259
Does not work on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3) Gecko/20100805 Firefox/4.0b3. 

It doesnt show a undo message when I remove a disabled add on.
(In reply to comment #28)
> Does not work on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3)
> Gecko/20100805 Firefox/4.0b3. 
> 
> It doesnt show a undo message when I remove a disabled add on.

Could you please file a new bug with steps to reproduce.
Assignee

Updated

9 years ago
Depends on: 585339
(In reply to comment #29)
> (In reply to comment #28)
> > Does not work on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3)
> > Gecko/20100805 Firefox/4.0b3. 
> > 
> > It doesnt show a undo message when I remove a disabled add on.
> 
> Could you please file a new bug with steps to reproduce.

I filed bug 585339
Dave, do we have a chance to keep those undo messages open until the next restart? Right now you will lose the capability to undo an uninstall after you have switched the panes.
(In reply to comment #31)
> Dave, do we have a chance to keep those undo messages open until the next
> restart? Right now you will lose the capability to undo an uninstall after you
> have switched the panes.

It would require more work than I'm willing to block the release on.
(In reply to comment #32)
> It would require more work than I'm willing to block the release on.

Filed bug 590508 for future work.

Also an undo is not possible for already disabled addons. I have filed bug 590509.

Otherwise it looks good for Jetpacks. Marking as verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED

Updated

5 years ago
Depends on: 1125613
You need to log in before you can comment on or make changes to this bug.