Implement theme experiments

RESOLVED FIXED in Firefox 63

Status

enhancement
P5
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: mikedeboer, Assigned: ntim)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: triaged [ntim-intern-project])

User Story

As a theme author I’d like to be able to propose new properties that will allow all theme authors to style different parts of the browser using the following API:

```json
{
  "manifest_version": 2,
  "name": "Moar Custom Icons",
  "description": "Example of a theme experiment",
  "version": "1.0",
  "applications": {
    "gecko": {
      "id": "moar-custom-icons@example.com",
      "strict_min_version": "57.0",
      "strict_max_version": "65.*"
    }
  },
  "icons": {
    "48": "icon.png",
    "96": "icon@2x.png"
  },
  "theme": {
    "colors": {
      "popup_affordance": [128,128,128]
    },
    "images": {
      "headerURL": "image.jpeg",
      "theme_toolbar": "toolbar.jpg"
    }
  },
  "theme_experiment": {
    "stylesheet": "style.css", // optional
    "colors": {
      "popup_affordance": "arrowpanel-dimmed"
    },
    "images": {
      "theme_toolbar": "toolbar-bgimage"
    },
    "properties": {
      "toolbar_image_alignment": "toolbar-bgalignment"
    }
  }
}
```

Attachments

(3 attachments)

No description provided.
Blocks: 1347210
Priority: -- → P5
Talking with ntim about this and we think that the experimental CSS section would have better use if it allowed for specifying a CSS file that would get included instead of converting CSS to a JSON-friendly format.

The downside here is that it would make it harder for us to figure out what components theme authors are overriding, but we may also be able to learn these by analyzing popular experimental themes anyways, it would just be a little more work. The upside is that this would allow theme authors a better developer experience, and the previously suggested "selector" property-name was not required to coordinate with an existing schema so it's value was questionable.
User Story: (updated)
Assignee: nobody → ntim.bugs
I'm not sure what the intentions are for this feature but we've steadfastly avoided giving access to chrome css to extensions and I don't think we should start now.  There are certainly interesting legitimate uses but a malicious extension could easily abuse it as well.  For instance they could change the site identity or permissions indicators on particular sites.

