Bugzilla

(In reply to Punam Dahiya [:pdahiya] from comment #10)
> Thanks Stuart, NI Luca to help answer below questions, please feel free to redirect if someone else can help answer
> 
> a) Whats the scope of pre-installing a colorful theme https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/ in firefox assuming we don't want to surface this new theme in about:addons and customize -> themes
> 
> I see below places where we should be updating
> https://searchfox.org/mozilla-central/source/browser/components/BrowserGlue.jsm#1275
> https://searchfox.org/mozilla-central/source/browser/themes/addons/light
> https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.js#87
> https://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm#31
> 
> Are there other touchpoints? Will be great to get high level guidance on scope here

Follows some details about the parts that would need changes and what the purpose of that change is.
Let me know if this helps and if you have more doubts or questions (or issues with this details that are blocking you).

### BrowserGlue.jsm

Based on my current understanding (derived by reading the other comments in this issue) I think that in BrowserGlue.jsm this theme should be handled a bit differently, if you call AddonManager.maybeInstallBuiltinAddon for this theme unconditionally, the theme will be listed in the available ones even if it hasn't been selected during the about:welcome onboarding wizard.

I personally think it would be totally fine, if we are ok to have that theme listed by default as all other builtin themes, but if we are not, then the change in BrowserGlue.jsm should only be called if the theme is already installed, with the purpose to update it to a more recent version if necessary when the browser is updated.  
 
### browser/themes/addons

In this directory, a new sub-directory should be created for the new builtin theme, and inside that directory the https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/ xpi should be unpacked and after that:

- a new entry should be added to the DIRS variable in the browser/themes/addons/moz.build file
- a new moz.build file should be added in the new theme sub-directory, and based on the current content of the xpi it should look like more or less like:

```
EXTRA_JS_MODULES.themes['theme-dirname'] += [
    'icon.svg',
    'manifest.json',
]

EXTRA_JS_MODULES.themes['theme-dirname']["images"] += [
    'images/*.svg',
```

- to the theme manifest.json file, we should also add (as done for the other builtin themes):
  - an explicit extension id using the `applications.gecko.id` manifest property (something like `firefox-themename@mozilla.org`)
  - an icon (by adding something like `"icons": { "32": "icon.svg" }`), which is what will be shown in the customize mode theme selector
    (otherwise it would show a default theme icon)
  - a description property

I see that the theme name is currently "Firefox Radiance", that name will be visible in the customize mode theme selector and in about:addons, we should change it if that isn't the name that should be visible there.

## toolkit/mozapps/extensions/content/aboutaddons.js

The change in aboutaddons.js is needed to associate a screenshot to the builtin theme.
Besides that, there shouldn't be any additional changes needed on the about:addons page internals.

## browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm

I'm not sure about this part, I guess that listing a builtin theme there may be required to take some screenshot automatically, but that is just a guess, we should double-check that with someone with more direct knowledge about what is the purpose of this jsm.

## browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm

In this jsm module I think that we may need to just special case this particular theme and before trying to enable it we should be sure that it is installed.

`AddonManager.maybeInstallBuiltinTheme` takes care of also checking if the theme is already installed and only need to be updated, but unfortunately it doesn't return the resulting addon wrapper, not a promise to wait that the addon has been completely installed and so calling `AddonManager.getAddonByID` right after calling `AddonManager.maybeInstallBuiltinAddon` is not going to return an addon wrapper as you would expect.

As an alternative approach, AboutWelcomeParent.jsm could check if the addon is already installed using `AddonManager.getAddonByID`  and if it is not it could call `AddonManager.installBuiltinAddon` which return a promise that resolves to the addon wrapper (on which we can call the `enable` method and have the theme enabled as for the other builtin themes).

Then I guess that the `browser.aboutwelcome.overrideContent` pref would also need to be updated to list this additional theme.

> This theme should surface alongside Default, Light and Dark in 'Choose a look' screen during onboarding and enabled if user selects
> https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm#197
> 
> b) Can we retrieve files needed for new theme from AMO side?

If you want or need it to be a builtin, I don't think so, at least is not something that I think we have ever done before and so it is not unlikely that to achieve it we may have to evaluate what kind of changes should be done to the related internals.

