Tim Nguyen :ntim Reporter
	Description • 6 years ago

This is the only blocker to removing old style lightweight themes from Firefox.

	Jan Henning [:JanH] Assignee
	Comment 1 • 6 years ago

Supporting text colour would be nice, but not strictly necessary either, so I've spun that off into bug 1493519.

The only thing that's really necessary is supporting image URIs specified as moz-extension URIs.
If no one comes up with a better idea for that, I'll first try what I've described in bug 1406454 comment 15, i.e. intercepting those images in Android's LightweightThemeConsumer.jsm, persisting them to disk similar to what happens now with the LightweightThemeManager and rewriting to URL as a file://-URL before passing the theme on to the Android front-end.

	Jan Henning [:JanH] Assignee
	Comment 2 • 6 years ago

(In reply to Jan Henning [:JanH] from comment #1)
> If no one comes up with a better idea for that, I'll first try what I've
> described in bug 1406454 comment 15 [...]

Some other things came up in the meantime so this took a little longer than originally planned, however I've successfully managed to implement this approach. Patches will follow shortly once I've polished them up a little more.

	Tim Nguyen :ntim Reporter
	Comment 3 • 6 years ago

(In reply to Jan Henning [:JanH] from comment #2)
> (In reply to Jan Henning [:JanH] from comment #1)
> > If no one comes up with a better idea for that, I'll first try what I've
> > described in bug 1406454 comment 15 [...]
> 
> Some other things came up in the meantime so this took a little longer than
> originally planned, however I've successfully managed to implement this
> approach. Patches will follow shortly once I've polished them up a little
> more.

Thanks for working on this! \o/

	Jan Henning [:JanH] Assignee
	Comment 4 • 6 years ago

Most of robocop_head.js is based on xpcshell's head.js, but the log statements
should probably still use the new filename after all to reduce confusion.

Additionally, I've noticed that in do_report_unexpected_exception(),
Components.stack.caller can return null under some circumstances - to avoid
breaking error reporting as well as to make debugging easier, we should
therefore anticipate this possibility when getting caller_stack.filename.

	Jan Henning [:JanH] Assignee
	Comment 5 • 6 years ago

The head.js used for Android chrome mochitests has a few useful functions for
waiting on events and observer service notifications, which I don't want to
duplicate yet another time.

While superficially similar, the JS environment of Robocop tests differs in
several ways from that of (chrome) Mochitests. Thankfully the only relevant
difference here is that setTimeout() isn't directly available from within a
Robocop JS test. As we're only using that to set a 0-length timeout, we can
easily replace it with an equivalent dispatchToMainThread() call.

	Jan Henning [:JanH] Assignee
	Comment 6 • 6 years ago

This tests that the moz-extension URI for the headerURL will be properly
rewritten into a file URI before being passed to the Android front-end.

The JS part of the test has been adapted for Robocop from toolkit/mozapps/
extensions/test/browser/browser_webapi_theme.js.

Likewise, the test extension is based on browser_theme.xpi from the same
directory, but modified to include a real image file that can actually be
persisted.

	Jan Henning [:JanH] Assignee
	Comment 7 • 6 years ago

The Android front-end must be able to access theme resources before Gecko has
loaded. Therefore, historically it took advantage of the fact that for
Lightweight Themes we were persisting the files for the current theme on disk
anyway and directly accessed those files.

With Webextension static themes, all resources are now stored inside the
extension package, but to avoid having to teach the front-end how to access the
correct files from the correct extension package without any direct support from
Gecko, the easiest course of action is to continue persisting any required
resources to the profile folder.

Because the LightweightThemeManager behaves differently for WE static themes (it
no longer calls "lightweight-theme-styling-update" again after persisting has
finished in order to update the theme data with the location of the freshly
persisted files, because this is no longer required for WE static themes as they
are used on Desktop) and I've also gathered that the long-term intention is for
the LWTManager to be completely removed, the plan is to move responsibility for
persisting theme resources on Android into Android's LightweightThemeConsumer.

To that effect, we move the persisting code into its own JSM so that it can be
shared between the LWTManager (as long as it still exists) and Android's LWT-
Consumer.

	Jan Henning [:JanH] Assignee
	Comment 8 • 6 years ago

Bug 1344926 integrated static themes more closely into the existing infra-
structure for lightweight themes and also intended the static theme's image data
to be persisted to disk as well.

While the headerURL image file is in fact successfully copied out of the
extension archive into the profile, the persist progress listener being used
isn't equipped to properly handle this case and therefore the success callback
is never executed.

As a result
- the callback passed to _persistImages in the LWTManager isn't executed,
  either, although because setting the fallbackThemeData passes in an empty
  callback anyway, no one noticed.
- the persist operation never actually completes, so subsequent calls to
  currentThemeForDisplay() always return the original moz-extension:// image URI
  and never the persisted file from the profile folder.

For Android we definitively require a working callback in order to be able to
forward the fixed-up theme data once the image data has been persisted, so the
persistProgressListener's logic is modified accordingly.
Additionally, because as far as the LWTManager is concerned, WE static themes
are only fallback themes and a call to LWTManager.currentTheme will therefore
never return a WE static theme, the LWTPersister's logic to check whether the
theme, whose files have just been successfully persisted, is still the current
theme, needs to be modified.

	Jan Henning [:JanH] Assignee
	Comment 9 • 6 years ago

To optimise the behaviour of the LightweightThemePersister, we want the ID and
version of the theme that we want to persist to be available for static themes,
too.

	Jan Henning [:JanH] Assignee
	Comment 10 • 6 years ago

We intercept the theme data just before sending it to the front-end in order to
fix it up as described in part 4.

There is one snag, though: When the theme data processed in Android's
LightweightThemeConsumer isn't fresh data received via lightweight-theme-
styling-update, but instead a theme retrieved via LWTManager
.currentThemeForDisplay, we risk passing already persisted theme data
to LWTPersister.persistImages().

When the LWTPersister encounters an image which already has a file:// URI, it
just skips it and doesn't call the success callback in that case, which means we
never execute the rest of the code to pass the data on to the Android front-end.
Even if it did, this would mean we'd be calling LWTPersister.getPersistedData()
twice: Once implicitly through asking for currentThemeForDisplay, and a second
time explicitly.

So instead, we're asking the LWTManager for currentThemeWithFallback, which
returns the original theme data, which we can then safely pass to
persistImages() and fix up ourselves through getPersistedData().

This introduces a different problem, though: If the same theme has previously
already been successfully persisted, we'd be unnecessarily persisting it again,
and even worse, we'd do that on each startup.

To avoid that, we make the LWTPersister store and then check the persisted
theme's ID and version.
If they have remained unchanged, we can short-circuit the call to persistImages
and immediately declare success.

	Jan Henning [:JanH] Assignee
	Comment 11 • 6 years ago

about:addons on Android has some logic to ensure that only one theme can be
active at the same time.

At some point however, themes must have learned to take care of themselves in
that regard, which means that this code
a) has become unnecessary, and
b) with Webextension themes it can actually cause a harmful race condition:

Explicitly disabling the previously active theme implicitly enables the default
theme, so what happens is that depending on the size of the Webextension theme
to be enabled, the new theme can be enabled before the previous disable() call
completes. This leads to the new theme then immediately being disabled again as
the previous disable() call finally gets around to enabling the default theme.
The smaller the Webextension theme to be enabled and the faster it loads, the
more likely this is to happen.

We still need to manually fix up the disables state of themes in the UI, though,
so that it shows the correct state without a reload. Therefore, we take the
opportunity to fix the problem that up until now, disabling a theme wouldn't
mark the default theme as enabled in the UI, unless you manually reloaded the
page afterwards.

	Pulsebot
	Comment 12 • 6 years ago

Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d96a6d361db
Part 1: robocop_head.js improvements. r=gbrown
https://hg.mozilla.org/integration/mozilla-inbound/rev/2633e660f2c6
Part 2: Allow using mochitest-chrome's head.js for Robocop, too. r=gbrown
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e11568e7c82
Part 3: Test minimal Android static theme support. r=gbrown
https://hg.mozilla.org/integration/mozilla-inbound/rev/af60b25f4a0e
Part 4: Move code for persisting theme images into its own JSM. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/57806d472b17
Part 5: Support persisting moz-extension resources. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/116776356ba2
Part 6: Include some metadata in LWT data extracted from static themes. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e24466d3f96
Part 7: Move responsibility for persisting themes on Android to LWTConsumer. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/71df0988d340
Part 8: Fix mobile about:addons code for enabling/disabling themes. r=aswan

	Stefan Hindli [:stefan_hindli]
	Comment 13 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9d96a6d361db
https://hg.mozilla.org/mozilla-central/rev/2633e660f2c6
https://hg.mozilla.org/mozilla-central/rev/9e11568e7c82
https://hg.mozilla.org/mozilla-central/rev/af60b25f4a0e
https://hg.mozilla.org/mozilla-central/rev/57806d472b17
https://hg.mozilla.org/mozilla-central/rev/116776356ba2
https://hg.mozilla.org/mozilla-central/rev/9e24466d3f96
https://hg.mozilla.org/mozilla-central/rev/71df0988d340

	Jan Henning [:JanH] Assignee
	Comment 14 • 6 years ago

Comment on attachment 9022165 [details]
Bug 1429488 - Part 7: Move responsibility for persisting themes on Android to LWTConsumer. r?jaws

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: AMO migrating Lightweight Thems to Static Themes

User impact if declined: Android users already can't install freshly created (static) themes and once addons.mozilla.org migrates all existing Lightweight Themes to the new Static Theme format (which apparently is intended to happen soon), Android users won't be able to install any themes at all.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Install a few LWTs (e.g. https://addons.mozilla.org/de/firefox/addon/colorful-crazy-cats/, https://addons.mozilla.org/de/firefox/addon/batik-firefox/ or similar) and a few static themes with image backgrounds (https://addons.mozilla.org/en-US/firefox/search/?type=statictheme, might have to be visited in desktop mode if AMO claims that Android doesn't support those themes, and it *has* to be a theme with an image background - themes that merely change various UI colours around currently don't have any effect on Android) and test that everything works okay - installation, and various combinations of enabling and disabling themes.

List of other uplifts needed: 1502646 (test-only changes required for tests in this bug)

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): The main code for persisting images itself has remained unchanged other than having been moved into a separate module and therefore shouldn't be too risky, and most of the other changes are relative small, self-contained units, too. I've also been using the new code on a local build for a while and didn't notice anything strange,
but on the other hand I can't exclude the possibility that the new theme persisting integration into Android (mostly part 7 of this bug) has some subtle bugs that only come up under more obscure circumstances. So the risk might be not quite full "Medium" level, but I wouldn't feel entirely confident claiming it to be "Low", either.

As to my knowledge, AMO doesn't want to substantially delay the migration for the sake of Android, though, so the alternative would be Android users being unable to install themes for a month or two.

String changes made/needed: none

	Jan Henning [:JanH] Assignee
	Comment 15 • 6 years ago

Note: This uplift request applies to everything in this patch (parts 1 - 8 inclusive), as well as bug 1502646.

	Jan Henning [:JanH] Assignee
	Comment 16 • 6 years ago

Oh, and while not a direct dependency, bug 1502096 touches the same area of code in the LightweightThemeManager. Thankfully that bug has already been uplifted, so no separate Beta patch is required for this bug.

	CosminB
	Comment 17 • 6 years ago

I installed https://addons.mozilla.org/en-US/firefox/addon/arc-dark-theme-we/ on Fennec 65.0a1(20181112100457) under Android 8.0.0 but I was not able to see it, into the browser.

I attached a video and some logs.

Jan, could you please take a look at my attachment?

	Jan Henning [:JanH] Assignee
	Comment 18 • 6 years ago

I did say that
> [...] it *has* to be a theme with an image background -
> themes that merely change various UI colours around currently don't have any
> effect on Android

	Tim Nguyen :ntim Reporter
	Comment 19 • 6 years ago

Here's a query with some WebExtension themes with image backgrounds: https://addons.mozilla.org/en-US/firefox/search/?platform=mac&q=MaDonna&type=statictheme

	CosminB
	Comment 20 • 6 years ago

I installed some extensions from https://addons.mozilla.org/en-US/firefox/search/?platform=mac&q=MaDonna&type=statictheme 
Together with https://addons.mozilla.org/de/firefox/addon/colorful-crazy-cats/ and https://addons.mozilla.org/de/firefox/addon/batik-firefox/   

Install, enable/disable work as expected.

For the uninstall part, if the enabled theme is uninstalled, the default theme is not enabled in about:addons.

Is this expected?

Thank you for the help, Jan and Tim!

	Jan Henning [:JanH] Assignee
	Comment 21 • 6 years ago

(In reply to CosminB from comment #20)
> For the uninstall part, if the enabled theme is uninstalled, the default
> theme is not enabled in about:addons.

Oh right - as part of part 8, I fixed a similar problem for the case of disabling an add-on, but didn't think of the case of uninstalling an add-on.
It's a pre-existing issue, though, i.e. it affects Lightweight Themes as well and happens even without the patches for this bug, so thankfully no unintentional regression. So thanks for testing and as long as that is the only issue [1], we should still be good.

[1] Which would already be covered by bug 1505894, so no need for an extra follow-up bug.

	CosminB
	Comment 22 • 6 years ago

This issue is verified as fixed on Fennec 65.0a1(20181112100457) under Android 8.0.0. 

Thanks again, Jan!

	Julien Cristau [:jcristau]
	Comment 23 • 6 years ago

(In reply to Jan Henning [:JanH] from comment #14)
> User impact if declined: Android users already can't install freshly created
> (static) themes and once addons.mozilla.org migrates all existing
> Lightweight Themes to the new Static Theme format (which apparently is
> intended to happen soon), Android users won't be able to install any themes
> at all.
> 
Where can I read more about that / who do I talk to?  Especially around the timeline and why fennec is apparently not taken into account?

	Jan Henning [:JanH] Assignee
	Comment 24 • 6 years ago

I'm not quite sure, maybe try asking Jorge Villalobos?

As to why, presumably because
- When this issue first came up (bug 1406454), it wasn't yet super urgent from an Android perspective (LWTs were still working after all) and the fact that shortly afterwards the Taipei team was pulled from working on Fennec and Fennec itself put into maintenance mode didn't help with keeping this on anybody's radar.
- Because of this situation, I'm guessing that the AMO team didn't want to block the LWT to Static Theme conversion on Android, because it was unclear when, if ever, some minimal Static Theme support (at least comparable to LWTs) would be implemented on Android (and to be honest, I can't blame them for that)
- The next time this was brought up it was already September, with the launch of Static Themes imminent and the migration of existing LWTs scheduled for sometime soon, which together with the time taken to develop a patch (and maybe also that dropping theme support was still considered a possibility after all and I ended doing up the patch because I personally didn't want to lose theme support on Android and I only have a limited amount of time for this) takes us to where we are today.

	Julien Cristau [:jcristau]
	Comment 25 • 6 years ago

Comment on attachment 9022165 [details]
Bug 1429488 - Part 7: Move responsibility for persisting themes on Android to LWTConsumer. r?jaws

The migration of LWTs to static themes won't happen for android until 65.  Installing themes from AMO will be affected until then, but I think leaving this change until 65 is safer overall.

Thanks Jan for your work on this issue.

	Jan Henning [:JanH] Assignee
	Comment 27 • 6 years ago

MDN compatibility data needs updating - basically, right now Android supports headerURL (but unlike Desktop it's still mandatory) and accentcolor.

	Chris Mills [:cmills]
	Comment 28 • 5 years ago

Note to MDN writers:

I've added a note to cover this in the Fx65 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#Other_2

I'm not sure this is quite right, or exactly what else needs doing here. Please investigate ;-)

	Jan Henning [:JanH] Assignee
	Comment 29 • 5 years ago

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #28)
> I'm not sure this is quite right, or exactly what else needs doing here.
> Please investigate ;-)

Thanks. I've rewritten it a little, though, as the main point is actually the headerURL/theme_frame property support - the moz-extension:// protocol is merely an implementation detail.
I think the main point I had in mind was fixing up the compatibility data, for which I've filed https://github.com/mdn/browser-compat-data/issues/3233.

	Chris Mills [:cmills]
	Comment 30 • 5 years ago

(In reply to Jan Henning [:JanH] from comment #29)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #28)
> > I'm not sure this is quite right, or exactly what else needs doing here.
> > Please investigate ;-)
> 
> Thanks. I've rewritten it a little, though, as the main point is actually
> the headerURL/theme_frame property support - the moz-extension:// protocol
> is merely an implementation detail.
> I think the main point I had in mind was fixing up the compatibility data,
> for which I've filed https://github.com/mdn/browser-compat-data/issues/3233.

Cool, thanks!