1182514 - Light-weight kidfox theme

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Profile Handling, defect)

Description

[Sam Fang [:sfang]]

Kid style themes, colors, gender-neutral

Possibility bundle add-on?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Take a look at AMO to see if any existing themes are "kid style" enough to bundle somehow.

Comment 2

[:Margaret Leibovic]

A lightweight theme is really just an image, so we could see if we can "bundle" one and find a way to apply it when launching with a kidfox profile.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I just want to point out there are still some lwt changes (meta: bug 1079416) to be made to improve the experience, specifically bug 1082928 and bug 1082542.

Comment 4

[Jenn Chaulk (:jchaulk)]

Cloned to 1186027 to move the UX work over to that bug.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret once played around with using Lightweight Themes from add-ons. Might be useful for this too:
https://github.com/leibovic/privacy-coach/commit/39612a31df29b2da68096b5177919d8d7cb48a45

Comment 6

[Robin Andersen [:tecgirl]]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Margaret once played around with using Lightweight Themes from add-ons.
> Might be useful for this too:
> https://github.com/leibovic/privacy-coach/commit/
> 39612a31df29b2da68096b5177919d8d7cb48a45

Thanks, Mark!

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1182514-kidfox-theme.patch

This is a quite hacky first patch. It installs the Kinderfox Cyan theme (bug 1191208) from the web for a restricted profile on app start.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8644912 [details] [diff] [review]
1182514-kidfox-theme.patch

installKidFoxTheme -> _installRestrictedTheme or _installParentalControlsTheme

(I'm not a fan of putting codenames in the code)

Comment 9

[Sam Fang [:sfang]]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> Created attachment 8644912 [details] [diff] [review]
> 1182514-kidfox-theme.patch
> 
> This is a quite hacky first patch. It installs the Kinderfox Cyan theme (bug
> 1191208) from the web for a restricted profile on app start.

Just to confirm, the light-weight theme is Green, not Cyan :)

Comment 10

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 8644912 [details] [diff] [review]
> 1182514-kidfox-theme.patch
> 
> installKidFoxTheme -> _installRestrictedTheme or
> _installParentalControlsTheme
> 
> (I'm not a fan of putting codenames in the code)

I agree here, but otherwise this patch seems pretty simple. My main concern would be whether or not this affects startup performance, but I assume it won't affect performance any more than a regular theme would.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1182514-kidfox-theme-v2.patch

This is the updated and simplified patch for web installation of the theme.

I dug into LightweightThemeManager.jsm to see whether I could load the theme from a local file. It seems like it supports resource:// uris but I couldn't find a way how I could use this to load a shipped file.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8650478 [details] [diff] [review]
1182514-kidfox-theme-v2.patch

Review of attachment 8650478 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +3131,5 @@
>    },
>  
> +  _installParentalControlsTheme: function() {
> +    this._install({
> +      "headerURL": "https://addons.cdn.mozilla.net/user-media/addons/636224/header.png",

I don't like that we'll require a network request to install this theme on first run. I think we should look into how desktop Firefox implements its recommended themes and do that instead. I see code here for that:
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#198
http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1365
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/defaultthemes/

You're right, we should use a resource: URI. I would look into how desktop Firefox does this and copy that. I'd hope that it would work the same way for Fennec.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

After reading a lot of code about themes, jars, manifest and the help of MDN I can:

* Package a theme header into the omni.ja
* Create an alias to obtain a resource:// uri to that header
* Install a theme with the resource URI at runtime
* Load the theme's header in Java if I know the complete(!) path inside the JAR(!)

The missing piece is translating a resource URI to the path inside the JAR so that I can pass that to GeckoJarReader. It seems like we do not have anything for that yet.

Right now LightweightThemeManager can handle http/https/file/resource URIs but our LightweightTheme class only knows how to handle http(s) URIs. For extending the functionality with the help of GeckoJarReader I'd need to translate the resource URI to the actual path. But I guess this will require to parse the manifests to lookup the actual path in the JAR?

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

nalexander gave me the hint to use resource://android/assets/. This seems to work pretty well. I'll finalize the patch based on that.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1182514-kidfox-theme-v3.patch

This patch now adds and installs a built-in theme served from the assets directory.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8651684 [details] [diff] [review]
1182514-kidfox-theme-v3.patch

Review of attachment 8651684 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. I wonder if we could use a similar approach to fix bug 1004517.

::: mobile/android/base/lwt/LightweightTheme.java
@@ +132,5 @@
> +        }
> +
> +        private Bitmap loadFromAssets(String url) {
> +            try {
> +                InputStream stream = mApplication.getAssets().open(url.substring(ASSETS_PREFIX.length()));

Do you need to close this stream after you're done with it?

::: mobile/android/chrome/content/browser.js
@@ +3133,5 @@
> +  _installParentalControlsTheme: function() {
> +    let mgr = this._manager;
> +    let parentalControlsTheme = {
> +      "headerURL": "resource://android/assets/parental_controls_theme.png",
> +      "name": "Parental Controls Theme",

Might be worth noting that we don't localize this because the add-on manager is disabled when parental controls are enabled, so users will never see this.

Although... is it possible for the restricted profile admin to enable add-on installs separately? Maybe we should file a follow-up to localize this for Fx43 (and uplift the non-localized patch).

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #16)
> ::: mobile/android/base/lwt/LightweightTheme.java
> @@ +132,5 @@
> > +        }
> > +
> > +        private Bitmap loadFromAssets(String url) {
> > +            try {
> > +                InputStream stream = mApplication.getAssets().open(url.substring(ASSETS_PREFIX.length()));
> 
> Do you need to close this stream after you're done with it?

Oh wow, yeah, definitely. Not sure how this slipped through.. :)


(In reply to :Margaret Leibovic from comment #16)
> Although... is it possible for the restricted profile admin to enable add-on
> installs separately? Maybe we should file a follow-up to localize this for
> Fx43 (and uplift the non-localized patch).

Yeah, the default is to hide it but the admin can enable it. I'll file a follow-up.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/c4cd293d7edd80740c9eae1368821307a7699c45
changeset:  c4cd293d7edd80740c9eae1368821307a7699c45
user:       Sebastian Kaspari <[e-mail]>
date:       Fri Aug 07 13:23:18 2015 +0200
description:
Bug 1182514 - Add Lightweight theme for restricted profiles. r=margaret

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4cd293d7edd

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1182514-kidfox-theme-v4.patch

Updating patch to version that landed (for uplift request).

Approval Request Comment
[Feature/regressing bug #]: Bug 1125984
[User impact if declined]: This is the last missing feature for KidFox v1 (Support for restricted profiles) (Bug 1125984). Requesting uplift because KidFox is targeting Fx42.
[Describe test coverage new/current, TreeHerder]: Local testing. Restricted profiles are not covered by UI tests yet.
[Risks and why]: Patch installs and applies a built-in theme at runtime. I verified that the theme is not installed for normal profiles and guest profiles. So I estimate the risk to be low.
[String/UUID change made/needed]: -

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8652354 [details] [diff] [review]
1182514-kidfox-theme-v4.patch

Approved for uplift to aurora. We want this for 42 and it looks like many of the kinks have been worked out now.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ca02c1d8027

Comment 23

[miralobontiu]

Hello,

Taking into consideration the long period of time since the qe-verify+ flag has been set, I will remove it.

Thank you!