1525762 - Convert built-in LWTs to static themes

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, task, P1)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1a - Add maybeInstallBuiltinAddon method. r=aswan

This allows us to install built-in themes at startup only when they're new, or
have changed, without requiring special work from callers, and in particular
without requiring loading the add-on database when no changes are required.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1b - Add databaseReady promise to replace xpi-database-loaded observer. r=aswan

All of the consumers of this observer really want it to behave like a promise.
And, for the cases where the DB may or may not already be loaded when those
callers run, getting the logic correct is difficult.

This patch replaces the observer with a promise, and also delays the
resolution of that promise until any built-in add-ons registered during
XPIProvider startup have finished installing. This latter feature is currently
unused, but will be necessary after subsequent patches for code that relies
querying the default theme immediately after provider startup.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1c - Resolve built-in add-on resource: URIs at startup, not install. r=aswan

It's possible for the application install location to vary from session to
session, particularly when the same profile is used with multiple app
versions, or with both packed and unpacked builds. Resolving resource: URIs at
install time causes problems in those instances, since it will always point to
the inital app location.

Resolving the resource URIs at runtime solves this.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1d - Don't throw from hasResource for resources that don't exist. r=aswan

When attempting to fetch a local resource which doesn't exist, the fetch
promise rejects, causing the hasResource promise to reject. Since we don't
want hasResource to reject for nonexistent resources, we need to catch this
rejection and treat it as a nonexistent resource.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1e - Don't hide add-ons in the built-in location. r=aswan

We want add-ons in the built-in location, particularly the built-in themes, to
appear in the add-on manager so that users can enable and disable them. We
don't want them to be able to uninstall them, though, so this also makes the
built-in location locked.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1f - Fix dodgy error message.. r=aswan

The filePath property is used in error messages, and is expected to be a
string. Setting it to a nsIFile object makes those error messages inscrutable.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1g - Fix issues with add-on sourceBundle and rootURI properties. r=aswan

There are all sorts of random issues with rootURI sometimes not being set for
sourceBundle add-ons, or callers expecting sourceBundle to never be null. This
patch fixes all of those issues that I came across.

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1h - Use SCOPE_APPLICATION consistently for built-in add-ons. r=aswan

Most of our tests disable SCOPE_SYSTEM add-ons, which are meant to have been
registered externally, but still rely on SCOPE_APPLICATION addons (i.e., the
default theme), which are meant to be part of the application.

We currently flag the built-in location as SCOPE_SYSTEM in some places and
SCOPE_APPLICATION in others, which leads to those add-ons not being available
to some tests that need them.

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1i - Allow overriding some localized properties for built-in add-ons. r=aswan

We still don't have a great way to bundle localizations for built-in add-ons,
since our localization tools are not compatible with the WebExtension
localization format, and there's no way to fetch locales from language packs.

We used to have a fairly complicated mechanism for this which used localized
preference values, pointing to localized string bundle URLs, based on the
add-on ID and the property we wanted to localized. That seems needlessly
complicated at this point, so this patch just allows overrides in two specific
string bundles, one for toolkit, and one for app-specific extensions.

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1j - Don't try to package omni.ja extensions as XPIs. r=build

We have some convoluted packaging logic to automatically package any directory
with a manifest.json file as an XPI. Unfortunately, that logic also applies to
extensions that are supposed to be part of omni.ja.

It would be nice to have a cleaner way to filter out any resources which we've
already flagged to be part of omni.ja, but this patch takes the simpler
approach of just including anything that's in a modules/ directory, which is
whitelisted for omni.ja elsewhere.

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1k - Update mozscreenshots to use AddonManager rather than LightweightThemeManager. r=aswan

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1l - Don't delay serving image resources until extensions are ready. r=aswan

Since we want themes to be loaded before the main browser window is loaded, we
really need their resources to be ready as soon as possible. We typically
delay serving moz-extension: requests until the extension is ready, since
extension page and CSS loads rely on the extension being fully initialized.

Image loads, though, are perfectly safe to load as early as we need, so this
patch whitelists them to bypass the delayed load logic.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 2 - Migrate built-in LWTs to static WebExtension themes. r=aswan

Comment 14

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 3a - Get rid of LWTManager/AddonManager integration. r=aswan

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 3b - Get rid of LWT update code. r=aswan

Comment 16

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 3c - Get rid of LWT preview code. r=aswan

