Consider adding remove addon in about:debugging

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: about:debugging
P3
enhancement
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: jkt, Assigned: mstriemer)

Tracking

54 Branch
Firefox 55
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
When building an extension, I am often wanting to remove or disable other extensions at the same time.

Having to open up about:addons to remove/disable extensions at the same time seems an additional step that perhaps isn't obvious or convenient when loading extensions is only possible through about:debugging.

Comment 1

3 months ago
Why not add the remove buttons to about:debugging?
(Reporter)

Comment 2

3 months ago
To clarify loading extensions is available under the sub menu on about:addons

Yang I was asking exactly for that, sorry if I wasn't clear.

Comment 3

3 months ago
Sorry, I seem to misread this summary. I agree this idea.
Thanks for filing and sorry about the delay for the triage!

Mark, is adding a remove button to about:debugging in your roadmap? 
I think it would make sense to be able to do that from the about:debugging UI.
Severity: normal → enhancement
Flags: needinfo?(mstriemer)
Priority: -- → P3
See Also: → bug 1359558
See Also: → bug 1349109
(Assignee)

Comment 5

a month ago
I'm looking at this now.
Flags: needinfo?(mstriemer)
(Assignee)

Updated

a month ago
Assignee: nobody → mstriemer
Comment hidden (mozreview-request)

Comment 7

a month ago
mozreview-review
Comment on attachment 8868719 [details]
Bug 1338827 - Add button to remove add-on in about:debugging

https://reviewboard.mozilla.org/r/140310/#review143912

This looks good thanks! 

Just clearing the review so that we can decide between hiding or disabling this new "remove" link. 
Let me know what you think.

::: commit-message-d30ef:1
(Diff revision 1)
> +Bug 1338827 - Remove add-on button in about:debugging r?jdescottes

nit: extremely nitpicky :) but the message makes me think we are removing a button from the about:debugging UI, maybe "add button to remove add-on in about:debugging"

::: devtools/client/locales/en-US/aboutdebugging.properties:99
(Diff revision 1)
> +# LOCALIZATION NOTE (removeDisabledTooltip):
> +# This string is displayed in a tooltip that appears when hovering over a
> +# disabled 'remove' button.
> +removeDisabledTooltip = Only temporarily installed add-ons can be removed

To be removed if you follow my previous comment about hiding vs disabling the link.
Attachment #8868719 - Flags: review?(jdescottes)

Comment 8

a month ago
mozreview-review
Comment on attachment 8868719 [details]
Bug 1338827 - Add button to remove add-on in about:debugging

https://reviewboard.mozilla.org/r/140310/#review143960

::: devtools/client/aboutdebugging/components/addons/target.js:116
(Diff revision 1)
> +          disabled: !canBeReloaded,
> +          title: !canBeReloaded ?
> +            Strings.GetStringFromName("removeDisabledTooltip") : "",
> +        }, Strings.GetStringFromName("remove")),

[Argh of course my comment was lost :'( ]

We used to disable the reload button when all extensions were in the same list, to have consistent actions on the right side of the screen.

Now that we have separated lists, I'm not sure it makes sense to display those links for the regular extensions. My rationale is that the links will never be enabled for items in the "Extensions" list, but seeing them disabled like that makes me think I must do something to enable them.

So for this new "remove" action, I would simply not render it for Extensions. Unless you had other guidelines from UX?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

a month ago
mozreview-review
Comment on attachment 8868719 [details]
Bug 1338827 - Add button to remove add-on in about:debugging

https://reviewboard.mozilla.org/r/140310/#review144546

Great, thank you for adding this!

::: devtools/client/aboutdebugging/test/browser_addons_remove.js:13
(Diff revision 2)
> +
> +function getRemoveButton(document, id) {
> +  return document.querySelector(`[data-addon-id="${id}"] .uninstall-button`);
> +}
> +
> +add_task(function* () {

nit: maybe give a name to this method now that we have two test tasks in this file.
Attachment #8868719 - Flags: review?(jdescottes) → review+

Comment 12

a month ago
mozreview-review
Comment on attachment 8869089 [details]
Bug 1338827 - Only show reload button for temporary add-ons in about:debugging

https://reviewboard.mozilla.org/r/140712/#review144548

Thanks for the cleanup!
Attachment #8869089 - Flags: review?(jdescottes) → review+
Mark, since the original bug report was about having a remove button for any extension, can you explain the reasoning behind allowing the feature only for temporary ones?
Flags: needinfo?(mstriemer)
See Also: → bug 1366215
(Assignee)

Comment 14

a month ago
(In reply to Julian Descottes [:jdescottes] from comment #13)
> Mark, since the original bug report was about having a remove button for any
> extension, can you explain the reasoning behind allowing the feature only
> for temporary ones?

I didn't see a way of detecting if an add-on could be uninstalled. The system add-ons that show up in Extensions couldn't be uninstalled when I tried, but an add-on I installed from AMO could be. Since I didn't see a way to do this I just made remove for temporarily installed extensions only.

Would reload work for extensions installed from AMO/local file, too? It seems to work for AdBlock but there are some errors in the console, possibly unrelated.

It might be nice to move the system add-ons into a third group that is hidden by default and have reload and remove for Temporary Extensions and Extensions in the future.
Flags: needinfo?(mstriemer)
Thanks for the clarifications! 

> It might be nice to move the system add-ons into a third group that is 
> hidden by default and have reload and remove for Temporary Extensions 
> and Extensions in the future.

We actually have a bug where this was being discussed: Bug 1284711 (the conversation ended up with the same conclusion as what you proposed).

We can revisit the actions for "regular" extensions when the system addons have been extracted to a separate list.

> Would reload work for extensions installed from AMO/local file, too? 
> It seems to work for AdBlock but there are some errors in the console, 
> possibly unrelated.

Technically I don't know, but I don't see why you would like to reload them. Is there any scenario where they might have changed and it would be useful to reload them?

Now disabling/removing ... Maybe, if it makes the testing of your current extension easier?
See Also: → bug 1284711
Comment hidden (mozreview-request)
(Assignee)

Comment 17

a month ago
Disable+enable probably makes more sense for regular extensions since the code won't change. There might be some situation where you want to see a website handle the extension going away or handle connecting with the extension.

I added a webextension test while I was in there since it was using a legacy extension and we got bit by only testing the legacy path that on the file path additions.
Keywords: checkin-needed

Comment 18

a month ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/34cfc890b2e5
Add button to remove add-on in about:debugging r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/952d8db302c2
Only show reload button for temporary add-ons in about:debugging r=jdescottes
Keywords: checkin-needed

Comment 19

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34cfc890b2e5
https://hg.mozilla.org/mozilla-central/rev/952d8db302c2
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Mark 54 won't fix as this is late for Beta54.
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.