If should be possible to install the theme from a AMO url, e.g. I recall that some times ago activity-stream used to support installing extensions as part of the onboarding wizard (I'm not sure if it is still something that is still supported by activity-stream, but from AboutWelcomeParent it would be definitely possible to install an extension from a given AMO url).

By default installing the theme from an AMO url would show a confirmation doorhanger (a doorhanger with a Add and Cancel buttons, no permissions to accept given that it would be a pure theme add-on without any js code in it), it may be possible to workaround that, but it would likely be an hack without apply changes to make it possible without any crude workaround.

(In reply to Punam Dahiya [:pdahiya] from comment #10)
> Thanks Stuart, NI Luca to help answer below questions, please feel free to redirect if someone else can help answer
> 
> a) Whats the scope of pre-installing a colorful theme https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/ in firefox assuming we don't want to surface this new theme in about:addons and customize -> themes
> 
> I see below places where we should be updating
> https://searchfox.org/mozilla-central/source/browser/components/BrowserGlue.jsm#1275
> https://searchfox.org/mozilla-central/source/browser/themes/addons/light
> https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.js#87
> https://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm#31
> 
> Are there other touchpoints? Will be great to get high level guidance on scope here

Follows some details about the parts that would need changes and what the purpose of that change is.
Let me know if this helps and if you have more doubts or questions (or issues with this details that are blocking you).

### BrowserGlue.jsm

Based on my current understanding (derived by reading the other comments in this issue) I think that in BrowserGlue.jsm this theme should be handled a bit differently, if you call AddonManager.maybeInstallBuiltinAddon for this theme unconditionally, the theme will be listed in the available ones even if it hasn't been selected during the about:welcome onboarding wizard.

I personally think it would be totally fine, if we are ok to have that theme listed by default as all other builtin themes, but if we are not, then the change in BrowserGlue.jsm should only be called if the theme is already installed, with the purpose to update it to a more recent version if necessary when the browser is updated.  
 
### browser/themes/addons

In this directory, a new sub-directory should be created for the new builtin theme, and inside that directory the https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/ xpi should be unpacked and after that:

- a new entry should be added to the DIRS variable in the browser/themes/addons/moz.build file
- a new moz.build file should be added in the new theme sub-directory, and based on the current content of the xpi it should look like more or less like:

```
EXTRA_JS_MODULES.themes['theme-dirname'] += [
    'icon.svg',
    'manifest.json',
]

EXTRA_JS_MODULES.themes['theme-dirname']["images"] += [
    'images/*.svg',
]
```

- to the theme manifest.json file, we should also add (as done for the other builtin themes):
  - an explicit extension id using the `applications.gecko.id` manifest property (something like `firefox-themename@mozilla.org`)
  - an icon (by adding something like `"icons": { "32": "icon.svg" }`), which is what will be shown in the customize mode theme selector
    (otherwise it would show a default theme icon)
  - a description property

I see that the theme name is currently "Firefox Radiance", that name will be visible in the customize mode theme selector and in about:addons, we should change it if that isn't the name that should be visible there.

## toolkit/mozapps/extensions/content/aboutaddons.js

The change in aboutaddons.js is needed to associate a screenshot to the builtin theme.
Besides that, there shouldn't be any additional changes needed on the about:addons page internals.

## browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm

I'm not sure about this part, I guess that listing a builtin theme there may be required to take some screenshot automatically, but that is just a guess, we should double-check that with someone with more direct knowledge about what is the purpose of this jsm.

## browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm

In this jsm module I think that we may need to just special case this particular theme and before trying to enable it we should be sure that it is installed.

`AddonManager.maybeInstallBuiltinTheme` takes care of also checking if the theme is already installed and only need to be updated, but unfortunately it doesn't return the resulting addon wrapper, not a promise to wait that the addon has been completely installed and so calling `AddonManager.getAddonByID` right after calling `AddonManager.maybeInstallBuiltinAddon` is not going to return an addon wrapper as you would expect.

As an alternative approach, AboutWelcomeParent.jsm could check if the addon is already installed using `AddonManager.getAddonByID`  and if it is not it could call `AddonManager.installBuiltinAddon` which return a promise that resolves to the addon wrapper (on which we can call the `enable` method and have the theme enabled as for the other builtin themes).

Then I guess that the `browser.aboutwelcome.overrideContent` pref would also need to be updated to list this additional theme.

> This theme should surface alongside Default, Light and Dark in 'Choose a look' screen during onboarding and enabled if user selects
> https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm#197
> 
> b) Can we retrieve files needed for new theme from AMO side?

If you want or need it to be a builtin, I don't think so, at least is not something that I think we have ever done before and so it is not unlikely that to achieve it we may have to evaluate what kind of changes should be done to the related internals.

It should be possible to install the theme from a AMO url, e.g. I recall that some times ago activity-stream used to support installing extensions as part of the onboarding wizard (I'm not sure if it is still something that is still supported by activity-stream, but from AboutWelcomeParent it would be definitely possible to install an extension from a given AMO url).

By default installing the theme from an AMO url would show a confirmation doorhanger (a doorhanger with a Add and Cancel buttons, no permissions to accept given that it would be a pure theme add-on without any js code in it), it may be possible to workaround that, but it would likely be an hack without apply changes to make it possible without any crude workaround.