Comment 17

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 3d - Get rid of built-in LWT code. r=aswan

Comment 18

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 3e - Get rid of LWTManager theme management code. r=aswan

Comment 19

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 3f - Get rid of LWTPersister. r=aswan

Comment 20

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 4 - Support automatic dark mode fallback for default theme again. r=aswan

Comment 21

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 5 - Fix FOUC at startup when non-default theme is used.. r=aswan

The static theme startup code is both super asynchronous and super
inefficient. It currently takes a noticeable amount of time after startup to
finish its work and apply its theme, which results in the user seeing a flash
of the default theme before their selected them is applied.

This is particularly noticeable when dark mode themes are enabled.

This patch caches the fully-processed theme data in the addonStartup cache,
and applies it immediately after extension startup begins, if it's available.

Comment 22

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 2 - Migrate built-in LWTs to static WebExtension themes. r=aswan

Comment 23

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 1c - Resolve built-in add-on resource: URIs at startup, not install. r=aswan

It's possible for the application install location to vary from session to
session, particularly when the same profile is used with multiple app
versions, or with both packed and unpacked builds. Resolving resource: URIs at
install time causes problems in those instances, since it will always point to
the inital app location.

Resolving the resource URIs at runtime solves this.

Comment 24

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 6 - Stop dispatching theme change observers using JSON strings. r=aswan

Comment 25

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 2b - Migrate selected lightweight theme when installing built-in themes. r=aswan

Comment 26

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 2c - Use compact dark as default theme for dev edition builds. r=aswan

Comment 28

[Kris Maglione [:kmag]]

Jorg, you guys are probably going to need to update your LightweightThemeManager usage and remove your lightweightThemes default preferences, too:

https://searchfox.org/comm-central/search?q=LightweightThemeManager&case=true&regexp=false&path=mail%2F

LightweightThemeManager.addBuiltInTheme is going away, and LightweightThemeManager.currentThemeWithPersistedData is more or less being replaced with LightweightThemeManager.currentThemeWithFallback for the time being.

Comment 29

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up. I'll forward this to the add-on and theme guys, fortunately we have more workers now, but I'm happy to be the point of contact.

Comment 30

[Richard Marti (:Paenglab)]

Thank you. Kris, could you give us a list of which patches we need to port? Probably every which has changes in browser.

Comment 32

[Kris Maglione [:kmag]]

Attachment: Bug 1525762: Part 7 - Remove defunct LightweightThemeOptimizer. r=aswan

It only works with "persisted" theme images, which no longer exist.

Comment 33

[Tim Nguyen :ntim]

I notice that you're removing LightweightThemePersister.

Does that not break Android theme support ? On Android, webextension themes didn't work before bug 1429488 because the images were moz-extension:// URIs, which the Android java code had trouble using, so the persister is used to pass non-moz-extension:// URIs.

Comment 35

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6d3341f350209bc140849f073867608b55c51a
Bug 1525762: Part 1a - Add maybeInstallBuiltinAddon method. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/40936b75fd328853be677891f47dcff4ba85b2c0
Bug 1525762: Part 1b - Add databaseReady promise to replace xpi-database-loaded observer. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/935a2cd834c2a489a585dec44cbc90964471501c
Bug 1525762: Part 1c - Resolve built-in add-on resource: URIs at startup, not install. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/f595c6ccb68af17285f3aefe96e9124a2b3cc81f
Bug 1525762: Part 1d - Don't throw from hasResource for resources that don't exist. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc45935ed2c2f3ae4627584f5c7318cb52271875
Bug 1525762: Part 1e - Don't hide add-ons in the built-in location. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e9739ac28b03a09001c60674dd6699fa9741af
Bug 1525762: Part 1f - Fix dodgy error message. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e4177fca6f63b1703687ad809048904e2415c2
Bug 1525762: Part 1g - Fix issues with add-on sourceBundle and rootURI properties. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e0f5aa1c6df624ed2ad10d09fc5bb9aadee493
Bug 1525762: Part 1h - Use SCOPE_APPLICATION consistently for built-in add-ons. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7df3ee8aaa9367dfc9715edd95586acfb2627a
Bug 1525762: Part 1i - Allow overriding some localized properties for built-in add-ons. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/e289c1b047d7e2c02845decf7618f31bf145dea6
Bug 1525762: Part 1k - Update mozscreenshots to use AddonManager rather than LightweightThemeManager. r=MattN

