1376941 - Make it easier to load an extension after install fails

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement)

Description

[Mark Striemer [:mstriemer]]

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 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1376941 - Retry install add-on button on error in about:debugging

Review commit: https://reviewboard.mozilla.org/r/154290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154290/

Comment 2

[Mark Striemer [:mstriemer]]

I forgot to update the styles, I'll push an update tomorrow.

Comment 3

[Mark Striemer [:mstriemer]]

Comment on attachment 8883400 [details]
Bug 1376941 - Retry install add-on button on error in about:debugging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154290/diff/1-2/

Comment 4

[Mark Striemer [:mstriemer]]

Attachment: 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

[Julian Descottes [:jdescottes]]

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.

Comment 6

[Mark Striemer [:mstriemer]]

Comment on attachment 8883400 [details]
Bug 1376941 - Retry install add-on button on error in about:debugging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154290/diff/2-3/

Comment 7

[Mark Striemer [:mstriemer]]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28409af996ff
Retry install add-on button on error in about:debugging r=jdescottes

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28409af996ff