Closed Bug 1429488 Opened 7 years ago Closed 6 years ago

Support moz-extension://URL images on Android

Categories

(WebExtensions :: Android, defect, P5)

All
Android
defect

Tracking

(firefox64 wontfix, firefox65 verified)

VERIFIED FIXED
mozilla65
Tracking Status
firefox64 --- wontfix
firefox65 --- verified

People

(Reporter: ntim, Assigned: JanH)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(10 files)

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
1.23 MB, application/x-zip-compressed
Details
1.96 MB, image/gif
Details
This is the only blocker to removing old style lightweight themes from Firefox.
Priority: -- → P5
Product: Toolkit → WebExtensions
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.
Assignee: nobody → jh+bugzilla
OS: Unspecified → Android
Hardware: Unspecified → All
Summary: Support basic lightweight theme set of properties on Android → Support moz-extension://URL images on Android
(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.
(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/
Depends on: 1502646
Depends on: 1502135
No longer depends on: 1502135
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.
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.
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.
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.
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.
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.
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.
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.
Attachment #9022163 - Attachment description: Bug 1429488 - Part 5: Support persisting moz-extension resources. r?jaws → Bug 1429488 - Part 5: Support persisting moz-extension resources. r?Gijs
Attachment #9022166 - Attachment description: Bug 1429488 - Part 8: Fix mobile about:addons code for enabling/disabling themes. r?jaws → Bug 1429488 - Part 8: Fix mobile about:addons code for enabling/disabling themes. r?aswan
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
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
Attachment #9022165 - Flags: approval-mozilla-beta?
Note: This uplift request applies to everything in this patch (parts 1 - 8 inclusive), as well as bug 1502646.
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.
Flags: qe-verify+
Attached file Bug1429488.zip
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?
Flags: needinfo?(jh+bugzilla)
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
Flags: needinfo?(jh+bugzilla)
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
Attached image Bug1429488.gif
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!
(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.
This issue is verified as fixed on Fennec 65.0a1(20181112100457) under Android 8.0.0. Thanks again, Jan!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(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?
Flags: needinfo?(jh+bugzilla)
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.
Flags: needinfo?(jh+bugzilla)
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.
Attachment #9022165 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
MDN compatibility data needs updating - basically, right now Android supports headerURL (but unlike Desktop it's still mandatory) and accentcolor.
Keywords: dev-doc-needed
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 ;-)
(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.
(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!

Looks like the compat data has now been fixed for this one (see https://github.com/mdn/browser-compat-data/issues/3233), so setting to DDC

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: