Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement management.get and getAll

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Untriaged
P5
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: mattw, Assigned: bsilverberg)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {dev-doc-needed})

unspecified
mozilla56
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [management] triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Chrome documentation: https://developer.chrome.com/extensions/management#method-getAll
(Reporter)

Updated

a year ago
Blocks: 1282855
(Reporter)

Updated

a year ago
Duplicate of this bug: 1283279
The last time that we talked about this, there was some question about whether we actually wanted to implement it. As I recall, we decided we wouldn't implement it until we had some idea of what extensions are actually using it for.
Summary: Implement chrome.management.getAll for WebExtensions → Implement management.getAll
(Reporter)

Updated

a year ago
Blocks: 1282856
(Assignee)

Updated

a year ago
No longer blocks: 1282855
(Reporter)

Updated

a year ago
Blocks: 1282895
(Reporter)

Updated

a year ago
Blocks: 1283654
(Reporter)

Updated

a year ago
Blocks: 1283660
(Assignee)

Updated

a year ago
Blocks: 1281815
(Assignee)

Updated

a year ago
No longer blocks: 1282593
(Assignee)

Updated

a year ago
No longer blocks: 1282856
(Assignee)

Comment 3

a year ago
Some examples of usage from "popular chrome extensions":

- Video Downloader Pro is using it to look for a particular extension (Google Cast) to see if it is installed and/or enabled. If both are true then it uses it for playback. If it is not installed the user is asked to install it, and if it is not enabled then the user is asked to enable it.

- Adblock uses it in a number of ways:
    - It gathers a list of all installed extensions which is then provided to a bug report which can be submitted.
    - It gathers a list of all installed extensions for display on a screen to help diagnose problems.
    - It has a feature that allows a user to disable all other extensions, for which it uses getAll to get the list of those
      extensions. I assume that this is also for troubleshooting purposes.

- Flash Video Downloader uses it in a similar manner to Video Downloader Pro in that it looks for specific extensions to see if they are installed, and if they are it enables features of itself that are only available if said extensions are installed.

Do these seem like valid enough use cases to implement the method? We have flagged it for implementation as part of the project to enable more Chrome extensions to run in Firefox.
Flags: needinfo?(kmaglione+bmo)

Updated

a year ago
Duplicate of this bug: 1283687

Comment 5

a year ago
should also implement chrome.management.get at same time - putting on one bug (closed bug 1283687 as dupe to this one)

