783092 - (themes) Add lightweight theme (Persona) support

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Fabrice's old patch

We'd like to add some support for simple personalization using Lightweight Themes (Persona)

Fabrice started a patch a while ago. I'll add it as a WIP, but it's bitrotted.

Comment 1

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

CC' shorlander

He had a nice mockup of an idea for the startup page

Comment 2

[Stephen Horlander [:shorlander]]

Attachment: Lightweight Theme Mockup

Mockup of how it could work on the start page and in the toolbar while browsing.

Comment 3

[[:fabrice] Fabrice Desré]

That looks great, but persona headers are not tall enough to fill the full height of the start page. In the add-on I wrote for XUL fennec, I worked around that by fading into a gradient using the dominant color of the header image.

Comment 4

[Stephen Horlander [:shorlander]]

Attachment: Lightweight Theme Mockup (Existing Persona)

(In reply to Fabrice Desré [:fabrice] from comment #3)
> That looks great, but persona headers are not tall enough to fill the full
> height of the start page. In the add-on I wrote for XUL fennec, I worked
> around that by fading into a gradient using the dominant color of the header
> image.

Yes, that definitely could work for existing themes. Probably out of scope for this but I think for the best experience we should consider adding an option for submitting full sized mobile specific images. 

Or for even more future proofing just larger sizes in general that we could use for desktop and resize+crop for mobile. :)

Comment 5

[Ian Barlow (:ibarlow)]

Attachment: guidelines for how to apply desktop personas to Android

Sent this in an email, but it probably belongs here too

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

(So many things :O :O)
This patch adds the capability in the Gecko side to listen for LightweightTheme based events and passes it on to the Java. Gecko takes charge of downloading the image file. Java reads the image file and informs the listeners (who registered themselves to sport a persona image) about the new persona. Listeners ask the Persona for cropped drawable based on where they are in the window. They apply this as their background.

