Jonathan Kingston [:jkt] he/him Reporter
	Description • 8 years 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.

	YF (Yang)
	Comment 1 • 7 years ago

Why not add the remove buttons to about:debugging?

	Jonathan Kingston [:jkt] he/him Reporter
	Comment 2 • 7 years 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.

	YF (Yang)
	Comment 3 • 7 years ago

Sorry, I seem to misread this summary. I agree this idea.

	Julian Descottes [:jdescottes]
	Comment 4 • 7 years ago

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.

	Mark Striemer [:mstriemer] Assignee
	Comment 5 • 7 years ago

I'm looking at this now.

	Julian Descottes [:jdescottes]
	Comment 7 • 7 years 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.

	Julian Descottes [:jdescottes]
	Comment 8 • 7 years 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?

	Julian Descottes [:jdescottes]
	Comment 11 • 7 years 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.

	Julian Descottes [:jdescottes]
	Comment 12 • 7 years 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!

	Julian Descottes [:jdescottes]
	Comment 13 • 7 years ago

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?

	Mark Striemer [:mstriemer] Assignee
	Comment 14 • 7 years 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.

	Julian Descottes [:jdescottes]
	Comment 15 • 7 years ago

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?

	Mark Striemer [:mstriemer] Assignee
	Comment 17 • 7 years 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.

	Pulsebot
	Comment 18 • 7 years 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

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/34cfc890b2e5
https://hg.mozilla.org/mozilla-central/rev/952d8db302c2

	Gerry Chang [:gchang]
	Comment 20 • 7 years ago

Mark 54 won't fix as this is late for Beta54.