User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36 Steps to reproduce: We are currently in the process of migrating our Persona Switcher add-on (https://github.com/drsjb80/personaswitcher) to a WebExtension. Persona Switcher is an add-on that lets you easily switch between personas/themes in Mozilla Firefox, Thunderbird, and SeaMonkey. Currently, as an add-on, we are grabbing the themes with the LightWeightThemeManager library (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm) where it allows us to access all the themes and switch between them by enabling/disabling them. Since moving to a WebExtension only has a limited amount of API's available, we looked into a way to be able to grab themes with the API's listed in (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Browser_support_for_JavaScript_APIs). We found out that the management API would let us grab the themes with getAll() since the themes are considered add-ons. We then can enable them with the setEnabled() function. Actual results: The getAll() and setEnabled() functions in the management API are currently not compatible with the Firefox browser. This prevents us from grabbing themes and switching through them. Expected results: The getAll() and setEnabled() functions in the management API should be compatible with the Firefox browser and have the ability to grab the themes and provide us the necessary functionality so that we can switch through the themes. We know that we need these specific functions for sure because we created a small prototype extension for chrome using these functions, since they are compatible with chrome, and it gave us an example of the functionality we needed for our add-on.

The add-on is here: https://addons.mozilla.org/en-US/firefox/addon/personaswitcher/

Forwarding to Andy to make sure this gets on his radar.

And also bug 1282982.

Supporting this use case for themes would be useful. Rather than implement a custom theme API it would make sense to implement the management API and limit to only themes. Currently the management API takes a type "theme". We should do the same. Anything beyond themes would require a permission and consideration if we wanted to do this. So we'd suggest there should be a discreet permission for this: managementThemes that must be required to get management of themes.

Comment on attachment 8861267 [details] Bug 1336908 implement management APIs needed for theme management, aswan, you've been in addon manager land more recently than I. There are a couple strange issues I see in using it to implement some theme support in the management api, if you could take a look that would be great.

Comment on attachment 8861267 [details] Bug 1336908 implement management APIs needed for theme management, https://reviewboard.mozilla.org/r/133214/#review136028 I commented on the TODOs and one other question, not sure if I overlooked any of the strange issues you mentioned? ::: toolkit/components/extensions/ext-management.js:47 (Diff revision 1) > - }), > - hostPermissions: extension.whiteListedHosts.pat, > - installType: installType(addon), > + installType: installType(addon), > + type: addon.type, > - }; > + }; > + // TODO Should we figure out how to do this if we don't have an extension object? If this is just for themes, then I think its safe to return empty lists for permissions and hostPermissions. For disabled webextensions, we don't have this information available. And I am not inclined to worry at all about legacy extensions. ::: toolkit/components/extensions/ext-management.js:78 (Diff revision 1) > + getAll: function() { > + return new Promise((resolve, reject) => AddonManager.getAddonsByTypes(["webextension-theme"], addons => { I would just make this `async function getAll() { ...` and then `await AddonManager.getAddonsByTypes(..);` ::: toolkit/components/extensions/ext-management.js:83 (Diff revision 1) > + getAll: function() { > + return new Promise((resolve, reject) => AddonManager.getAddonsByTypes(["webextension-theme"], addons => { > + let result = []; > + for (let addon of addons) { > + if (addon.type !== "theme") { > + // Why do we get all addons here? does this comment mean this is returning things that aren't themes?

(In reply to Andrew Swan [:aswan] from comment #8) > Comment on attachment 8861267 [details] > Bug 1336908 implement management APIs needed by PersonaSwitcher > > https://reviewboard.mozilla.org/r/133214/#review136028 > > I commented on the TODOs and one other question, not sure if I overlooked > any of the strange issues you mentioned? I think those came from using webextension-theme. Still may be an issue in general, but just using "theme" made everything work as I expected.

(In reply to Andy McKay [:andym] from comment #5) > Anything beyond themes would require a permission and consideration if we > wanted to do this. So we'd suggest there should be a discreet permission for > this: managementThemes that must be required to get management of themes. I'm inclined towards not creating a new permission for theme management, and the current patch does not have that. Do you feel strongly about this, if so why?

(In reply to Shane Caraveo (:mixedpuppy) from comment #11) > I'm inclined towards not creating a new permission for theme management, and > the current patch does not have that. Do you feel strongly about this, if > so why? I don't think it needs a new permission.

Comment on attachment 8861267 [details] Bug 1336908 implement management APIs needed for theme management, https://reviewboard.mozilla.org/r/133214/#review137072 A bunch of nits. The only big thing is, I think the test should also cover a lightweight theme. I think leaving out complete themes is fine since those are on the verge of extinction... ::: toolkit/components/extensions/ext-management.js:59 (Diff revision 2) > + type: addon.type, > - }; > + }; > + > + if (extension) { > + let m = extension.manifest; > + extInfo.permissions = Array.from(extension.permissions).filter(perm => { Can you mark permissions as optional in the schema? I don't think it should be here at all for themes but that's a bigger issue I guess, like bug 1344243. Also other properties that you only set if there's an extension: hostPermissions, shortName, optionsURL ::: toolkit/components/extensions/ext-management.js:84 (Diff revision 2) > - } > + } > - if (m.icons) { > - extInfo.icons = Object.keys(m.icons).map(key => { > - return {size: Number(key), url: m.icons[key]}; > + return extInfo; > +} > + > +let listenerMap = new WeakMap(); > +let allowedTypes = ["theme"]; let -> const ::: toolkit/components/extensions/ext-management.js:130 (Diff revision 2) > + } > + this.emit("onUninstalled", this.getExtensionInfo(addon)); > + } > +} > + > +function getListener(extension, context) { why do you need one of these per extension? won't a single global instance do? then you don't need the listenerMap... ::: toolkit/components/extensions/ext-management.js:155 (Diff revision 2) > - reject(err); > + let {extension} = context; > + return { > + management: { > + async getAll() { > + let result = []; > + await AddonManager.getAddonsByTypes(allowedTypes, addons => { Most AddonManager methods behave kind of like `browser.*` webextensions apis and return a promise if you leave out the callback. Which means that this can just be `let addons = await AddonManager.get...` ::: toolkit/components/extensions/ext-management.js:156 (Diff revision 2) > + for (let addon of addons) { > + // If the extension is enabled get it and use it for more data. > + let ext = addon.isWebExtension && GlobalManager.extensionMap.get(addon.id); > + result.push(getExtensionInfoForAddon(ext, addon)); > } this could be `let result = addons.map(addon => { ... });` ::: toolkit/components/extensions/ext-management.js:186 (Diff revision 2) > - let response = promptService.confirmEx(null, title, message, buttonFlags, button0Title, button1Title, null, null, {value: 0}); > + let response = promptService.confirmEx(null, title, message, buttonFlags, button0Title, button1Title, null, null, {value: 0}); > - if (response == 1) { > + if (response == 1) { > - return reject({message: "User cancelled uninstall of extension"}); > + throw new ExtensionError("User cancelled uninstall of extension"); > - } > + } > - } > + } > - AddonManager.getAddonByID(extension.id, addon => { > + await AddonManager.getAddonByID(extension.id, addon => { same comment as above ::: toolkit/components/extensions/ext-management.js:196 (Diff revision 2) > - } > - }); > + }); > + }, > + > + async setEnabled(id, enabled) { > + await AddonManager.getAddonByID(id, addon => { same comment as above ::: toolkit/components/extensions/ext-management.js:209 (Diff revision 2) > }); > }, > + > + onDisabled: new SingletonEventManager(context, "management.onDisabled", fire => { > + let listener = (event, data) => { > + fire.sync(data); why not async? ::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:62 (Diff revision 2) > + }); > + > + async function getAddon(type) { > + let addons = await browser.management.getAll(); > + // We get the 3 built-in themes plus our addon. > + browser.test.assertEq(4, addons.length, "got an addon"); The "got an addon" string is a little misleading here ::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:64 (Diff revision 2) > + async function getAddon(type) { > + let addons = await browser.management.getAll(); > + // We get the 3 built-in themes plus our addon. > + browser.test.assertEq(4, addons.length, "got an addon"); > + for (let addon of addons) { > + browser.test.assertEq(addon.type, "theme", "addon is theme"); do you mean to test this for all entries in the returned array? because this function returns early below. ::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:106 (Diff revision 2) > + await theme.startup(); > + await extension.awaitMessage("onInstalled"); > + // no enabled event when installed. > + > + extension.sendMessage("test"); > + await Promise.all([ The Promise.all() isn't doing anything here... ::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:112 (Diff revision 2) > + await extension.awaitMessage("done"), > + await extension.awaitMessage("onDisabled"), > + await extension.awaitMessage("onEnabled"), > + ]); > + > + await Promise.all([ same here

(In reply to Andrew Swan [:aswan] from comment #13) > Comment on attachment 8861267 [details] > Bug 1336908 implement management APIs needed for theme management, > > https://reviewboard.mozilla.org/r/133214/#review137072 > > A bunch of nits. The only big thing is, I think the test should also cover > a lightweight theme. I think leaving out complete themes is fine since > those are on the verge of extinction... IIUC the only options for getAddonsByTypes are "extension" or "theme", theme includes any kind of theme. Do I have that wrong? Or am I misunderstanding covering lightweight themes?

(In reply to Shane Caraveo (:mixedpuppy) from comment #14) > (In reply to Andrew Swan [:aswan] from comment #13) > > Comment on attachment 8861267 [details] > > Bug 1336908 implement management APIs needed for theme management, > > > > https://reviewboard.mozilla.org/r/133214/#review137072 > > > > A bunch of nits. The only big thing is, I think the test should also cover > > a lightweight theme. I think leaving out complete themes is fine since > > those are on the verge of extinction... > > IIUC the only options for getAddonsByTypes are "extension" or "theme", theme > includes any kind of theme. Do I have that wrong? Or am I misunderstanding > covering lightweight themes? That's right, but lightweight themes have type "theme" and the test doesn't currently check that all the new code works correctly when examining, enabling, etc. lightweight themes.

Comment on attachment 8861267 [details] Bug 1336908 implement management APIs needed for theme management, https://reviewboard.mozilla.org/r/133214/#review139330 ::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:109 (Diff revisions 3 - 8) > + textcolor: Math.random().toString(), > + accentcolor: Math.random().toString(), I don't think these are valid colors? Not that it matters for this test but it caught my eye... ::: toolkit/components/extensions/test/browser/browser_ext_management_themes.js:112 (Diff revisions 3 - 8) > + previewURL: "http://mochi.test:8888/data/preview.png", > + iconURL: "http://mochi.test:8888/data/icon.png", > + textcolor: Math.random().toString(), > + accentcolor: Math.random().toString(), > + }; > + Assert.ok(await extension.awaitMessage("onInstalled") === "Bling", "LWT installed"); i don't think you need the `Assert.` prefix (throughout the file) ::: mobile/android/components/extensions/test/mochitest/mochitest.ini:4 (Diff revision 8) > [DEFAULT] > support-files = > ../../../../../../toolkit/components/extensions/test/mochitest/test_ext_all_apis.js > + ../../../../../../toolkit/components/extensions/test/mochitest/file_sample.html what's this for?

Comment on attachment 8861267 [details] Bug 1336908 implement management APIs needed for theme management, https://reviewboard.mozilla.org/r/133214/#review139330 > I don't think these are valid colors? Not that it matters for this test but it caught my eye... heh, probably not, the lwt object is a copy/paste from another test. Doesn't affect anything. > what's this for? test_ext_all_apis uses that file, but it was not included. that produced a 404 that linux debug liked to timeout on.

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0df833bf2b9f implement management APIs needed for theme management, r=aswan

I've updated the compat data for browser.management: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/management#Browser_compatibility Marking this as dev-doc-complete.