Closed Bug 1338525 Opened 7 years ago Closed 7 years ago

Theming API - Themes should be required to contain only static resources.

Categories

(WebExtensions :: Frontend, defect)

Product:
WebExtensions
Component:
Frontend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mattw, Assigned: mattw)

References

Details

(Whiteboard: user-story, triaged)

User Story

As a developer, I'd like to have my themes show up on AMO and in the Appearance section of about:addons. I understand that themes are different from extensions, and since my themes only have static resources, they should show up in these areas.

Attachments

(2 files)

Bug 1338525 - Add schema validation for webextension themes
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1338525 - Add test coverage for theme validation
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
      No description provided.
User Story: (updated)
Assignee: nobody → mwein
Attachment #8836066 - Flags: review?(mdeboer)
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review112876

::: browser/components/extensions/test/xpcshell/test_ext_manifest_theme.js:40
(Diff revision 1)
> +
> +  // test supported properties
> +  supportedProperties.forEach(prop => {
> +    let manifest = {
> +      "theme": {},
> +      [prop]: {},

oh cool, I didn't know about this syntax before.

::: toolkit/components/extensions/Schemas.jsm:2134
(Diff revision 1)
> +   *
> +   * @param {object} obj The object to validate.
> +   * @returns {boolean} true if the `obj` contains a "theme" property and
> +   *     only static resources; false otherwise.
> +   */
> +  isTheme(obj) {

Should this function be called from within Schemas.jsm at some point?
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review113318

Clearing review, I think Matt is working on a new version of the patch. Please correct if wrong.
Attachment #8836066 - Flags: review?(jaws)
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

If so, I'm clearing mine too ;-)
Attachment #8836066 - Flags: review?(mdeboer)
Attachment #8836066 - Flags: feedback?(kmaglione+bmo)
Summary: Theming API - provide a mechanism for determining whether an extension is a theme → Theming API - Themes should be required to contain only static resources.
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review114874

Looks like a good start to me! I really like your way of constructing a whitelist using our blacklist :D

It does need a few changes, though. Please let me know if you have any questions. Also, I think you'll need to give a bit more background info to Kris, before he's able to review this as well.

::: browser/components/extensions/ext-theme.js:146
(Diff revision 2)
>  extensions.registerSchemaAPI("theme", "addon_parent", context => {
>    let {extension} = context;
>    return {
>      theme: {
>        update(details) {
> +        if (!Preferences.get("extensions.webextensions.themes.enabled")) {

This is a hot code-path, potentially. Can we make this a lazy getter?
We won't support flipping this pref during the app's lifetime, so we don't listen to changes.

::: browser/components/extensions/ext-theme.js:152
(Diff revision 2)
> +          // Return early if themes are disabled.
> +          return;
> +        }
> +
>          let theme = themeMap.get(extension);
>  

nit: Can you remove this newline?

::: browser/components/extensions/test/browser/browser_ext_themes_validation.js:3
(Diff revision 2)
> +"use strict";
> +
> +add_task(function* setup() {

Can you add a comment here that explains what you're testing here?

::: toolkit/components/extensions/Extension.jsm:409
(Diff revision 2)
>    }
>  
> +  /**
> +   * Validates a theme to ensure it only contains static resources.
> +   *
> +   * @throws if theme is invalid.

It doesn't really 'throw'

::: toolkit/components/extensions/Extension.jsm:413
(Diff revision 2)
> +   *
> +   * @throws if theme is invalid.
> +   * @param {object} context The extension's context.
> +   */
> +  validateTheme(context) {
> +    let blacklist = ["background", "content_scripts", "permissions"];

This looks like a `const` to me...

::: toolkit/components/extensions/Extension.jsm:415
(Diff revision 2)
> +   * @param {object} context The extension's context.
> +   */
> +  validateTheme(context) {
> +    let blacklist = ["background", "content_scripts", "permissions"];
> +    let baseProperties = ExtensionParent.getBaseManifestProperties();
> +    let whitelist = baseProperties.filter(prop => !blacklist.includes(prop));

I think this data is so static that it can be cached/ made into a lazy getter.

::: toolkit/components/extensions/Extension.jsm:426
(Diff revision 2)
> +      }
> +    }
> +
> +    if (invalidProps.length) {
> +      let message = `Static theme contains invalid properties: ${invalidProps}`;
> +      context.logError(message);

I have the feeling that this does *NOT* reject the extension when it fails validation and happily continues loading.
I was hoping you'd make this behave like `Schemas.normalize()`:
```js
// Calling code:
if (!!this.manifest.theme) {
  let result = this.validateTheme(context);
  if (results.error) {
    this.manifestError(result.error);
    // And if this doesn't throw, because of kmag's earlier work, we should implement that ourselves.
  }
}
```
What do you think?

::: toolkit/components/extensions/Extension.jsm:456
(Diff revision 2)
>          },
>  
>          preprocessors: {},
>        };
>  
> +      if ("theme" in this.manifest) {

Please see my comment above. Also, this will validate when `manifest.theme == null`, so please make this `if (!!this.manifest.theme) {`

::: toolkit/components/extensions/ExtensionParent.jsm:906
(Diff revision 2)
> +  let types = Schemas.schemaJSON.get(BASE_SCHEMA)[0].types;
> +  let manifest = types.find(type => type.id === "WebExtensionManifest");
> +  if (!manifest) {
> +    throw new Error("Unabled to find base manifest properties");
> +  }
> +  return Object.keys(manifest.properties);

Please turn this into a lazy getter:

```js
var gBaseManifestProperties = null;
const ExtensionParent = {
  //...
  get baseManifestProperties() {
    if (!gBaseManifestProperties) {
      // let types = ... etc.
    }
    return gBaseManifestProperties;
  }
};
```

Also, please attempt to keep this list sorted alphabetically. Or at least *something* less random ;)
Attachment #8836066 - Flags: review?(mdeboer) → review-
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review114874

Thanks for the review!

> This is a hot code-path, potentially. Can we make this a lazy getter?
> We won't support flipping this pref during the app's lifetime, so we don't listen to changes.

Yeah, that sounds good to me.

> nit: Can you remove this newline?

Done.

> Can you add a comment here that explains what you're testing here?

Sure!

> It doesn't really 'throw'

Good catch, I updated the comment to be more clear.

> This looks like a `const` to me...

It is :) though using `const` doesn't prevent the items in the array from being changed, I think it helps get the point across that it shouldn't be modified.

> I think this data is so static that it can be cached/ made into a lazy getter.

Sounds good to me :)

> I have the feeling that this does *NOT* reject the extension when it fails validation and happily continues loading.
> I was hoping you'd make this behave like `Schemas.normalize()`:
> ```js
> // Calling code:
> if (!!this.manifest.theme) {
>   let result = this.validateTheme(context);
>   if (results.error) {
>     this.manifestError(result.error);
>     // And if this doesn't throw, because of kmag's earlier work, we should implement that ourselves.
>   }
> }
> ```
> What do you think?

You're right, thank you for catching this! Switching to use `this.manifestError` does reject the extension if it fails validation.

The difference with `Schemas.normalize()` and this is that `Schemas.normalize()` doesn't have a reference to `this.manifestError` and it also returns something if no errors are thrown. Therefore I think my current approach is better here, after adding in the change to actually reject extensions that fail validation.

> Please see my comment above. Also, this will validate when `manifest.theme == null`, so please make this `if (!!this.manifest.theme) {`

Ah, I see, thanks.

> Please turn this into a lazy getter:
> 
> ```js
> var gBaseManifestProperties = null;
> const ExtensionParent = {
>   //...
>   get baseManifestProperties() {
>     if (!gBaseManifestProperties) {
>       // let types = ... etc.
>     }
>     return gBaseManifestProperties;
>   }
> };
> ```
> 
> Also, please attempt to keep this list sorted alphabetically. Or at least *something* less random ;)

I'm already switching to use a lazy getter for the whitelist which does the job of caching this.
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

Kris, I've added you for feedback so you can make sure what I'm doing in Extension.jsm and ExtensionParent.jsm seems reasonable.
Attachment #8836066 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review115410

::: browser/components/extensions/ext-theme.js:123
(Diff revision 3)
>    }
>  }
>  
>  /* eslint-disable mozilla/balanced-listeners */
>  extensions.on("manifest_theme", (type, directive, extension, manifest) => {
> -  if (!Preferences.get("extensions.webextensions.themes.enabled")) {
> +  if (!themesEnabled) {

nit: please rename this to `gThemesEnabled`

::: browser/components/extensions/test/browser/browser_ext_themes_validation.js:15
(Diff revision 3)
> +
> +/**
> + * Helper function for testing a theme with invalid properties.
> + * @param {object} invalidProps The invalid properties to load the theme with.
> + */
> +function* run(invalidProps) {

nit: can you rename this test to something that explains a bit more what it's doing?

::: browser/components/extensions/test/browser/browser_ext_themes_validation.js:22
(Diff revision 3)
> +    "theme": {},
> +  };
> +
> +  invalidProps.forEach(prop => {
> +    // Some properties require additional information:
> +    if (prop === "background") {

nit: perhaps a switch statement would look better here?

::: toolkit/components/extensions/ExtensionParent.jsm:440
(Diff revision 3)
>    get devToolsTarget() {
>      return this._devToolsTarget;
>    }
>  
>    shutdown() {
> -    if (this._devToolsTarget) {
> +    if (!this._devToolsTarget) {

Can you explain why you needed to make this change?

::: toolkit/components/extensions/ExtensionParent.jsm:904
(Diff revision 3)
> + */
> +function getBaseManifestProperties() {
> +  let types = Schemas.schemaJSON.get(BASE_SCHEMA)[0].types;
> +  let manifest = types.find(type => type.id === "WebExtensionManifest");
> +  if (!manifest) {
> +    throw new Error("Unabled to find base manifest properties");

nit: s/Unabled/Unable/

::: toolkit/components/extensions/ExtensionParent.jsm:906
(Diff revision 3)
> +  let types = Schemas.schemaJSON.get(BASE_SCHEMA)[0].types;
> +  let manifest = types.find(type => type.id === "WebExtensionManifest");
> +  if (!manifest) {
> +    throw new Error("Unabled to find base manifest properties");
> +  }
> +  return Object.keys(manifest.properties);

Please use `Object.getOwnPropertyNames(manifest.properties);`

::: toolkit/components/extensions/ExtensionParent.jsm:914
(Diff revision 3)
>  const ExtensionParent = {
>    GlobalManager,
>    HiddenExtensionPage,
>    ParentAPIManager,
>    apiManager,
> +  getBaseManifestProperties,

This is what I was thinking of...

```js
let gBaseManifestProperties = null;

const ExtensionParent = {
  // ...other members.
  get baseManifestProperties() {
    if (gBaseManifestProperties) {
      return gBaseManifestProperties;
    }

    let types = Schemas.schemaJSON.get(BASE_SCHEMA)[0].types;
    let manifest = types.find(type => type.id === "WebExtensionManifest");
    if (!manifest) {
      throw new Error("Unabled to find base manifest properties");
    }
  
    gBaseManifestProperties = Object.getOwnPropertyNames(manifest.properties);
    return gBaseManifestProperties;
  },
  // ...other members.
};
```

When you can cache the return value of a function in a reliable manner, you should do it. This is called memoization.
Attachment #8836066 - Flags: review?(mdeboer) → review-
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review115410

> nit: please rename this to `gThemesEnabled`

Done.

> nit: can you rename this test to something that explains a bit more what it's doing?

Sure.

> nit: perhaps a switch statement would look better here?

Yeah, I agree, it does look better in this case. I updated it to use a switch statement.

> nit: s/Unabled/Unable/

Fixed.

> Please use `Object.getOwnPropertyNames(manifest.properties);`

Done.

> This is what I was thinking of...
> 
> ```js
> let gBaseManifestProperties = null;
> 
> const ExtensionParent = {
>   // ...other members.
>   get baseManifestProperties() {
>     if (gBaseManifestProperties) {
>       return gBaseManifestProperties;
>     }
> 
>     let types = Schemas.schemaJSON.get(BASE_SCHEMA)[0].types;
>     let manifest = types.find(type => type.id === "WebExtensionManifest");
>     if (!manifest) {
>       throw new Error("Unabled to find base manifest properties");
>     }
>   
>     gBaseManifestProperties = Object.getOwnPropertyNames(manifest.properties);
>     return gBaseManifestProperties;
>   },
>   // ...other members.
> };
> ```
> 
> When you can cache the return value of a function in a reliable manner, you should do it. This is called memoization.

Updated.

Note thought that I'm already cacheing the base manifest properties within a lazy getter inside of Extension.jsm, but I agree with you that it would be a good idea to use memoization here.
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review116348

::: toolkit/components/extensions/Extension.jsm:425
(Diff revision 5)
> +        invalidProps.push(propName);
> +      }
> +    }
> +
> +    if (invalidProps.length) {
> +      let message = `Themes defined in the manifest may only contain static resources. ` +

I neglected to ask this before, but are we supposed to offer these messages as localizable? If not, perhaps it would be good practice to start with that now?

::: toolkit/components/extensions/ExtensionParent.jsm:885
(Diff revision 5)
> +      throw new Error("Unabled to find base manifest properties");
> +    }
> +
> +    gBaseManifestProperties = Object.getOwnPropertyNames(manifest.properties);
> +    return gBaseManifestProperties;
> +  },

I think you'll want to add an XPCShell test to test this getter. To free the global variable, you can unload ExtensionParent.jsm by using `Cu.unload()`.
Attachment #8836066 - Flags: review?(mdeboer)
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review116352
Forgot to mention: it'll be r=me with that test added! I believe you also wanted Kris to look at this patch?
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review117926

::: toolkit/components/extensions/ExtensionParent.jsm:880
(Diff revision 5)
> +    }
> +
> +    let types = Schemas.schemaJSON.get(BASE_SCHEMA)[0].types;
> +    let manifest = types.find(type => type.id === "WebExtensionManifest");
> +    if (!manifest) {
> +      throw new Error("Unabled to find base manifest properties");

s/Unabled/Unable/
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review120460

The newly added test need more work, but the code looks a-ok to me! Please think about separating your work between 'functional code/ implementation' and 'added test coverage' commits. That way reviewers can throw around more r+'s and be much happier ;-)
IOW: you're almost there! Kris, can you review the code as well, please?

::: toolkit/components/extensions/Extension.jsm:432
(Diff revision 7)
>  
>          preprocessors: {},
>        };
>  
> +      if (this.manifest.theme) {
> +        let invalidProps = validateThemeManifest(Object.keys(this.manifest));

nit: `Object.getOwnPropertyNames(this.manifest)`

::: toolkit/components/extensions/test/xpcshell/test_ext_themes_supported_properties.js:8
(Diff revision 7)
> +"use strict";
> +
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +Cu.import("resource://gre/modules/Schemas.jsm");
> +
> +let {

nit: s/let/const/

Please don't use `let` in the module scope, since their semantics are the same as `var` there and cause unnecessary overhead for the interpreter.
That's why you don't see `let` at the top of JS modules, only `var` and `const`.
`const` has similar scoping semantics, but still has value because of its immutability property.

::: toolkit/components/extensions/test/xpcshell/test_ext_themes_supported_properties.js:64
(Diff revision 7)
> +  "tabs",
> +  "topSites",
> +  "webNavigation",
> +  "webRequest",
> +  "windows",
> +];

By defining these lists here statically, you're making this test very fragile.

Please think of a way to fetch these from Schemas.jsm and ExtensionParent.jsm.

::: toolkit/components/extensions/test/xpcshell/test_ext_themes_supported_properties.js:66
(Diff revision 7)
> +  "webNavigation",
> +  "webRequest",
> +  "windows",
> +];
> +
> +function validateProperties(actual, expected) {

ITYM `checkProperties`

::: toolkit/components/extensions/test/xpcshell/test_ext_themes_supported_properties.js:67
(Diff revision 7)
> +  "webRequest",
> +  "windows",
> +];
> +
> +function validateProperties(actual, expected) {
> +  do_check_eq(actual.length, expected.length, `Should have found ${expected.length} invalid properties`);

Please use `Assert.equal`, `Assert.ok` and friends in new tests, because that's what all `do_check_*` assertion methods use under the hood anyways.

::: toolkit/components/extensions/test/xpcshell/test_ext_themes_supported_properties.js:74
(Diff revision 7)
> +    do_check_true(actual.includes(expected[i]), `The invalid properties should contain "${expected[i]}"`);
> +  }
> +}
> +
> +add_task(function* test_theme_supported_properties() {
> +  Schemas.load(BASE_SCHEMA_URL).then(() => {

```js
yield Schemas.load(BASE_SCHEMA_URL);
// ...
```
Attachment #8836066 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #20)
> toolkit/components/extensions/test/xpcshell/
> test_ext_themes_supported_properties.js:64
> (Diff revision 7)
> > +  "tabs",
> > +  "topSites",
> > +  "webNavigation",
> > +  "webRequest",
> > +  "windows",
> > +];
> 
> By defining these lists here statically, you're making this test very
> fragile.
> 
> Please think of a way to fetch these from Schemas.jsm and
> ExtensionParent.jsm.

From our meeting I thought we *wanted* to make this test fail if this list changes so that devs will know explicitly that they are changing behavior for webextension-themes? I would prefer that we keep this list duplicated.
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review120460

> By defining these lists here statically, you're making this test very fragile.
> 
> Please think of a way to fetch these from Schemas.jsm and ExtensionParent.jsm.

Jared mentioned in comment 21 on bugzilla that we talked about keeping this entire list static in our meeting last week. However, in that meeting we were only talking about the base manifest properties. I agree with you that the second list of properties, which are all APIs we support, is likely to change moving forward, so we should try not to list these statically. I'll work on this for the next revision :)
(In reply to Matthew Wein [:mattw] from comment #22) 
> Jared mentioned in comment 21 on bugzilla that we talked about keeping this
> entire list static in our meeting last week. However, in that meeting we
> were only talking about the base manifest properties.

Yes, that's what we talked about. Thanks for the clarification, Jared!
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review120958

I like it :)

::: toolkit/components/extensions/ExtensionUtils.jsm:73
(Diff revision 8)
> +    const propertiesToRemove = ["background", "content_scripts", "permissions"];
> +    return !propertiesToRemove.includes(prop);
> +  });
> +});
> +
> +// Validates a theme to ensure it only contains static resources, and returns

nit: please change this to a jsdoc comment.
Attachment #8836066 - Flags: review?(mdeboer) → review+
Comment on attachment 8845647 [details]
Bug 1338525 - Add test coverage for theme validation

https://reviewboard.mozilla.org/r/118778/#review120962

LGTM
Attachment #8845647 - Flags: review?(mdeboer) → review+
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review121106

::: toolkit/components/extensions/ExtensionUtils.jsm:73
(Diff revision 8)
> +    const propertiesToRemove = ["background", "content_scripts", "permissions"];
> +    return !propertiesToRemove.includes(prop);
> +  });
> +});
> +
> +// Validates a theme to ensure it only contains static resources, and returns

I'll update it, but while I do it I am going to be frowning at all of the other functions that don't use it (I originally had jsdoc but switched it to a comment to stay consistent with the rest of the file which seems to only use jsdoc for classes..)

I think you're right here though, because we should be trying to push towards getting jsdoc used everywhere, and therefore any new code added should use it. Hopefully we can get older functions updated soon though so we don't have this inconsistency.
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review121106

> I'll update it, but while I do it I am going to be frowning at all of the other functions that don't use it (I originally had jsdoc but switched it to a comment to stay consistent with the rest of the file which seems to only use jsdoc for classes..)
> 
> I think you're right here though, because we should be trying to push towards getting jsdoc used everywhere, and therefore any new code added should use it. Hopefully we can get older functions updated soon though so we don't have this inconsistency.

Actually, some of the other functions in the file do have jsdoc which is great! I'm looking forward to seeing this file eventually be jsdoc complete :)
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review116348

> I neglected to ask this before, but are we supposed to offer these messages as localizable? If not, perhaps it would be good practice to start with that now?

That's a great question. It looks like we don't localize any error messages, and I think it might be a good idea to do so. I'll file a follow up bug for this though, because I think it might a bigger project.
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review116348

> That's a great question. It looks like we don't localize any error messages, and I think it might be a good idea to do so. I'll file a follow up bug for this though, because I think it might a bigger project.

I filed bug 1346376 to track this. I think it would be great to have support for this by 57.
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

Mossop, would you mind looking over the JSM additions? I'd like to have a second pair of eyes look over where I decided to put the helpers I needed. Do they seem to be in reasonable locations to you? Thanks!
Attachment #8836066 - Flags: review?(kmaglione+bmo) → review?(dtownsend)
Attachment #8845647 - Flags: review?(kmaglione+bmo)
Comment on attachment 8836066 [details]
Bug 1338525 - Add schema validation for webextension themes

https://reviewboard.mozilla.org/r/111552/#review123076

Looks fine to me
Attachment #8836066 - Flags: review?(dtownsend) → review+
Attachment #8836066 - Flags: review?(kmaglione+bmo)
Attachment #8845647 - Flags: review?(kmaglione+bmo) → review?(dtownsend)
Attachment #8845647 - Flags: review?(dtownsend)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f55090db9e6
Add schema validation for webextension themes r=mikedeboer,mossop
https://hg.mozilla.org/integration/autoland/rev/1a4f971e5654
Add test coverage for theme validation r=mikedeboer
Flags: needinfo?(jaws) → needinfo?(mwein)
Attachment #8836066 - Flags: review?(kmaglione+bmo)
Attachment #8845647 - Flags: review?(kmaglione+bmo)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8caa66acbfc
Add schema validation for webextension themes r=mikedeboer,mossop
https://hg.mozilla.org/integration/autoland/rev/0232885afcb2
Add test coverage for theme validation r=mikedeboer
Flags: needinfo?(mwein)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: