Implement management.getAll and management.setEnabled for themes

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
P3
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: Stefany Segovia, Assigned: mixedpuppy)

Tracking

({dev-doc-complete})

51 Branch
mozilla55
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [design-decision-approved][triaged])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
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.
(Reporter)

Updated

10 months ago
Component: Untriaged → Extension Compatibility
Summary: Implement management getAll and setEnabled for Firefox browser → Implement management.getAll and management.setEnabled for Firefox browser
(Reporter)

Comment 1

10 months ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(amckay)

Updated

10 months ago
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Flags: needinfo?(amckay)
Resolution: --- → DUPLICATE
Duplicate of bug: 1282981

Comment 4

10 months ago
And also bug 1282982.

Comment 5

10 months ago
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.
Status: RESOLVED → REOPENED
Component: Extension Compatibility → WebExtensions: General
Product: Firefox → Toolkit
Resolution: DUPLICATE → ---
Summary: Implement management.getAll and management.setEnabled for Firefox browser → Implement management.getAll and management.setEnabled for themes
Whiteboard: [design-decision-approved][triaged]

Updated

10 months ago
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Assignee: nobody → mixedpuppy
(Assignee)

Comment 7

7 months ago
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.
Attachment #8861267 - Flags: feedback?(aswan)

Comment 8

7 months ago
mozreview-review
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?
(Assignee)

Comment 9

7 months ago
(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.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

7 months ago
(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?
Flags: needinfo?(amckay)

Comment 12

7 months ago
(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.
Flags: needinfo?(amckay)

Comment 13

7 months ago
mozreview-review
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
Attachment #8861267 - Flags: review?(aswan) → review+
(Assignee)

Comment 14

7 months ago
(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?
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
(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.
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

7 months ago
mozreview-review
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?
(Assignee)

Comment 23

7 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)

Comment 25

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

Comment 26

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0df833bf2b9f
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.