Closed Bug 1376941 Opened 4 years ago Closed 3 years ago

Make it easier to load an extension after install fails

Categories

(DevTools :: about:debugging, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: mstriemer, Assigned: mstriemer)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
I forgot to update the styles, I'll push an update tomorrow.
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 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 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.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/28409af996ff
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.