Bugzilla

Updated

•

7 years ago

Status: NEW → RESOLVED

Closed: 7 years ago

Flags: needinfo?(amckay)

Resolution: --- → DUPLICATE

Comment 4

•

7 years ago

And also bug 1282982.

Comment 5

•

7 years 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

•

7 years ago

Priority: -- → P3

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/133214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133214/

Assignee

Updated

•

7 years ago

Assignee: nobody → mixedpuppy

Assignee

Comment 7

•

7 years 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 years 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 years 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)

Comment on attachment 8861267 [details]
Bug 1336908 implement management APIs needed for theme management,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133214/diff/1-2/

Assignee

Comment 11

•

7 years 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 years 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 years 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 years 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)

Comment on attachment 8861267 [details]
Bug 1336908 implement management APIs needed for theme management,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133214/diff/2-3/

Comment 16

•

7 years ago

(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 on attachment 8861267 [details]
Bug 1336908 implement management APIs needed for theme management,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133214/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8861267 [details]
Bug 1336908 implement management APIs needed for theme management,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133214/diff/4-5/

Comment hidden (mozreview-request)

Comment on attachment 8861267 [details]
Bug 1336908 implement management APIs needed for theme management,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133214/diff/5-6/

Comment hidden (mozreview-request)

Comment on attachment 8861267 [details]
Bug 1336908 implement management APIs needed for theme management,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133214/diff/6-7/

Comment hidden (mozreview-request)

Comment on attachment 8861267 [details]
Bug 1336908 implement management APIs needed for theme management,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133214/diff/7-8/

Comment 22

•

7 years 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?