If we add this section, AMO should *not* sign themes that include it.
This looks like the theme equivalent of WebExtensions Experiments, so I agree that they don't belong on AMO (nor they should be easy to distribute and install broadly).
The patch works, I'm planning to add tests tomorrow.
Before getting into code review, I'd like to discuss the high level idea first.
If this just exists to support the built-in compact themes, then I think its a bit over-engineered, lets just add the features those themes need but limit access to them to built-in themes if necessary...
(In reply to Andrew Swan [:aswan] from comment #6)
> Before getting into code review, I'd like to discuss the high level idea
> first.
> If this just exists to support the built-in compact themes, then I think its
> a bit over-engineered, lets just add the features those themes need but
> limit access to them to built-in themes if necessary...

This was part of the original engineering plan: https://docs.google.com/document/d/1ueD6V7aLLTuc1GAOxxQYcwl2HR-k62HHu3q8knTJ4FU/pub in the spirit of WebExtensions Experiments. It wasn't originally created to support the built-in themes, I just thought later-on this feature would be a nice way to support them.
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

Hi Tim, cool that you're picking this up!

I talked about this API with Jared as well and I noted that I'm not sold on the idea of injecting additional CSS into the browser. It just grants too many superpowers to extension authors - even though it only works on Nightly.

I think we want to make a more specific solution driven that may also solve the missing features problem you have to implement the light and dark themes using the Theming API.

I'd like to propose the following:

 * Don't use the Experimental API for the things we need for the Dark and Light themes to be fully supported, but instead add & implement properties that Mozilla-signed themes may use. Indeed, proprietary stuff like '-moz-mac-vibrant-xxx' is also something we can enable that way.
 ** The idea here is to extend the `load()` method in ext-theme.js with a call to a new `loadPropertiesWhenSigned()` which will check if the extension is in fact signed and validated and adds the whitelisted properties when defined to the theme data.
 * Implement the original experimental API idea using property maps, so that it's easier to parse and prune CSS statements *before* they are evaluated in the chrome context.

What do you think? We can also talk the plan through on IRC & Vidyo, of course!
Attachment #8940885 - Flags: review?(jaws)
Attachment #8940885 - Flags: review?(aswan)
mass move of existing themes bugs to new WebExtensions: Themes component
Component: WebExtensions: Frontend → WebExtensions: Themes
Unassigning to reflect real status.
Assignee: ntim.bugs → nobody
Assignee: nobody → ntim.bugs
Updated user story with what was discussed in SF with :mikedeboer
User Story: (updated)
User Story: (updated)
Product: Toolkit → WebExtensions
Whiteboard: triaged → triaged [ntim-intern-project]
We should make sure that -moz-binding is not allowed to be used by the CSS with this API.
Depends on: 1477708
Attachment #8940885 - Flags: review?(jaws)
Attachment #8940885 - Flags: review?(aswan)
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review267274


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:68
(Diff revision 2)
> +    } else {
> -    this.load();
> +      this.load();
> -  }
> +    }
> +  }
> +
> +  /**

Error: Missing JSDoc @returns for function. [eslint: valid-jsdoc]

::: toolkit/components/extensions/parent/ext-theme.js:73
(Diff revision 2)
> +  /**
> +   * Check if the theme can run the theme experiment
> +   * Experiments are only allowed in system add-ons and on the dev channels.
> +   */
> +  async themeCanRunExperiment() {
> +    let addon = await AddonManager.getAddonByID(this.extension.id);

Error: 'AddonManager' is not defined. [eslint: no-undef]

::: toolkit/modules/LightweightThemeConsumer.jsm:236
(Diff revision 2)
> +    }
> +    if (active && experiment) {
> +      this._lastExperimentData = {};
> +      if (experiment.stylesheet) {
> +        let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;
> +        let stylesheet = doc.createProcessingInstruction("xml-stylesheet",

Error: 'stylesheet' is assigned a value but never used. [eslint: no-unused-vars]

::: toolkit/modules/LightweightThemeConsumer.jsm:236
(Diff revision 2)
> +    }
> +    if (active && experiment) {
> +      this._lastExperimentData = {};
> +      if (experiment.stylesheet) {
> +        let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;
> +        let stylesheet = doc.createProcessingInstruction("xml-stylesheet",

Error: 'doc' is not defined. [eslint: no-undef]

::: toolkit/modules/LightweightThemeConsumer.jsm:238
(Diff revision 2)
> +      this._lastExperimentData = {};
> +      if (experiment.stylesheet) {
> +        let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;
> +        let stylesheet = doc.createProcessingInstruction("xml-stylesheet",
> +          stylesheetAttr);
> +        this._doc.insertBefore(styleSheet, root);

Error: 'styleSheet' is not defined. [eslint: no-undef]
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review267276


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:68
(Diff revision 3)
> +    } else {
> -    this.load();
> +      this.load();
> -  }
> +    }
> +  }
> +
> +  /**

Error: Missing JSDoc @returns for function. [eslint: valid-jsdoc]

::: toolkit/components/extensions/parent/ext-theme.js:73
(Diff revision 3)
> +  /**
> +   * Check if the theme can run the theme experiment
> +   * Experiments are only allowed in system add-ons and on the dev channels.
> +   */
> +  async themeCanRunExperiment() {
> +    let addon = await AddonManager.getAddonByID(this.extension.id);

Error: 'AddonManager' is not defined. [eslint: no-undef]

::: toolkit/modules/LightweightThemeConsumer.jsm:236
(Diff revision 3)
> +    }
> +    if (active && experiment) {
> +      this._lastExperimentData = {};
> +      if (experiment.stylesheet) {
> +        let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;
> +        let stylesheet = doc.createProcessingInstruction("xml-stylesheet",

Error: 'doc' is not defined. [eslint: no-undef]
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review267290


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:68
(Diff revision 4)
> +    } else {
> -    this.load();
> +      this.load();
> -  }
> +    }
> +  }
> +
> +  /**

Error: Missing JSDoc @returns for function. [eslint: valid-jsdoc]

::: toolkit/components/extensions/parent/ext-theme.js:73
(Diff revision 4)
> +  /**
> +   * Check if the theme can run the theme experiment
> +   * Experiments are only allowed in system add-ons and on the dev channels.
> +   */
> +  async themeCanRunExperiment() {
> +    let addon = await AddonManager.getAddonByID(this.extension.id);

Error: 'AddonManager' is not defined. [eslint: no-undef]

::: toolkit/modules/LightweightThemeConsumer.jsm:236
(Diff revision 4)
> +    }
> +    if (active && experiment) {
> +      this._lastExperimentData = {};
> +      if (experiment.stylesheet) {
> +        let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;
> +        let stylesheet = doc.createProcessingInstruction("xml-stylesheet",

Error: 'doc' is not defined. [eslint: no-undef]
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review267294


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:68
(Diff revision 5)
> +    } else {
> -    this.load();
> +      this.load();
> -  }
> +    }
> +  }
> +
> +  /**

Error: Missing JSDoc return type. [eslint: valid-jsdoc]
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review267320


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:71
(Diff revision 6)
> +    } else {
> -    this.load();
> +      this.load();
> -  }
> +    }
> +  }
> +
> +  /**

Error: Missing JSDoc return type. [eslint: valid-jsdoc]
Status: NEW → ASSIGNED
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review267892

Looks good. Will need tests before this can land though (which you already know :) )

::: toolkit/components/extensions/parent/ext-theme.js:62
(Diff revision 6)
> +          if (experiment.stylesheet) {
> +            baseURI.resolve(experiment.stylesheet);
> +          }
> +          this.experiment = experiment;
> +        } else {
> +          logger.warn("This extension is not allowed to run theme experiments");

We should just throw an exception here and not load the extension.

::: toolkit/components/extensions/parent/ext-theme.js:64
(Diff revision 6)
> +          }
> +          this.experiment = experiment;
> +        } else {
> +          logger.warn("This extension is not allowed to run theme experiments");
> +        }
> +        this.load();

You can remove `this.load()` and then pull the `this.load()` outside of the `else` block below so it will always run.

::: toolkit/components/extensions/parent/ext-theme.js:373
(Diff revision 6)
> +    let {theme, theme_experiment} = manifest;
> +
> +    if (!theme && theme_experiment) {
> +      this.experiment = theme_experiment;
> +    } else {
> +      this.experiment = null;

should this be clearing theme_experiment too?

::: toolkit/modules/LightweightThemeConsumer.jsm:219
(Diff revision 6)
>        root.removeAttribute("lwthemefooter");
>  
> -    let contentThemeData = _getContentProperties(this._doc, active, aData);
> +    let contentThemeData = _getContentProperties(this._doc, active, theme);
>      Services.ppmm.sharedData.set(`theme/${this._winId}`, contentThemeData);
> +  },
> +  _setExperiment(active, experiment, properties) {

nit, put a blank line above this

::: toolkit/modules/LightweightThemeConsumer.jsm:235
(Diff revision 6)
> +      }
> +    }
> +    if (active && experiment) {
> +      this._lastExperimentData = {};
> +      if (experiment.stylesheet) {
> +        let stylesheetAttr = `href="${experiment.stylesheet}" type="text/css"`;

nb, this is okay since the schema restricts stylesheet to an ExtensionURL
Attachment #8940885 - Flags: review?(jaws) → review+
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review267900

::: toolkit/components/extensions/parent/ext-theme.js:78
(Diff revision 6)
> +   * Experiments are only allowed in system add-ons and on the dev channels.
> +   * @returns boolean true if experiments can be ran, false if not
> +   */
> +  async themeCanRunExperiment() {
> +    let addon = await AddonManager.getAddonByID(this.extension.id);
> +    return !AppConstants.RELEASE_OR_BETA || addon.isSystem;

I haven't read the rest of the patch but if this allows themes to insert arbitrary css into chrome documents, this is not strong enough, there should be a separate pref defaulting to off that needs to be flipped to enable this.
Ni? about using !RELEASE_OR_BETA or MOZ_ALLOW_LEGACY_EXTENSIONS.
Flags: needinfo?(jaws)
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review268986


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python/etc)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:55
(Diff revision 7)
> +      this.lwtStyles.experimental = {
> +        colors: {},
> +        images: {},
> +        properties: {},
> +      };
> +      const {baseURI, logger} = this.extension;

Error: 'logger' is assigned a value but never used. Allowed unused vars must match /^console$/. [eslint: no-unused-vars]
Comment on attachment 8940885 [details]
Bug 1347207 - Implement theme_experiment manifest field.

https://reviewboard.mozilla.org/r/211142/#review268988


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python/etc)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:49
(Diff revision 8)
>      this.lwtStyles = {
>        icons: {},
>      };
>  
> +    if (experiment) {
> +      const canRunExperiment = !AppConstants.RELEASE_OR_BETA && 

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/components/extensions/parent/ext-theme.js:57
(Diff revision 8)
> +        this.lwtStyles.experimental = {
> +          colors: {},
> +          images: {},
> +          properties: {},
> +        };
> +        const {baseURI, logger} = this.extension;

Error: 'logger' is assigned a value but never used. Allowed unused vars must match /^console$/. [eslint: no-unused-vars]
(In reply to Tim Nguyen :ntim from comment #28)
> Ni? about using !RELEASE_OR_BETA or MOZ_ALLOW_LEGACY_EXTENSIONS.

Using AppConstants.MOZ_ALLOW_LEGACY_EXTENSIONS is fine with me though you will to also check the `extensions.legacy.enabled` pref to satisfy aswan's request that an extra pref would be needed.
Flags: needinfo?(jaws)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/91a30d78c074
Implement theme_experiment manifest field. r=jaws
ESlint failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193006227&repo=autoland&lineNumber=260

[task 2018-08-09T11:49:03.547Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/parent/ext-theme.js:194:5 | Block must not be padded by blank lines. (padded-blocks)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:17:10 | Missing trailing comma. (comma-dangle)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:28:10 | Missing trailing comma. (comma-dangle)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:29:8 | Missing trailing comma. (comma-dangle)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:76:10 | Missing trailing comma. (comma-dangle)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:77:8 | Missing trailing comma. (comma-dangle)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:89:10 | Missing trailing comma. (comma-dangle)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:103:6 | Trailing spaces not allowed. (no-trailing-spaces)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:104:1 | Trailing spaces not allowed. (no-trailing-spaces)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:164:15 | Trailing spaces not allowed. (no-trailing-spaces)
[task 2018-08-09T11:54:14.595Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:181:6 | Missing trailing comma. (comma-dangle)
[taskcluster 2018-08-09 11:54:15.139Z] === Task Finished ===
[taskcluster 2018-08-09 11:54:15.139Z] Unsuccessful task run with exit code: 1 completed in 568.497 seconds
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/421c12a2837b
Implement theme_experiment manifest field. r=jaws
Duplicate of this bug: 1347210
No longer blocks: 1347210
Summary: Implement 'experimental' theme manifest section → Implement theme experiments
Backed out for failing browser chrome at toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=421c12a2837ba777d295a891ff4de9637792011e

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193029357&repo=autoland&lineNumber=15419

Backout: https://hg.mozilla.org/integration/autoland/rev/249570f5e1ba0dcddb6b0b562a28a03a38a715ef
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/20d0116ece9a
Implement theme_experiment manifest field. r=jaws
Flags: needinfo?(ntim.bugs)
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65a6a118cb1a
acked out changeset 20d0116ece9a for failing xpcshell at  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_manifest_themes.js on a CLOSED TREE
Backed out for failing xpcshell at xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_manifest_themes.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=20d0116ece9a1b88eceaf1876aabd2a162ca5496

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193064474&repo=autoland&lineNumber=7496

Backout: https://hg.mozilla.org/integration/autoland/rev/65a6a118cb1a7bbe4c2af3ee41e89eb6704f48bc
Flags: needinfo?(ntim.bugs)
Comment on attachment 8998936 [details]
Bug 1347207 - Add and fix tests for theme loading warnings.

https://reviewboard.mozilla.org/r/261662/#review269170
Attachment #8998936 - Flags: review?(mixedpuppy) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/98010ec67528
Implement theme_experiment manifest field. r=jaws
https://hg.mozilla.org/integration/autoland/rev/56a97a3eee82
Add and fix tests for theme loading warnings. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/20d0116ece9a
https://hg.mozilla.org/mozilla-central/rev/65a6a118cb1a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(ntim.bugs)
Keywords: dev-doc-needed
Depends on: 1482870
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs) → qe-verify-
You need to log in before you can comment on or make changes to this bug.