Make it easier to load an extension after install fails

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Developer Tools: about:debugging
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 months ago
When installing an extensions in about:debugging if the install initially fails then you must go back to the file picker to try installing again.

It would be much nicer to show some info about the failure and include a Retry button to try installing the extension again.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 months ago
I forgot to update the styles, I'll push an update tomorrow.
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a month ago
Created attachment 8884004 [details]
retry-failed-install.png

My style update patch grew quite a bit so I think I'll do that separately. I just moved the button to the right for now.

Comment 5

a month ago
mozreview-review
Comment on attachment 8883400 [details]
Bug 1376941 - Retry install add-on button on error in about:debugging

https://reviewboard.mozilla.org/r/154290/#review160222

Looks good and works fine, thanks for the patch. 
A few comments about the test, feel free to land after addressing them.

::: devtools/client/aboutdebugging/test/browser_addons_install.js:20
(Diff revision 2)
> +  let MockFilePicker = SpecialPowers.MockFilePicker;
> +  MockFilePicker.init(window);
> +  MockFilePicker.setFiles([file]);
> +}
> +
> +function promiseWriteWebManifestForExtension(

Maybe add a small JSDoc here? Third argument is never provided, should it be kept?

::: devtools/client/aboutdebugging/test/browser_addons_install.js:128
(Diff revision 2)
> +
> +  // Retry the install.
> +  retryButton.click();
> +
> +  // Wait for the add-on to be shown.
> +  yield waitForMutation(getTemporaryAddonList(document), { childList: true });

Maybe we should start the observer before doing the click here, to avoid races?

::: devtools/client/aboutdebugging/test/browser_addons_install.js:131
(Diff revision 2)
> +
> +  // Wait for the add-on to be shown.
> +  yield waitForMutation(getTemporaryAddonList(document), { childList: true });
> +
> +  // Verify the add-on is installed.
> +  ok(getAddonEl(), "Addon is installed");

The addon should be uninstalled before leaving aboutdebugging, otherwise running the test twice fails.

> yield uninstallAddon({document, id: addonId, name: "invalid-addon-install-retry"});

Could be nice to uninstall all temporary installed addon in a cleanup method.
Attachment #8883400 - Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a month ago
mozreview-review-reply
Comment on attachment 8883400 [details]
Bug 1376941 - Retry install add-on button on error in about:debugging

https://reviewboard.mozilla.org/r/154290/#review160222

> Maybe add a small JSDoc here? Third argument is never provided, should it be kept?

I lifted this from an xpcshell test. I'll remove the third argument.

> The addon should be uninstalled before leaving aboutdebugging, otherwise running the test twice fails.
> 
> > yield uninstallAddon({document, id: addonId, name: "invalid-addon-install-retry"});
> 
> Could be nice to uninstall all temporary installed addon in a cleanup method.

I added an uninstall to the test. That seems like something that could go in `closeAboutDebugging()` most likely. That seems like an unrelated change though. I'll file a bug for it.
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 8

a month ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/28409af996ff
Retry install add-on button on error in about:debugging r=jdescottes
Keywords: checkin-needed

Comment 9

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28409af996ff
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.