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)
WebExtensions
Frontend
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
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)
No description provided.
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mwein
Assignee | ||
Updated•7 years ago
|
Attachment #8836066 -
Flags: review?(mdeboer)
Comment 2•7 years ago
|
||
mozreview-review |
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 3•7 years ago
|
||
mozreview-review |
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 4•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8836066 -
Flags: feedback?(kmaglione+bmo)
Assignee | ||
Updated•7 years ago
|
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 6•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
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 10•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
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 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8836066 [details] Bug 1338525 - Add schema validation for webextension themes https://reviewboard.mozilla.org/r/111552/#review116352
Comment 16•7 years ago
|
||
Forgot to mention: it'll be r=me with that test added! I believe you also wanted Kris to look at this patch?
Comment 17•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
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-
Comment 21•7 years ago
|
||
(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.
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
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 :)
Comment 23•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
mozreview-review |
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 27•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-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.
Assignee | ||
Comment 29•7 years ago
|
||
mozreview-review-reply |
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 :)
Assignee | ||
Comment 30•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8845647 -
Flags: review?(kmaglione+bmo)
Comment 35•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8836066 -
Flags: review?(kmaglione+bmo)
Attachment #8845647 -
Flags: review?(kmaglione+bmo) → review?(dtownsend)
Updated•7 years ago
|
Attachment #8845647 -
Flags: review?(dtownsend)
Comment 38•7 years ago
|
||
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
Comment 39•7 years ago
|
||
Backed out for eslint failure in toolkit/components/extensions/ExtensionUtils.jsm: https://hg.mozilla.org/integration/autoland/rev/f89deb8fd3cefc493a123f6e47914bf37cb075d4 https://hg.mozilla.org/integration/autoland/rev/8f4dfccfd6fec3133e61b54fda977fbaec77951d Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1a4f971e56545d20b0b5825365029e64b37196d1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=84406941&repo=autoland >TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/ExtensionUtils.jsm:79:1 | JSDoc syntax error. (valid-jsdoc)
Flags: needinfo?(jaws)
Updated•7 years ago
|
Flags: needinfo?(jaws) → needinfo?(mwein)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9826232151e97d21f6a26130a3dae9851344392d
Updated•7 years ago
|
Attachment #8836066 -
Flags: review?(kmaglione+bmo)
Attachment #8845647 -
Flags: review?(kmaglione+bmo)
Comment 43•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mwein)
Comment 44•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a8caa66acbfc https://hg.mozilla.org/mozilla-central/rev/0232885afcb2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•