Test add-ons are not recognized as installed by their edit pages, and do not uninstall when all Builder tabs are closed

RESOLVED WONTFIX

Status

addons.mozilla.org Graveyard
Add-on Builder
P1
critical
RESOLVED WONTFIX
6 years ago
3 years ago

People

(Reporter: dbuc, Assigned: zalun)

Tracking

unspecified
Q1 2012
Dependency tree / graph

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
If you install an add-on then navigate away from the edit page of the installed add-on, the test button logic loses awareness of the add-on's installed state.

In the previous version of the ABH, all test add-ons were uninstalled when all Builder tabs were closed. This has regressed and is no longer functioning.
(Reporter)

Updated

6 years ago
Assignee: nobody → poirot.alex
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → Q1 2012
This issue is due to the new ABH feature. It now supports multiple addons.
So that you can run one addon per tab.
Unfortunatly, it ends up breaking current usage of `isInstalled` command.
We will need to improve code on both sides.
`isInstalled` should be send with an addon id, and ABH should read this addon id and return the expected answer.
I think we will have a similar issue with `uninstall` command. We should sent an precise addon id there too.

`isInstalled`:
https://github.com/mozilla/FlightDeck/blob/master/media/base/js/flightdeck/FlightDeck.js#L127
`uninstall`:
https://github.com/mozilla/FlightDeck/blob/master/media/base/js/flightdeck/FlightDeck.js#L301

Would it be ok on your side?
I can handle ABH fix if it is ok for FlightDeck.
(Assignee)

Comment 2

6 years ago
I think it's a good idea

There are some issues thought:

1. Will ABH uninstall onbeforeunload?
   We can register the loaded add-on ID in some special variable so the ABH would know which to uninstall

2. It would be good if ABH would run the callback with the Add-on ID as a response value.
(Reporter)

Comment 3

6 years ago
Within the scope of this fix, it would be awesome to modify the install experience to that the object in the page that does the postMessage passing to the add-on is injected into the already open Builder tabs. Currently you have to reload the page because clicking on the test button again causes the "Download the Add-on Builder Helper" message to be thrown again and again.
Specifically, the old ABH would emit an event "addonbuilderhelperstart": https://github.com/mozilla/FlightDeck/blob/master/media/base/js/flightdeck/FlightDeck.js#L102

It would also attach `mozFlightDeck` to the open window.
Depends on: 708190
Created attachment 608350 [details]
Pull request 4

Here is the pull request for this bug.
I started addressing issues mentioned during PR#3 that haven't being addressed as the PR was merged before the review.

Comment 3 depends on bug 708190. I'll try to fix this in SDK so that we can easily get this behavior fixed in ABH. (Note that I let the code that fires "addonbuilderhelperstart", it should still be sent! But mozFlightDeck isn't injected anymore.)

I still need a confirmation from you about how to change the mozFlightDeck API in order to fix these issues.
The first main question to answer is when should we uninstall addons: on firefox quit? on tab close? on `onunload` event? something else?
(currently they are only uninstalled if we re-click on test button.)

Piotr tend to suggest to automatically uninstall addons on unload event. It basically means that when a new AB tab is open, no addon can be already installed and only one can be installed at a time. So that we can keep the existing mozFlightDeck API as-is. But Daniel, you tend to suggest something different, where the addon can be kept installed when you browser the documentation.

Then, when we will have decided when addons are uninstalled, we will be able to define if we should modify the API and how.
I'm not sure what was best, but the previous ABH seemed to uninstall tested addons at Firefox quit.
(Assignee)

Comment 7

6 years ago
What I was suggesting is to uninstall the add-on onbeforeunload, which means on close of the tab. One certainly can browse other add-ons without uninstalling it.
(Assignee)

Comment 8

6 years ago
@seanmonstar in old ABH only one add-on at a time could be installed.
(Reporter)

Comment 9

6 years ago
I'd like to allow multiple add-ons to be installed at a time, and the uninstallation of an add-on happen either when the user clicks the test button to deactivate the add-on directly, or the tab that installed the add-on is closed (unload). If there is any other issue that requires feedback, just let me know.
(In reply to Daniel Buchner [:dbuc] from comment #9)
> I'd like to allow multiple add-ons to be installed at a time, and the
> uninstallation of an add-on happen either when the user clicks the test
> button to deactivate the add-on directly, or the tab that installed the
> add-on is closed (unload). If there is any other issue that requires
> feedback, just let me know.

What about when you navigate away from the addon's edit page? Should ABH add an onUnload to the document? Stop the user from navigating away while the editor's open?
(Reporter)

Comment 11

6 years ago
Yes, I would view any unload event occurrence on a page that has installed an add-on as a trigger to uninstall it, we could certainly prompt the user to ask whether they really want to leave the page and uninstall the test add-on associated. 

I'm open to other ideas, but this seems like it would satisfy the majority of use-cases.
Blocks: 750138
Comment on attachment 608350 [details]
Pull request 4

I've addressed all review comments reported lately, after the addon being published on AMO.
Attachment #608350 - Flags: review?(rFobic)
Blocks: 760853
Attachment #608350 - Flags: review?(rFobic) → review+
The pull request looks fine Alex, tho I don't think it closes the bug.
Landed:
https://github.com/mozilla/addon-builder-helper/commit/0c040e6f081472be35a5bbe7885fdc8d2fab4636

Yes, some more work will be needed to address this bug.
This revision allows to build ABH with SDK master.
(Assignee)

Comment 15

5 years ago
It is still the case - I'll check what's wrong on Builder side
Assignee: poirot.alex → zaloon
It can be fixed on both side. Either listen to page unload in the page itself or from the addon. Currently the addon doesn't keep a registry to addon currently installed, so it may be easier to do that from the website.
the add-on builder project has been retired.  More info at https://blog.mozilla.org/addons/2013/12/18/add-on-builder/
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.