Chrome documentation: https://developer.chrome.com/extensions/management#method-get
(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> - Video Downloader Pro is using it to look for a particular extension
> (Google Cast) to see if it is installed and/or enabled. If both are true
> then it uses it for playback. If it is not installed the user is asked to
> install it, and if it is not enabled then the user is asked to enable it.

Does Google Cast even work in Firefox? Is it even possible for it to?

> - Adblock uses it in a number of ways:
>     - It gathers a list of all installed extensions which is then provided
> to a bug report which can be submitted.
>     - It gathers a list of all installed extensions for display on a screen
> to help diagnose problems.

We have about:support for this.

>     - It has a feature that allows a user to disable all other extensions,
> for which it uses getAll to get the list of those
>       extensions. I assume that this is also for troubleshooting purposes.

This would require us to also implement that functionality for it to be
useful.

Either way, the lack of this feature doesn't seem like it should prevent
significant parts of the add-on from working.

> - Flash Video Downloader uses it in a similar manner to Video Downloader Pro
> in that it looks for specific extensions to see if they are installed, and
> if they are it enables features of itself that are only available if said
> extensions are installed.

Are any of these extensions relevant to Firefox?

Maybe we should consider initially implementing this as a stub that just
returns the entry for the add-on itself, and decide later how we want to
handle the full implementation.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Summary: Implement management.getAll → Implement management.get and getAll
Version: 34 Branch → unspecified
(Assignee)

Updated

a year ago
Depends on: 1287892

Updated

a year ago
Whiteboard: [management] → [management] triaged
(Assignee)

Comment 7

a year ago
We are going to hold off on implementing this API method until some compelling use cases are presented by the developer community.
Assignee: bob.silverberg → nobody
No longer blocks: 1282979
Status: ASSIGNED → NEW
Priority: P2 → P5

Comment 8

a year ago
Here is another use case (it's not in the current Chromium extension yet, but I'll implement it in the future if Firefox also supports it):

Retrieving a list of installed addons so that their source code can be viewed with my Extension Source Viewer (https://github.com/Rob--W/crxviewer). For this I would need management.getAll + the following of ExtensionInfo (https://developer.chrome.com/extensions/management#type-ExtensionInfo):
- id (to find the listing on AMO)
- name
- version (to select a specific listing on AMO)
- icons (optional, to make the list of extensions prettier)
- installType (to see whether it's probably installed via the addon gallery - e.g. "development" if loaded via about:debugging and "normal" otherwise).
Blocks: 1303905

Comment 9

10 months ago
Here is our use case for this feature:

We are an international market research company that recruits volunteers to install software (including a Firefox extension) that automatically reports on their media consumption and web usage. We would like to get information about other installed extensions so that we can report statistics such as marketshare and install base to our clients.

We currently report this information for Firefox using a legacy extension, but will lose this data when switching to WebExtensions. There is a potential for Firefox extension authors and Mozilla to be impacted by this if they happen to be clients as well.

Comment 10

9 months ago
This is a use case that I'm already using in Chrome & Opera.

I have an extension for OGame, a browser game, called UniverseView.
There are also a lot of other extensions and this API helps me to detect which extensions are installed.
When a certain extension is installed my extension doesn't execute certain code to avoid conflicts.

Comment 11

8 months ago
One more use case for this: the weh toolkit (https://github.com/mi-g/weh) is a set of tools to help developing WebExtensions add-ons. It contains an extension, weh-inspector, able to monitor messages, preferences and storage from weh-based installed extensions. The lack of browser.management.getAll API prevents weh-inspector to discover automatically what other add-ons can be inspected.

Note that the lack of runtime.messageExternal support (bug#1258360) is even more critical for weh-inspector to run on Firefox.

Comment 12

6 months ago
Another case here: in Adnauseam(https://github.com/dhowe/AdNauseam), we use management.getAll to check for enabled extensions that may conflict (other adblockers making modifications to request headers, etc), and then warn the user that only one should probably be used at the same time.

Updated

6 months ago
Duplicate of this bug: 1336689

Comment 14

6 months ago
I have a use case for this bug: Persona Switcher (https://github.com/drsjb80/personaswitcher) is an add-on that lets you easily switch between personas/themes in Mozilla Firefox, Thunderbird, and SeaMonkey.  Currently we are working on migrating this add-on to a WebExtension, however, we will lose the functionality and be unable to switch between personas once we do this. The management API would help us access the themes and switch between them in Firefox with the getAll() and setEnabled() functions

Updated

6 months ago
Duplicate of this bug: 1336908
(Assignee)

Comment 16

2 months ago
Bug 1336908 implemented management.getAll but for themes only. Based on all the use cases listed above, and the inclusion of this bug on Andy's list of papercuts, do we want to remove that restriction to allow management.getAll to return all extensions?
Flags: needinfo?(amckay)

Comment 17

2 months ago
I see many good reasons for having APIs and features other browsers don't have, like file access, sockets, user translations, ... but none to skip some, like management.getAll, that are available elsewhere, even more when we have valid use cases.

Comment 18

2 months ago
(In reply to Bob Silverberg [:bsilverberg] from comment #16)
> Bug 1336908 implemented management.getAll but for themes only. Based on all
> the use cases listed above, and the inclusion of this bug on Andy's list of
> papercuts, do we want to remove that restriction to allow management.getAll
> to return all extensions?

Fine by me.
Flags: needinfo?(amckay)
Keywords: dev-doc-needed
(Assignee)

Comment 19

a month ago
Alright, I will implement management.getAll for all extensions, but I am removing management.get from this bug as we're not going to implement that, at least not now.
Assignee: nobody → bob.silverberg
Summary: Implement management.get and getAll → Implement management.getAll
(In reply to Bob Silverberg [:bsilverberg] from comment #19)
> Alright, I will implement management.getAll for all extensions, but I am
> removing management.get from this bug as we're not going to implement that,
> at least not now.

Why?  get is almost a copy/paste of getSelf, just a couple lines of code.
Comment hidden (mozreview-request)
(Assignee)

Comment 22

a month ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> (In reply to Bob Silverberg [:bsilverberg] from comment #19)
> > Alright, I will implement management.getAll for all extensions, but I am
> > removing management.get from this bug as we're not going to implement that,
> > at least not now.
> 
> Why?  get is almost a copy/paste of getSelf, just a couple lines of code.

Fair enough. Initially I was thinking that we didn't implement get for a reason, and it doesn't seem like any use cases have presented themselves for it, but I guess if we're implementing getAll that will give developers all the same information, so we might as well implement get too.
(Assignee)

Updated

a month ago
Summary: Implement management.getAll → Implement management.get and getAll
sorry for the drive by but this patch should also include a permissions string for the management permissions
Comment hidden (mozreview-request)

Comment 25

a month ago
mozreview-review
Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review151962

Also ditto on the permission string.

::: toolkit/components/extensions/ext-management.js:92
(Diff revision 2)
>    return extInfo;
>  }
>  
>  const listenerMap = new WeakMap();
> -// Some management APIs are intentionally limited.
> -const allowedTypes = ["theme"];
> +// Some management APIs are intentionally limited to themes.
> +const themeType = "theme";

If we're allowing get/getAll to actually access any, I see no reason to limit the events to themes.  We should just set allowedTypes = [theme,extension].

::: toolkit/components/extensions/ext-management.js:212
(Diff revision 2)
>          async setEnabled(id, enabled) {
>            let addon = await AddonManager.getAddonByID(id);
>            if (!addon) {
>              throw new ExtensionError(`No such addon ${id}`);
>            }
> -          if (!allowedTypes.includes(addon.type)) {
> +          if (addon.type !== themeType) {

Is installType available?  If so, I'm wondering if we should not allow changing enabled state of installType=="admin".  If "admin" is not used, we should have a followup bug, I think system addons should have "admin".

::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:62
(Diff revision 2)
> +      let themes = addons.filter(addon => addon.type === "theme");
>        // We get the 3 built-in themes plus the lwt and our addon.
> -      browser.test.assertEq(5, addons.length, "got expected addons");
> +      browser.test.assertEq(5, themes.length, "got expected addons");
> +      // We should also get our test extension.
> +      let testExtension = addons.find(addon => { return addon.id === TEST_ID; });
> +      browser.test.assertEq(TEST_ID, testExtension.name,

This is confusing until you read further down.  Why not just assertTrue(!!testExtension)?

Comment 26

a month ago
mozreview-review
Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review151976
Attachment #8876142 - Flags: review?(mixedpuppy) → review-
(In reply to Shane Caraveo (:mixedpuppy) from comment #25)

> >  const listenerMap = new WeakMap();
> > -// Some management APIs are intentionally limited.
> > -const allowedTypes = ["theme"];
> > +// Some management APIs are intentionally limited to themes.
> > +const themeType = "theme";
> 
> If we're allowing get/getAll to actually access any, I see no reason to
> limit the events to themes.  We should just set allowedTypes =
> [theme,extension].

Just a further comment on this.  If a supported use case is to warn users about incompatible addons per comment 12, using the events to know that one is later installed is important as well.
(Assignee)

Comment 28

a month ago
(In reply to Andrew Swan [:aswan] from comment #23)
> sorry for the drive by but this patch should also include a permissions
> string for the management permissions

I wonder what this should say? My initial thought is something along the lines of "Manage add-ons and themes" (or is "themes" redundant if we're saying "add-ons"?), but this API is actually limited to just reading information about and monitoring add-ons in general. It only allows modification to themes, and on in terms of enabling/disabling them. So is "Manage add-ons and themes" too broad, or does it sound okay?
Flags: needinfo?(scaraveo)
Flags: needinfo?(aswan)
(Assignee)

Comment 29

a month ago
My comment above should read "...and only in terms of enabling/disabling them."
Comment hidden (mozreview-request)
(Assignee)

Comment 31

a month ago
mozreview-review-reply
Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review151962

I added a proposed permissions string into this patch, but I'm not sure if it is a good string.

> Is installType available?  If so, I'm wondering if we should not allow changing enabled state of installType=="admin".  If "admin" is not used, we should have a followup bug, I think system addons should have "admin".

`installType` is something we calculate in ext-management.js, and it doesn't currently generate a value of "admin". An addon can have a `isSystem` property and if that is set to "true" then we set `installType` to "other". I think `isSystem` corresponds to a system add-on, which you are referring to above as "admin", so we could use that as a test, and prevent changes to enabled for any addon where `isSystem` is "true". Looking at the various themes that are installed while running the test, none of them have an installType of "other". The ones that seem to be pre-packaged, such as "firefox-compact-light@mozilla.org@personas.mozilla.org" have an installType of "normal", so I wonder if "system themes" is even something that will ever exist, and if not, is a check for `isSystem` neccesary? I have added the change to support that to my latest commit, but please consider what I've written above in terms of whether this actually a change we want/need.
(In reply to Bob Silverberg [:bsilverberg] from comment #28)
> (In reply to Andrew Swan [:aswan] from comment #23)
> > sorry for the drive by but this patch should also include a permissions
> > string for the management permissions
> 
> I wonder what this should say? My initial thought is something along the
> lines of "Manage add-ons and themes" (or is "themes" redundant if we're
> saying "add-ons"?), but this API is actually limited to just reading
> information about and monitoring add-ons in general. It only allows
> modification to themes, and on in terms of enabling/disabling them. So is
> "Manage add-ons and themes" too broad, or does it sound okay?

By making a single api read-only for extensions and read/write for themes, we have made it difficult to describe concisely.
I would avoid a work like "manage" since isn't very precise.  But over to Scott...
Flags: needinfo?(aswan) → needinfo?(sdevaney)
(In reply to Bob Silverberg [:bsilverberg] from comment #31)
> Comment on attachment 8876142 [details]
> Bug 1282981 - Implement management.get and getAll,
> 
> https://reviewboard.mozilla.org/r/147560/#review151962
> 
> I added a proposed permissions string into this patch, but I'm not sure if
> it is a good string.
> 
> > Is installType available?  If so, I'm wondering if we should not allow changing enabled state of installType=="admin".  If "admin" is not used, we should have a followup bug, I think system addons should have "admin".
> 
> `installType` is something we calculate in ext-management.js, and it doesn't
> currently generate a value of "admin". An addon can have a `isSystem`
> property and if that is set to "true" then we set `installType` to "other".
> I think `isSystem` corresponds to a system add-on, which you are referring
> to above as "admin", so we could use that as a test, and prevent changes to
> enabled for any addon where `isSystem` is "true". Looking at the various
> themes that are installed while running the test, none of them have an
> installType of "other". The ones that seem to be pre-packaged, such as
> "firefox-compact-light@mozilla.org@personas.mozilla.org" have an installType
> of "normal", so I wonder if "system themes" is even something that will ever
> exist, and if not, is a check for `isSystem` neccesary? I have added the
> change to support that to my latest commit, but please consider what I've
> written above in terms of whether this actually a change we want/need.

I haven't looked at the patch but we don't display system add-ons in about:addons.  Looking over the use cases that motivated this, none of them seem to require having system add-ons returned and some appear like they would actually be better not seeing them.
(Assignee)

Comment 34

a month ago
(In reply to Andrew Swan [:aswan] from comment #33)
> 
> I haven't looked at the patch but we don't display system add-ons in
> about:addons.  Looking over the use cases that motivated this, none of them
> seem to require having system add-ons returned and some appear like they
> would actually be better not seeing them.

Interesting point. Yes, it might be better to filter out system add-ons from the list of add-ons that get returned. It probably makes sense to apply that to the events as well, and not emit an event if the add-on in question is a system add-on. What do you think, Shane?
Yeah, I think that's a good solution.

Just to comment on themes not being system addons, addons can distributed in Firefox without them being system.

Comment 36

a month ago
mozreview-review
Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review152468

::: browser/locales/en-US/chrome/browser/browser.properties:104
(Diff revision 3)
>  webextPerms.description.clipboardRead=Get data from the clipboard
>  webextPerms.description.clipboardWrite=Input data to the clipboard
>  webextPerms.description.downloads=Download files and read and modify the browser’s download history
>  webextPerms.description.geolocation=Access your location
>  webextPerms.description.history=Access browsing history
> +webextPerms.description.management=Manage add-ons and themes

Use extension, not add-on

::: toolkit/components/extensions/ext-management.js:212
(Diff revision 3)
>          async setEnabled(id, enabled) {
>            let addon = await AddonManager.getAddonByID(id);
>            if (!addon) {
>              throw new ExtensionError(`No such addon ${id}`);
>            }
> -          if (!allowedTypes.includes(addon.type)) {
> +          if (addon.type !== "theme") {

I think we should leave this checking allowedTypes, log who is disabling what.

::: toolkit/components/extensions/ext-management.js:215
(Diff revision 3)
>              throw new ExtensionError(`No such addon ${id}`);
>            }
> -          if (!allowedTypes.includes(addon.type)) {
> +          if (addon.type !== "theme") {
>              throw new ExtensionError("setEnabled applies only to theme addons");
>            }
> +          if (addon.isSystem) {

Per bug discussion, lets filter out system addons everywhere.
Attachment #8876142 - Flags: review?(mixedpuppy) → review-
(Assignee)

Comment 37

a month ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #36)
> Comment on attachment 8876142 [details]
> Bug 1282981 - Implement management.get and getAll,
> 
> https://reviewboard.mozilla.org/r/147560/#review152468
> 
> ::: toolkit/components/extensions/ext-management.js:212
> (Diff revision 3)
> >          async setEnabled(id, enabled) {
> >            let addon = await AddonManager.getAddonByID(id);
> >            if (!addon) {
> >              throw new ExtensionError(`No such addon ${id}`);
> >            }
> > -          if (!allowedTypes.includes(addon.type)) {
> > +          if (addon.type !== "theme") {
> 
> I think we should leave this checking allowedTypes, log who is disabling
> what.
>

I think we decided long ago that we didn't want to allow extensions to disable other extensions. We made an explicit exception for themes, but I don't think we now want to open it up so any extension can disable any other extension, which is what I believe you are suggesting. I'll check with Andy to see what he thinks.
Flags: needinfo?(amckay)

Comment 38

a month ago
> I think we decided long ago that we didn't want to allow extensions to
> disable other extensions. We made an explicit exception for themes, but I
> don't think we now want to open it up so any extension can disable any other
> extension, which is what I believe you are suggesting. I'll check with Andy
> to see what he thinks.

Most definitely: extensions should not be able to disable other extensions. We've made an exception for themse.
Flags: needinfo?(amckay)
Flags: needinfo?(scaraveo)
Comment hidden (mozreview-request)
(Assignee)

Comment 40

a month ago
mozreview-review-reply
Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review152468

> Use extension, not add-on

I made this change, but I think the string is still very poor. Let's wait to see what Scott suggests.

> I think we should leave this checking allowedTypes, log who is disabling what.

As per Andy's response, we do not want to allow extensions to disable other extensions, so I think this code needs to stay as is.

Comment 41

a month ago
mozreview-review
Comment on attachment 8876142 [details]
Bug 1282981 - Implement management.get and getAll,

https://reviewboard.mozilla.org/r/147560/#review152588
Attachment #8876142 - Flags: review?(mixedpuppy) → review+

Comment 42

a month ago
Yea I'm not sure we can achieve poetry with this message, but do one of these two options ring true?:

Monitors extension usage and manages themes

Reads extensions and manages themes
Flags: needinfo?(sdevaney)
(Assignee)

Comment 43

a month ago
(In reply to sdevaney from comment #42)
> Yea I'm not sure we can achieve poetry with this message, but do one of
> these two options ring true?:
> 
> Monitors extension usage and manages themes
> 
> Reads extensions and manages themes

Thanks Scott. Yeah, neither of those are beautiful either, but not sure what would be better. Andrew suggested that he didn't like using the term "manage" back in comment #32, but I don't have any better ideas.

I'm going to go with "Monitors extension usage and manages themes" for now.
Comment hidden (mozreview-request)

Comment 45

a month ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe3ed659f568
Implement management.get and getAll, r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/fe3ed659f568
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.