Convert built-in LWTs to static themes

RESOLVED FIXED in Firefox 68

Status

()

task
P1
normal
RESOLVED FIXED
5 months ago
Last month

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug, Regressed 4 bugs)

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 wontfix, firefox68 fixed)

Details

Attachments

(25 attachments, 2 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

5 months ago
No description provided.

Updated

5 months ago
See Also: → 1370919
Priority: -- → P1
Assignee

Comment 1

3 months ago

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.

Assignee

Comment 2

3 months ago

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.

Assignee

Comment 3

3 months ago

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.

Assignee

Comment 4

3 months ago

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.

Assignee

Comment 5

3 months ago

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.

Assignee

Comment 6

3 months ago

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.

Assignee

Comment 7

3 months ago

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.

Assignee

Comment 8

3 months ago

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.

Assignee

Comment 9

3 months ago

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.

Assignee

Comment 10

3 months ago

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.

Assignee

Comment 12

3 months ago

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.

Assignee

Comment 21

3 months ago

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.

Assignee

Comment 23

3 months ago

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.

Attachment #9053386 - Attachment is obsolete: true
Attachment #9053385 - Attachment is obsolete: true
Depends on: 1539355
Assignee

Updated

3 months ago
Duplicate of this bug: 1538318
Assignee

Comment 28

3 months ago

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.

Flags: needinfo?(jorgk)

Comment 29

3 months ago

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.

Flags: needinfo?(richard.marti)
Flags: needinfo?(jorgk)
Flags: needinfo?(geoff)

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

Flags: needinfo?(richard.marti)
Assignee

Updated

3 months ago
Duplicate of this bug: 1539785
Assignee

Comment 32

3 months ago

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

Comment 33

3 months ago

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.

Flags: needinfo?(kmaglione+bmo)

Updated

3 months ago
Blocks: 1482870

Updated

3 months ago
Blocks: 1540387

Updated

3 months ago
Duplicate of this bug: 1386004
Assignee

Updated

3 months ago
Blocks: 1540435
Assignee

Comment 35

3 months ago
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
Assignee

Comment 36

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

Updated

3 months ago
Flags: needinfo?(geoff)

Updated

3 months ago
Depends on: 1540515

Comment 37

3 months ago
bugherder
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Depends on: 1540557
Assignee

Updated

3 months ago
Blocks: 1540856

Updated

3 months ago
Blocks: 1535568

Updated

3 months ago
Blocks: 1535565

Updated

3 months ago
No longer depends on: 1540515, 1540548, 1540557
Regressions: 1540557, 1540548, 1540515
Assignee

Updated

3 months ago
Flags: needinfo?(kmaglione+bmo)

Updated

3 months ago
Duplicate of this bug: 1524660

Updated

3 months ago
Regressions: 1542023

Updated

3 months ago
Regressions: 1542026

Updated

3 months ago
Depends on: 1542044

Updated

3 months ago
No longer depends on: 1542044
Regressions: 1542044

Updated

3 months ago
Regressions: 1540984
Assignee

Updated

2 months ago
No longer blocks: 1535568
Assignee

Updated

2 months ago
No longer blocks: 1535565

Updated

2 months ago
Duplicate of this bug: 1546182
Regressions: 1551183
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.