https://hg.mozilla.org/integration/mozilla-inbound/rev/f31ce378439a073fb7a54c44ffe5dc68e1c03b77
Bug 1525762: Part 1l - Don't delay serving image resources until extensions are ready. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/b62c3bde4bc3f7cba452ad57f8d728b175c15a62
Bug 1525762: Part 2a - Migrate built-in LWTs to static WebExtension themes. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/83612982ab33e156ec51052fe86781d7e86e50ad
Bug 1525762: Part 2b - Migrate selected lightweight theme when installing built-in themes. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbbb63ea73d877b4b609ccebf6d6420b27ad14c
Bug 1525762: Part 2c - Use compact dark as default theme for dev edition builds. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c10bcdc3d96942c9413984e8e25ef4c29de6e31
Bug 1525762: Part 3a - Get rid of LWTManager/AddonManager integration. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb90dbf5abc88a452434245a0a1b2fae0ed835d6
Bug 1525762: Part 3b - Get rid of LWT update code. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b72a80ec78c1cf423c1fc9314df0bed0edd844a
Bug 1525762: Part 3c - Get rid of LWT preview code. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad35a4e471750b50d76bc12c9e1fad00bf904d4
Bug 1525762: Part 3d - Get rid of built-in LWT code. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7481329e0b7986283fbf9d443e19d575a841c9c
Bug 1525762: Part 3e - Get rid of LWTManager theme management code. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f524da2a6163d4c7cd10eae30e942fe59c5e07
Bug 1525762: Part 3f - Get rid of LWTPersister. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/238bd73cdf0b759712c6b5ae8a73d77a930a0663
Bug 1525762: Part 4 - Support automatic dark mode fallback for default theme again. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b30a2c10d998efaf789db9cf66c14b5a965e13d
Bug 1525762: Part 5 - Fix FOUC at startup when non-default theme is used. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d58601921993a4ae6bf87849eb295b6ffe389ee
Bug 1525762: Part 6 - Stop dispatching theme change observers using JSON strings. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1529f7fd786c0aa5340c4ccd5738dc69274935
Bug 1525762: Part 7 - Remove defunct LightweightThemeOptimizer. r=aswan

Comment 36

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/824a86797c8ecbb1a6395ac9fd76ad6abe724766
Bug 1525762: Part 1j - Work around XPI omni.jar packaging issues. r=bustage CLOSED TREE

Comment 37

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/cf6d3341f350
https://hg.mozilla.org/mozilla-central/rev/40936b75fd32
https://hg.mozilla.org/mozilla-central/rev/935a2cd834c2
https://hg.mozilla.org/mozilla-central/rev/f595c6ccb68a
https://hg.mozilla.org/mozilla-central/rev/cc45935ed2c2
https://hg.mozilla.org/mozilla-central/rev/e8e9739ac28b
https://hg.mozilla.org/mozilla-central/rev/d7e4177fca6f
https://hg.mozilla.org/mozilla-central/rev/f9e0f5aa1c6d
https://hg.mozilla.org/mozilla-central/rev/7c7df3ee8aaa
https://hg.mozilla.org/mozilla-central/rev/e289c1b047d7
https://hg.mozilla.org/mozilla-central/rev/f31ce378439a
https://hg.mozilla.org/mozilla-central/rev/b62c3bde4bc3
https://hg.mozilla.org/mozilla-central/rev/83612982ab33
https://hg.mozilla.org/mozilla-central/rev/5cbbb63ea73d
https://hg.mozilla.org/mozilla-central/rev/2c10bcdc3d96
https://hg.mozilla.org/mozilla-central/rev/eb90dbf5abc8
https://hg.mozilla.org/mozilla-central/rev/9b72a80ec78c
https://hg.mozilla.org/mozilla-central/rev/9ad35a4e4717
https://hg.mozilla.org/mozilla-central/rev/b7481329e0b7
https://hg.mozilla.org/mozilla-central/rev/b9f524da2a61
https://hg.mozilla.org/mozilla-central/rev/238bd73cdf0b
https://hg.mozilla.org/mozilla-central/rev/1b30a2c10d99
https://hg.mozilla.org/mozilla-central/rev/5d5860192199
https://hg.mozilla.org/mozilla-central/rev/ba1529f7fd78
https://hg.mozilla.org/mozilla-central/rev/824a86797c8e