Design decisions:
1. Persona has to be single instance. (With all hatred over existing single instances :( ), I am creating the instance inside GeckoApplication -- which by itself is a single instance (yaaaay!).
2. Instead of Persona taking care of applying the background, the view takes care of how to apply the background. And since we have already created most of the views that render the curved background, this is just an extension of the capability.
3. Some views (like TabsButton, MenuButton) ask for drawable with "alpha" (yes! LayerDrawable and alpha are worst enemies! and have to do it in CanvasDelegate way). This is taken care of my a special kind of Drawable called PersonaDrawable.
4. AboutHomeContent is a special kind. Persona, by default, sees if the cropped image exacts fits the View. If not, it gets the dominant color, creates a ColorDrawable and adds the cropped image to it. The cropped image has to nicely blend with this ColorDrawable. Again, PersonaDrawable comes in for rescue. (Captain America!)

To Do:
1. Reset is not currently handled. The view knows (or actually, should know) its default background and reset it.
2. Desktop shows a "confirmation" doorhanger UI. The support is there in Gecko, just needs to be un-commented.
3. On rotation, AboutHomeContent (alone) doesn't refresh. This is a bit crazy UI, and there are a few race conditions. Needs a bit more effort to see when exactly to refresh it.
4. I haven't really tested on tablets yet. Might/might not work as expected. I need to tweak the XML a bit.
5. Each TextView (or the parent like AboutHomeSection) should register for Persona changes. Based on the brightness of the dominant color, they can change the text color. This is a one-time change for the views as long as they exist, and wouldn't regress performance.
6. I would like moving PersonaDrawable, TabsPanelToolbar out of their containing classes. This would make the code more readable and each class self contained.
7. Back/Forward buttons in tablet should be recreated like just another ShapedButton, and then Persona should be applied.
8. AwesomeBar should sport Persona (including SearchSuggestions).

Difficulties faces:
1. There is a setAlpha() for Drawable. Unfortunately LayerDrawable doesn't use this properly. For TabsButton, the image should have some alpha over the dark textured background. Android does some optimization and thinks that the image fits the entire view and doesn't draw the textured background. This makes the alpha drawable (set through bits in bitmap), to show the "+" from the TabsPanel. Hence PersonaDrawable mentioned above is the only way.
2. I tried getting the drawable by getResources().getDrawable() and finding the layer and replacing the image. I didn't work. LevelListDrawable and StateListDrawable doesn't really have option to get a Drawable for a particular level / state. Hence, any change in XML should be manually reflected in Java code too.
3. When registering as a listener, if the event is not posted to the UI thread, the app crashes as the width and height are not ready yet. So, we need to post it on UI thread. And this looks a bit ugly.
4. Padding gets reset when applying a background: http://www.mail-archive.com/[e-mail]/msg09595.html. Hence we need to manually restore the padding when changing the background.

Doubts:
1. Finding the dominant color:
   My current logic is to find the dominant color of the AboutHomeContent (only section that would need this) and use it for ColorDrawable (Design decision #4). However, this is done by Persona and wouldn't be available for AboutHomeSection (Todo #5). I made Persona take the color from the last row. This would ensure Persona to know the dominant color, and use it for ColorDrawable and inform TextViews. However, the effect wasn't as good as the first approach (of using a larger drawable). What's the minimum size that I can take to get the dominant color -- so that Persona has it.
2. Text colors:
   There are 3 text-colors in AboutHomeContent currently -- title, subtitle, link. What should be the colors for light and dark themes?
3. Opacity:
   I don't understand the 70% opacity in the spec. Which should have opacity and which should be opaque?

Suggestions:
   I somehow feel like adding the dominant color (with opacity) to the background of the TabsPanel. I am not sure how it'll look though.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Current state: http://cl.ly/image/0m002Z3O4225 and http://cl.ly/image/0G0z2Y180q2p

Comment 8

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

Comment on attachment 673133 [details] [diff] [review]
WIP

The general direction here is good.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/4): Change in animation for personas

This patch changes the animation used in tablets. Once the animation completes, the translation is made to be 0, and the margin is applied to the entire BrowserToolbar. Thereby the entire BrowserToolbar fits to be in the screen.

There might be a very small visual glitch while animating on closing tabs-tray. This cannot be avoided.

Also, the use of RightEdge is causing a visual glitch while animating, as its background doesn't animate with the entire animation.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/4): Personas

This patch is same as the WIP posted earlier -- with animations and rotations perfected. Currently there is no visual lag while rotating. The dominant color is found only once for an image (to preserve consistency while rotating).

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (3/4): Persona for awesomebar

This adds persona for the awesomebar. The "suggestions opt-in" has a transparent background and the awesomebar background fits there too.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (4/4): Persona for TabsTray

This patch adds the persona to the Tabs-Tray.

Comment 13

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

Comment on attachment 676050 [details] [diff] [review]
Patch (1/4): Change in animation for personas

Why do we need to do all the setTranslation calls in setTabsAnimation and resetTabsAnimation?

Otherwise it looks OK to me. I defer to Lucas for the actual r+

Comment 14

[Sriram Ramasubramanian [:sriram]]

The animation translates the views in the BrowserToolbar -- for smooth animation. Hence I reset the translation and add a margin -- which shrinks the BrowserToolbar as required.

Comment 15

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

Comment on attachment 676051 [details] [diff] [review]
Patch (2/4): Personas


>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java
>+    public void refreshAddressBarBackground() {

rename to refreshBackground ?

>+        mAddressBarBg.requestLayout();
>+
>+        if (mAwesomeBarRightEdge != null)

Not in love with mAwesomeBarRightEdge either. What exactly is the "right edge" doing?

>diff --git a/mobile/android/base/LightweightTheme.java b/mobile/android/base/LightweightTheme.java

>+        // Post the reset on the UI thread.
>+        for (OnChangeListener listener : mListeners) {
>+             final OnChangeListener oneListener = listener;
>+             oneListener.post(new Runnable() {

Are we sure this will always get called on the UI thread?

>+        // Post the change on the UI thread.
>+        for (OnChangeListener listener : mListeners) {
>+             final OnChangeListener oneListener = listener;
>+             oneListener.post(new Runnable() {

Same

>diff --git a/mobile/android/base/ShapedButton.java b/mobile/android/base/ShapedButton.java

>+    public void onAttachedToWindow() {
>+        super.onAttachedToWindow();
>+        post(new Runnable() {
>+            @Override
>+            public void run() {
>+                ShapedButton.this.mActivity.getLightweightTheme().addListener(ShapedButton.this);
>+            }
>+        });

This is the only onAttachToWindow that uses a runnable. Why?

>diff --git a/mobile/android/base/TabsButton.java b/mobile/android/base/TabsButton.java

>+        a = context.obtainStyledAttributes(attrs, R.styleable.TabsPanel);
>+        mSideBar = a.getBoolean(R.styleable.TabsPanel_sidebar, false);

>+        // If there is a side bar, the expanded state will have a filled button.
>+        if (mSideBar)
>+            levelList.addLevel(2, 2, stateList);
>+        else
>+            levelList.addLevel(2, 2, new ColorDrawable(Color.TRANSPARENT));

I would feel better if we used a different name for "mSidebar" (and the attribute). It seems like you want to know if the button is filled or transparent, right? Could we use "mOpaque" and "gecko:opaque"? If true, use "stateList" else use "Color.TRANSPARENT"

>diff --git a/mobile/android/base/TabsPanel.java b/mobile/android/base/TabsPanel.java

>-public class TabsPanel extends LinearLayout {
>+public class TabsPanel extends LinearLayout { 

You added trailing whitespace

>diff --git a/mobile/android/base/gfx/BitmapUtils.java b/mobile/android/base/gfx/BitmapUtils.java

>+          colors[h]++;
>+          sat[s]++;
>+          val[v]++;
>+          // we only care about the most unique non white or black hue, but also
>+          // store its saturation and value params to match the color better

nit: add a blank line before the comment

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+var LightWeightThemeWebInstaller = {
>+  init: function sh_init() {
>+    let temp = {};
>+    Cu.import("resource://gre/modules/LightweightThemeConsumer.jsm", temp);
>+    var theme = new temp.LightweightThemeConsumer(document);

var -> let

>+  get _manager () {
>+    var temp = {};

var -> let

>+  _installRequest: function (event) {

lots of var -> let

>+  _install: function (newLWTheme) {
>+    var previousLWTheme = this._manager.currentTheme;
>+
>+    var listener = {

var -> let

>+        let action = {
>+          label: Strings.browser.GetStringFromName("lwthemeNeedsRestart.button"),
>+          callback: function () {
>+            Application.restart();

This appears to be using FUEL which is not supported in Fennec. I assume you never saw a Persona that needed to be restarted after install?

Here is a snippet of code needed for restarting in Fennec:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4897

>+  _postInstallNotification: function (newTheme, previousTheme) {
>+    function text(id) {
>+      return Strings.browser.GetStringFromName("lwthemePostInstallNotification." + id);
>+    }

This function is not useful enough to keep and we use the raw "Strings.browser.GetStringFromName" everywhere else.

>+    var buttons = [{

var -> let

>+    //NativeWindow.doorhanger.show(text("message"), "Personas", buttons, BrowserApp.selectedTab.id);

Why is this disabled?

>+  _preview: function (event) {
>+    var data = this._getThemeFromNode(event.target);

var -> let

>+    this._previewWindow.addEventListener("pagehide", this, true);
>+    //gBrowser.tabContainer.addEventListener("TabSelect", this, false);

Why is this disabled? In Fennec, you want to use:

      BrowserApp.deck.addEventListener("TabSelect", this, false);

>+  _resetPreview: function (event) {

>+    //gBrowser.tabContainer.removeEventListener("TabSelect", this, false);

Same here

>+  _isAllowed: function (node) {
>+    var pm = Services.perms;
>+
>+    var uri = node.ownerDocument.documentURIObject;

var -> let

>+  _getThemeFromNode: function (node) {
>+    return this._manager.parseTheme(node.getAttribute("data-browsertheme"),
>+                                    node.baseURI);

No line wrap needed

>diff --git a/toolkit/content/LightweightThemeConsumer.jsm b/toolkit/content/LightweightThemeConsumer.jsm

>+let Cc = Components.classes;
>+let Ci = Components.interfaces;

Move these below EXPORTED_SYMBOLS

>+
>+Components.utils.import("resource://gre/modules/Services.jsm");
>+

Move below importing XPCOMUtils.jsm

>   handleEvent: function (aEvent) {
>+    // Fennec handles the resize in Java.
>+    if (Services.appinfo.ID == "{aa3c5121-dab2-40e2-81ca-7ea25febc110}")
>+      return;

I don't like this approach as a first attempt. Let's think about a different way to tackle it.

>   _update: function (aData) {

>+    if (Services.appinfo.ID == "{aa3c5121-dab2-40e2-81ca-7ea25febc110}") {
>+      this._updateFennec(aData);

Same

r-, you can start on the other changes while we think about LightweightThemeConsumer.jsm and we will also need a toolkit peer to review the LightweightThemeConsumer.jsm changes.

Comment 16

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> Comment on attachment 676051 [details] [diff] [review]
> Patch (2/4): Personas
> 
> 
> >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java
> >+    public void refreshAddressBarBackground() {
> 
> rename to refreshBackground ?

I will change it.

> 
> >+        mAddressBarBg.requestLayout();
> >+
> >+        if (mAwesomeBarRightEdge != null)
> 
> Not in love with mAwesomeBarRightEdge either. What exactly is the "right
> edge" doing?

RightEdge (now made a custom view) is used by lucasr to show the right curve of the url-bar. Basically the url-bar translates "behind" the "right edge, action-bar menu items and menu button". This makes it feel like the "url-bar" shrinks and doesn't actually "cut off".

> 
> >diff --git a/mobile/android/base/LightweightTheme.java b/mobile/android/base/LightweightTheme.java
> 
> >+        // Post the reset on the UI thread.
> >+        for (OnChangeListener listener : mListeners) {
> >+             final OnChangeListener oneListener = listener;
> >+             oneListener.post(new Runnable() {
> 
> Are we sure this will always get called on the UI thread?

Yup. oneListener is basically a View, and it has "post" method. Hence it will be called on the UI thread for sure.

> 
> >diff --git a/mobile/android/base/ShapedButton.java b/mobile/android/base/ShapedButton.java
> 
> >+    public void onAttachedToWindow() {
> >+        super.onAttachedToWindow();
> >+        post(new Runnable() {
> >+            @Override
> >+            public void run() {
> >+                ShapedButton.this.mActivity.getLightweightTheme().addListener(ShapedButton.this);
> >+            }
> >+        });
> 
> This is the only onAttachToWindow that uses a runnable. Why?

Aah. I didn't clean it up. Will change it. This doesn't need a Runnable here.

> 
> >diff --git a/mobile/android/base/TabsButton.java b/mobile/android/base/TabsButton.java
> 
> >+        a = context.obtainStyledAttributes(attrs, R.styleable.TabsPanel);
> >+        mSideBar = a.getBoolean(R.styleable.TabsPanel_sidebar, false);
> 
> >+        // If there is a side bar, the expanded state will have a filled button.
> >+        if (mSideBar)
> >+            levelList.addLevel(2, 2, stateList);
> >+        else
> >+            levelList.addLevel(2, 2, new ColorDrawable(Color.TRANSPARENT));
> 
> I would feel better if we used a different name for "mSidebar" (and the
> attribute). It seems like you want to know if the button is filled or
> transparent, right? Could we use "mOpaque" and "gecko:opaque"? If true, use
> "stateList" else use "Color.TRANSPARENT"

I am using "gecko:sidebar" just like we use with TabsPanel on tablets. This is for the consistency sake. (I believe we might move towards a "top-bar" like in Metro, and this wouldn't be needed).

> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+var LightWeightThemeWebInstaller = {
> >+  init: function sh_init() {
> >+    let temp = {};
> >+    Cu.import("resource://gre/modules/LightweightThemeConsumer.jsm", temp);
> >+    var theme = new temp.LightweightThemeConsumer(document);
> 
> var -> let

That was fabrice. :( Will change them.

> 
> >+        let action = {
> >+          label: Strings.browser.GetStringFromName("lwthemeNeedsRestart.button"),
> >+          callback: function () {
> >+            Application.restart();
> 
> This appears to be using FUEL which is not supported in Fennec. I assume you
> never saw a Persona that needed to be restarted after install?

I was about to ask about it. And then thought may be used by "Themes". I can remove that part.

> 
> Here is a snippet of code needed for restarting in Fennec:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#4897
> 
> >+  _postInstallNotification: function (newTheme, previousTheme) {
> >+    function text(id) {
> >+      return Strings.browser.GetStringFromName("lwthemePostInstallNotification." + id);
> >+    }
> 
> This function is not useful enough to keep and we use the raw
> "Strings.browser.GetStringFromName" everywhere else.

I will remove this function. This was used in 2-3 places. (But we haven't enabled "postInstallNotification" yet).

> 
> >+    //NativeWindow.doorhanger.show(text("message"), "Personas", buttons, BrowserApp.selectedTab.id);
> 
> Why is this disabled?

I wasn't sure about the flow yet. Do we need to show 2 doorhangers for each theme installed? "Are you sure you want to install?" + "You have just installed a theme".

Comment 17

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

> >diff --git a/toolkit/content/LightweightThemeConsumer.jsm b/toolkit/content/LightweightThemeConsumer.jsm

> >   handleEvent: function (aEvent) {
> >+    // Fennec handles the resize in Java.
> >+    if (Services.appinfo.ID == "{aa3c5121-dab2-40e2-81ca-7ea25febc110}")
> >+      return;
> 
> I don't like this approach as a first attempt. Let's think about a different
> way to tackle it.
> 
> >   _update: function (aData) {
> 
> >+    if (Services.appinfo.ID == "{aa3c5121-dab2-40e2-81ca-7ea25febc110}") {
> >+      this._updateFennec(aData);
> 
> Same

I am thinking about adding an nsINativeThemeConsumer (or module). Then LightweightThemeConsumer can query for the component (or module) and call nsINativeThemeConsumer.update(...) which would do whatever needed to happen for the native UI. Fennec would send the message to the Java UI.

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 676050 [details] [diff] [review]
Patch (1/4): Change in animation for personas

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

I'd like to see another version of this patch with the comments addressed and question answered. Could you please post an apk with these patches so that I can try it locally?

::: mobile/android/base/BrowserApp.java
@@ +563,4 @@
>  
> +                mGeckoLayout.requestLayout();
> +            }
> +        } else {

This refactoring is unrelated, right?

::: mobile/android/base/BrowserToolbar.java
@@ +514,5 @@
>          animator.attach(mSiteSecurity,
>                          PropertyAnimator.Property.TRANSLATION_X,
>                          width);
>  
> +        setTabsAnimation();

You're triggering a relayout at the beginning of the animation. This will likely cause several frames to get dropped at the beginning of the animation. I guess you can just translate then set margin on all cases?

@@ +519,1 @@
>          mTabsPaneWidth = width;

There's a subtle thing here that should probably be documented somewhere as a comment. You're using the previous value of mTabsPaneWidth in setTabsAnimation before the animation starts. Then you set it to the new value.

@@ +537,5 @@
> +        mFavicon.setTranslationX(mTabsPaneWidth);
> +        mSiteSecurity.setTranslationX(mTabsPaneWidth);
> +
> +        ((ViewGroup.MarginLayoutParams) mLayout.getLayoutParams()).leftMargin = 0;
> +    }

Given that setTabsAnimation and resetTabsAnimation have pretty much the same code with different values, you should probably have one method with an argument?

Comment 19

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #18)
> Comment on attachment 676050 [details] [diff] [review]
> Patch (1/4): Change in animation for personas
> 
> Review of attachment 676050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see another version of this patch with the comments addressed
> and question answered. Could you please post an apk with these patches so
> that I can try it locally?
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +563,4 @@
> >  
> > +                mGeckoLayout.requestLayout();
> > +            }
> > +        } else {
> 
> This refactoring is unrelated, right?

The about:home didnt resize on phones. Hence had to refactor it. (But actually this patch is wrong -- my new patch will have it right).

> You're triggering a relayout at the beginning of the animation. This will likely cause 
> several frames to get dropped at the beginning of the animation. I guess you can just 
> translate then set margin on all cases?

A re-layout is happening only for BrowserToolbar. I don't think all elements in it will have a re-layout (this happened to me when I was trying to do a mLayout.requestLayout() and mTabs didnt receive any). My assumption is that only mAddressBarBg alone is re-laid due to this. The reason behind not all elements are re-laid out is because of hardware acceleration. Please correct me if am wrong here. Also, this will not affect the phones. This is only for 10" tablets.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/4): Change in animation for personas

This changes the setTabsAnimation() and resetTabsAnimation() to use a single method -- adjustTabsAnimation(). I've addressed other questions in my previous comment.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2.1/4): LightweightThemeConsumer for Fennec

This patch disables the LightweightThemeConsumer.jsm in toolkit/content/ and adds a new one for Fennec in mobile/android/modules/.

Comment 22

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2.2/4): LightweightTheme support in browser.js

s/var/let is done.
I've cleaned up some parts of code as per the previous review comments.

Comment 23

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2.3/4): Persona support in Java

This patch adds the Persona support in Java side. This only has the LightweightTheme related files. No view is attached to this, with this patch.

Comment 24

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2.4/4): Persona for BrowserApp

This adds the persona support for views in BrowserApp. I've addressed the questions in my previous comment.

Comment 25

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

Comment on attachment 676718 [details] [diff] [review]
Patch (2.1/4): LightweightThemeConsumer for Fennec

I don't know if this is exactly what Blair was looking for, but it seems to fit the bill well enough for me. The mobile/android parts look OK.

I see we use the same OS_TARGET test in toolkit/content/Makefile.in
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Makefile.in#35

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 676717 [details] [diff] [review]
Patch (1/4): Change in animation for personas

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

As I expected, the a few frames are dropped at the beginning of the slide in animation. I don't think it worsens the situation too much though (the current animation is not that smooth on tablets anyway). So, giving my r+.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 676718 [details] [diff] [review]
Patch (2.1/4): LightweightThemeConsumer for Fennec

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

As a followup, I'd suggest looking at LightweightThemeImageOptimizer.jsm and only storing cropped images that are no bigger than the resolution of the device's screen. That will greatly reduce the amount of I/O and image decoding needed.

Are the other patches handling switching the text color for dark/light themes? (Looks like you're not using the textcolor property of themes.)

And I haven't seen any mention of text legibility or text shadows. On desktop, we use text shadows to make text significantly more legible when a lightweight theme is applied - it can be quite important on themes that add significant visual noise behind text (the shadow reduces that noise). The normal LightweightThemeConsumer calculates the luminance of the theme (which is a far better indicator than the brightness, and should apply well for text color too), and the shadow color is determined from that (dark themes get a black shadow, light themes get a white shadow).

::: toolkit/content/Makefile.in
@@ +75,5 @@
>    WindowDraggingUtils.jsm \
>    Troubleshoot.jsm \
>    $(NULL)
>  
> +ifneq (Android,$(OS_TARGET))

This should be MOZ_WIDGET_TOOLKIT.

Comment 28

[Sriram Ramasubramanian [:sriram]]

(In reply to Blair McBride (:Unfocused) from comment #27)
> Comment on attachment 676718 [details] [diff] [review]
> Patch (2.1/4): LightweightThemeConsumer for Fennec
> 
> Review of attachment 676718 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As a followup, I'd suggest looking at LightweightThemeImageOptimizer.jsm and
> only storing cropped images that are no bigger than the resolution of the
> device's screen. That will greatly reduce the amount of I/O and image
> decoding needed.

Yup. I could try cropping it to max-dev-width X available-height.

> 
> Are the other patches handling switching the text color for dark/light
> themes? (Looks like you're not using the textcolor property of themes.)

The dominant color is chosen in the Java side. And hence the text-color will be set there based on the dominant color. Thanks for reminding me. I can try using the textColor from the persona itself.

> 
> And I haven't seen any mention of text legibility or text shadows. On
> desktop, we use text shadows to make text significantly more legible when a
> lightweight theme is applied - it can be quite important on themes that add
> significant visual noise behind text (the shadow reduces that noise). The
> normal LightweightThemeConsumer calculates the luminance of the theme (which
> is a far better indicator than the brightness, and should apply well for
> text color too), and the shadow color is determined from that (dark themes
> get a black shadow, light themes get a white shadow).

This will all be in Java side and we would add a shadow layer if needed.

> 
> ::: toolkit/content/Makefile.in
> @@ +75,5 @@
> >    WindowDraggingUtils.jsm \
> >    Troubleshoot.jsm \
> >    $(NULL)
> >  
> > +ifneq (Android,$(OS_TARGET))
> 
> This should be MOZ_WIDGET_TOOLKIT.

I don't get this part. :(

Comment 29

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #28)

> > > +ifneq (Android,$(OS_TARGET))
> > 
> > This should be MOZ_WIDGET_TOOLKIT.
> 
> I don't get this part. :(

Blair is asking you to use this approach:
http://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#41

We do use that approach in Makefiles, but we actually use the OS_TARGET approach in this very Makefile already.

Blair, still want the MOZ_WIDGET_TOOLKIT approach here?
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Makefile.in#35

Comment 30

[Dave Townsend [:mossop]]

(In reply to Mark Finkle (:mfinkle) from comment #29)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #28)
> 
> > > > +ifneq (Android,$(OS_TARGET))
> > > 
> > > This should be MOZ_WIDGET_TOOLKIT.
> > 
> > I don't get this part. :(
> 
> Blair is asking you to use this approach:
> http://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#41
> 
> We do use that approach in Makefiles, but we actually use the OS_TARGET
> approach in this very Makefile already.
> 
> Blair, still want the MOZ_WIDGET_TOOLKIT approach here?
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Makefile.in#35

If we have something that uniquely identifies the build target as native fennec then I'd prefer that, otherwise using OS_TARGET is fine I think.

Comment 31

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

(In reply to Dave Townsend (:Mossop) from comment #30)

> > Blair, still want the MOZ_WIDGET_TOOLKIT approach here?
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Makefile.in#35
> 
> If we have something that uniquely identifies the build target as native
> fennec then I'd prefer that, otherwise using OS_TARGET is fine I think.

We don't have anything unique yet, but will soon in bug 797613. We can file new bugs for updating the Makefiles once that lands.

Comment 32

[Sriram Ramasubramanian [:sriram]]

Landed safely in inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f405fc4e323
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ae34fa1b3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/76232441ae06
https://hg.mozilla.org/integration/mozilla-inbound/rev/881eb2a2906e
https://hg.mozilla.org/integration/mozilla-inbound/rev/daccc3fe7c70
https://hg.mozilla.org/integration/mozilla-inbound/rev/06fcbaf40cb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4884aab5ce6

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this was backed out due to Android bustage that it was caught up in the middle of (the bustage couldn't be cleanly backed out without taking this with it). Feel free to re-land when the tree's a bit quieter.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2301d07ca953

Comment 34

[Sriram Ramasubramanian [:sriram]]

Landed (again) safely:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f6788ae5f6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c322955aff7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c27f644b081
https://hg.mozilla.org/integration/mozilla-inbound/rev/434fb54eccd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5786cc98c4d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bde6434a39a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7599929c5258

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f6788ae5f6d
https://hg.mozilla.org/mozilla-central/rev/6c322955aff7
https://hg.mozilla.org/mozilla-central/rev/9c27f644b081
https://hg.mozilla.org/mozilla-central/rev/434fb54eccd7
https://hg.mozilla.org/mozilla-central/rev/5786cc98c4d5
https://hg.mozilla.org/mozilla-central/rev/2bde6434a39a
https://hg.mozilla.org/mozilla-central/rev/7599929c5258

Comment 36

[Cristian Nicolae (:xti)]

I am able to add personas themes to the latest Nightly. Closing bug as verified fixed on:

Firefox 19.0a1 (2012-11-06)
Device: Galaxy S2
OS: Android 4.0.3

Comment 37

[Aaron Train [:aaronmt]]

:xti, can you check if there's a basic smoke-test for applying a theme (maybe archived from the XUL test-cases); if not, can you add-one?

Comment 38

[Cristian Nicolae (:xti)]

(In reply to Aaron Train [:aaronmt] from comment #37)
> :xti, can you check if there's a basic smoke-test for applying a theme
> (maybe archived from the XUL test-cases); if not, can you add-one?

I've added several testcases to Lightweight themes (Persona) suite: https://moztrap.mozilla.org/manage/cases/?filter-suite=180