1011738 - Theme support for b2g/gaia

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: themes.patch

wip. Still need to flush styles when we change themes instead of having to restart.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: themes.patch (gaia bits)

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: themes.patch (gaia bits)

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: themes.patch (gaia bits)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: themes.patch (gaia bits)

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Some paths are still incorrect and there is some small bits I need to fix in the lazy loader but that more or less the thing for Gaia.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: themes.patch (gaia bits)

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

One thing that still hard to style with this are application specific bits. The current idea is to use a app://theme.gaiamobile.org/APPNAME.css file in the theme app. Application will then opt-in to this file, and so it should be possible to style application specific bits.

That may be sometimes tricky with some of the lazy loading, as it may results into some specificity war or !important rules. That's probably OK as a first step, and that something to take into account in the future building blocs clean up.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: Gecko part

Patch ready for feedback, works both for in-process and out of process frames.

Here's how this works:
- The "theme.selected" setting can be changed from gaia to the manifest url of the themeing app. From there we set the "host" part of the current theme in the dom.mozApps.selected_theme preference. 

- dom.mozApps.selected_theme is used in the app:// protocol handler to check if we are accessing a theme resource (if it's origin is the same as the b2g.theme.origin pref) and if it's the case we override the host part of the appinfo to build the path to the zip without changing the uri.

- We also now whitelist theme resources in NeckoParent.

- Changing a theme means that we need to update theme related stylesheet without restarting gecko. nsGlobalWindow now observes the "app-theme-changed" observer notification and then checks which stylesheets are from the theme to reload them. I had to add a new "force reload" parameter to the css loader method to do that.

- When OOP, ContentParent observes "app-theme-changed" and remotes that to the ContentChild that relays the same observer notification.

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: pink_theme.png

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: green_theme.png

Comment 12

[[:fabrice] Fabrice Desré]

Paul, I made some security related changes here to allow cross-package loading of app://theme.gaiamobile.org/ urls. Let me know if that sounds problematic to you.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm curious (a) what feedback you want from me and (b) if there's a description somewhere about how the theming works (I'm most interested in what the entry points from outside-theme to inside-theme are).

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #13)
> I'm curious (a) what feedback you want from me and (b) if there's a
> description somewhere about how the theming works (I'm most interested in
> what the entry points from outside-theme to inside-theme are).

(a) Checking the changes I did in layout/style and in nsGlobalWindow.cpp
(b) Overall, theming works this way: apps include stylesheets with a url like:
 <link href="app://theme.gaiamobile.org/style.css>. There is no "theme.gaiamobile.org" app, but some other app (let's say green.gaiamobile.org) is selected as a theme provider. So the app:// protocol handler will fetch the resource style.css in the package from green.gaiamobile.org.

Since we want to be able to change themes without restarting b2g, I added code similar to nsChromeRegistry::RefreshWindow in order to reload the |app://theme.gaiamobile.org/...| stylesheets. This part is in nsGlobalWindow.cpp and is triggered by an observer notification.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8425173 [details] [diff] [review]
Gecko part

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

I only looked at the dom/ipc changes and they look good!

Comment 16

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Paul, I made some security related changes here to allow cross-package
> loading of app://theme.gaiamobile.org/ urls. Let me know if that sounds
> problematic to you.

So I assume:
- only certified apps will choose the choose the theme (its a setting and we will be careful not to expose it in the future, even after 846200)
- themes are only provided by certified apps (sounds really dangerous if third-party apps have this since there isn't any validation of theming resources)
- the theme app will be a sepearte standalone app and wont have anything sensitive in it (i.e. <iframe src='app://theme.gaiamobile.org/'> will embed content from the theming app, but wont be a security issue)
- we don't allow apps to choose the origin theme.gaiamobile.org. (I have a feeling the installer already blocks *.gaiamobile.org)

That's about all that I can think of, so if the above assumptions hold then I think this is ok.

Comment 17

[Kevin Grandon :kgrandon]

I don't want to bikeshed too much, but it seems like a waste if this doesn't work with our new shared web components which we will probably push third party app developers to use in the future.

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Paul Theriault [:pauljt] from comment #16)
> So I assume:
> - only certified apps will choose the choose the theme (its a setting and we
> will be careful not to expose it in the future, even after 846200)

Yes

> - themes are only provided by certified apps (sounds really dangerous if
> third-party apps have this since there isn't any validation of theming
> resources)

I think we will want to let third party provided themes. Let me know what validation we should do to make that acceptable. Is checking that we only have css/images good enough?

> - the theme app will be a sepearte standalone app and wont have anything
> sensitive in it (i.e. <iframe src='app://theme.gaiamobile.org/'> will embed
> content from the theming app, but wont be a security issue)

Yes. The will be a direct consequence of the previous point.

> - we don't allow apps to choose the origin theme.gaiamobile.org. (I have a
> feeling the installer already blocks *.gaiamobile.org)

We don't block that for now, but we should! I'll add that.

Comment 19

[Olle Klang]

Looks nice so far, but how are you supposed to change between two installed themes?

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Olle Klang from comment #19)
> Looks nice so far, but how are you supposed to change between two installed
> themes?

We'll have a new panel or section in the settings app, with a way to select the theme among all the apps that have a "theme" role. We'll ship the default one by default.

Comment 21

[Jason Duell]

Comment on attachment 8425173 [details] [diff] [review]
Gecko part

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

The necko bits look fine.

Comment 22

[Olle Klang]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #7)
> Created attachment 8424261 [details] [diff] [review]
> themes.patch (gaia bits)

Looks good, don't forget to add 'theme' as one of the HIDDEN_ROLES in homescreen/js/grid.js as theme apps should probably not show up in the homescreen.

Comment 23

[[:fabrice] Fabrice Desré]

Vivien, can you update your patch or post the script you used?

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: themes.patch

themes.patch (gaia bits)

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1011738.homescreen.ignore.theme.patch

The one line change for the homescreen to ignore 'theme'.

Comment 26

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #24)
> Created attachment 8428628 [details] [diff] [review]
> themes.patch
> 
> themes.patch (gaia bits)

hm, that doesn't apply on current master. For instance you have shared/style_unstable changes but this directory is gone.

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #26)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #24)
> > Created attachment 8428628 [details] [diff] [review]
> > themes.patch
> > 
> > themes.patch (gaia bits)
> 
> hm, that doesn't apply on current master. For instance you have
> shared/style_unstable changes but this directory is gone.

Yeah this has landed between the patch I attached and now... (see bug 1013952)

I will re-attach a new one.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: themes.patch

Here an updated version.

Comment 29

[[:fabrice] Fabrice Desré]

Attachment: gecko patch

I added a pref to turn off the feature if needed.

Vivien, I'm away the next 2 weeks, so feel free to push that further.

Comment 30

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to < away until June 17 > from comment #18)

> I think we will want to let third party provided themes. Let me know what
> validation we should do to make that acceptable. Is checking that we only
> have css/images good enough?

My concern is about injecting arbitrary CSS into gaia. As far as I can tell, this patch makes all gaia apps take at least some CSS from the theme app. This is extremely dangerous if we allow any and all developers to create themes. UI redressing and data exfiltration are the main concerns. This risk is why we block inline script in CSP in the first place. A theme could change characters on the keypad of the dialer, it could change the meaning of security prompts or phish for user details. Also previously security researchers have demonstrated complex ways to steal data by use of CSS alone [1].

The only options I can think of are to mitigate this risk are:
1. limit who can create a theme (e.g. certified, or some way restricted)
2. reduce which apps take a theme (Do we really want to style the system app?)
3. try to validate the theme CSS based on a whitelist of some kind (or maybe provide some kind of template for themes, instead of letting the theme app introduce arbitrary CSS). However trying to validate CSS to ensure that it is safe sounds like a very difficult and error prone task (akin to HTML sanitization). 


[1] http://www.nds.rub.de/media/emma/veroeffentlichungen/2012/08/16/scriptlessAttacks-ccs2012.pdf

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry for being late to the party, but I think we should restrict this to certified app only too (both the theme app and the app who use the theme). Our permission model allow us to do just that today. I am afraid that exposing this capability to 3rd-party makes the packaged app environment less and less webby (by asking people to use special URLs to get theme CSSes). We also did not fix the versioning issue too (Imaging if everyone is getting Twitter Bootstrap from one URL and imagine that file somehow need to be changed.)

We should be bold in terms of innovating on certified-only packaged app features, but be very careful when considering exposing them to 3rd parties.

Comment 32

[Olle Klang]

If I'm not mistaken, changing fonts via a theme app would not be possible as same origin restriction would not allow to load a font file from a different app. Also changing wallpaper images will suffer from this as they are read into a canvas to be displayed. Any thoughts on how to make this work?

Comment 33

[Wilson Page [:wilsonpage]]

(In reply to Olle Klang from comment #32)
> If I'm not mistaken, changing fonts via a theme app would not be possible as
> same origin restriction would not allow to load a font file from a different
> app. Also changing wallpaper images will suffer from this as they are read
> into a canvas to be displayed. Any thoughts on how to make this work?

If fonts are served with the `Access-Control-Allow-Origin` header from the theme app they would be fine. Alternatively we could inline them into the CSS, although this would add some bloat.

Comment 34

[Olle Klang]

(In reply to Wilson Page [:wilsonpage] from comment #33)
> If fonts are served with the `Access-Control-Allow-Origin` header from the
> theme app they would be fine. Alternatively we could inline them into the
> CSS, although this would add some bloat.

OK, so adding fonts should work. Would a similar approach solve the wallpaper issue?

Comment 35

[Olle Klang]

Attachment: Bug-1011738-Adds-theme-section-in-settings-app_v1.patch

Adding a patch for a themes section in settings app where you can change between installed themes.

Comment 36

[Olle Klang]

Attachment: Bug-1011738-Adds-theme-section-in-settings-app_v2.patch

Themes settings section as an AMD module.

Comment 37

[Wilson Page [:wilsonpage]]

I'm not that familiar with these Gecko patches. Can anyone give me some steps to run through so that I can try this out on device?

Comment 38

[Stephany Wilkes]

Shouldn't this have ui-review? set, especially since we're working on our theme-ability for 2.1?

Comment 39

[Olle Klang]

(In reply to Wilson Page [:wilsonpage] from comment #37)
> I'm not that familiar with these Gecko patches. Can anyone give me some
> steps to run through so that I can try this out on device?

Apply the following patches with git apply:
gecko patch -> in your gecko folder
themes.patch (pick the latest) -> in you gaia folder
Bug-1011738-Adds-theme-section-in-settings-app_v2.patch -> in your gaia folder

Rebuild gecko and gaia:
https://developer.mozilla.org/en-US/Firefox_OS/Building

Comment 40

[Wilson Page [:wilsonpage]]

(In reply to Olle Klang from comment #39)
> Apply the following patches with git apply:
> gecko patch -> in your gecko folder
> themes.patch (pick the latest) -> in you gaia folder
> Bug-1011738-Adds-theme-section-in-settings-app_v2.patch -> in your gaia
> folder
> 
> Rebuild gecko and gaia:
> https://developer.mozilla.org/en-US/Firefox_OS/Building

That worked for your last Gaia patch, but the Gecko and other Gaia patch warn: 'patch does not apply' for many of the files. What am I doing wrong?

Comment 41

[Wilson Page [:wilsonpage]]

I am unable to test this patch, but from looking at the diffs it seems that a theme comprises of several stylesheets that target specific building blocks. I'm not sure if this is just for demonstration purposes or not.

Myself and Casey have been working on a solution that should be far more scalable than this approach. It involves decoupling the theme from the component it's styling, so that a single theme.css could style 1000s of different components, and a single component could be styled by 1000s of different themes.

A theme.css would comprise of a collection of CSS variables (custom properties) conforming to a known theme spec. At a bare minimum this would be a simple colour pallet, but could be shadows, borders, backgrounds, etc. A component or app author would then opt parts in to by dynamically themeable, with a appropriate fallback if a theme weren't present.

Here is an early (unfinished) working example of this that myself and Casey have worked on: http://gaia-components.github.io/gaia-theme/

In order to change themes, Gaia would simply have to switch the single theme.css file currently being served by the theme app.

SUB-THEMES

We don't want all our Gaia app to look exactly the same, this is were 'sub-themes' come in. A theme.css can optionally provide a sub-themes, and apps can opt into a sub-theme by adding a class to an element: 

<body class="theme-media">

You can see this approach in action in the above example.

Would like to hear feedback on this suggestion, hope it aligns wit what you lot have been working on :)

Comment 42

[Olle Klang]

(In reply to Wilson Page [:wilsonpage] from comment #40)
> That worked for your last Gaia patch, but the Gecko and other Gaia patch
> warn: 'patch does not apply' for many of the files. What am I doing wrong?
The patches are getting old so you are getting conflicts with new changes in gecko and gaia since they where created.
If you want you can force apply the patch using --reject flag. This will require some manual work since you'll have to manually resolve all conflicts. 

Using the --reject flag will create a $filename.rej file containing the diffs for every file with unresolved conflicts.

Or we could just ask Vivien and Fabrice to update their patches :)

Comment 43

[[:fabrice] Fabrice Desré]

Attachment: gecko patch rebased

Comment 44

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Wilson Page [:wilsonpage] from comment #41)
> I am unable to test this patch, but from looking at the diffs it seems that
> a theme comprises of several stylesheets that target specific building
> blocks. I'm not sure if this is just for demonstration purposes or not.
> 
> Myself and Casey have been working on a solution that should be far more
> scalable than this approach. It involves decoupling the theme from the
> component it's styling, so that a single theme.css could style 1000s of
> different components, and a single component could be styled by 1000s of
> different themes.
> 
> A theme.css would comprise of a collection of CSS variables (custom
> properties) conforming to a known theme spec. At a bare minimum this would
> be a simple colour pallet, but could be shadows, borders, backgrounds, etc.
> A component or app author would then opt parts in to by dynamically
> themeable, with a appropriate fallback if a theme weren't present.
> 

This is actually something I looked at. I think this would be much less intrusive and would let app author opt-in to the system theme even with custom css/html that differs from the building blocks.

That's something we have already explored in bug 849223 with the old css 3 colors module spec. Sadly the number of keywords was limited and it makes it hard to use for various use case and there was not fallback support AFAICT.

With the current patch from Fabrice you can already do that, just import a theme.css file that contains variable and that's it.

> Here is an early (unfinished) working example of this that myself and Casey
> have worked on: http://gaia-components.github.io/gaia-theme/
> 
> In order to change themes, Gaia would simply have to switch the single
> theme.css file currently being served by the theme app.
> 

What I would really like to see is to hide the theme.css link from the app itself.
So apps would not change their current html files and ask for a theme.css file with a special magic url, but instead the platform will inject this CSS file inside the childs - or just override the variables directly, and if app authors have opt-in the system theme by using the right css variables names, then the system theme would apply.

If there is no system theme (on other web app capable runtime), then nothing will happens and the app will just use it's fallback mechanims or a custom theming system with built-in variable stylesheets.

> SUB-THEMES
> 
> We don't want all our Gaia app to look exactly the same, this is were
> 'sub-themes' come in. A theme.css can optionally provide a sub-themes, and
> apps can opt into a sub-theme by adding a cl.ass to an element: 
> 
> <body class="theme-media">
> 

I think this part really belongs to the app author itself. That may be how you define your css internally, and select the appropriate variables based on this selector, but I think we agree that this is an app level decision ?

> You can see this approach in action in the above example.
> 
> Would like to hear feedback on this suggestion, hope it aligns wit what you
> lot have been working on :)

It does. The patches here has been built in a few days after some conversations to see how we can solve the various use cases we got. At first we took an approach that let us do everything - it's good but it has some issues, like introducing a custom url for apps theming, as well as making things a bit painful in a security point of view.

Using variables has always been a possible solution (as we tried to push for it in the bug I mentioned earlier) but the previous variable system we got was not scaling very well.

CSS Variables with fallbacks seems a good idea to me.

Comment 45

[Wilson Page [:wilsonpage]]

One thing to point out is that within the <gaia-header> currently being rolled out across Gaia (bug 1005830). Styling is done within the shadow-dom; this means that CSS-Variables are currently (before `/deep/` and `/shadow/` selectors land) the only way to alter styling from the outside document.

Comment 46

[[:fabrice] Fabrice Desré]

Even if we'd like to use a css variable approach in an ideal world, that will not happen in the 2.1 timeframe so I'm moving forward with the "full css overriding" mechanism.

The way it works is as follows:
- Anytime an app tries to load a css from the app://theme.gaiamobile.org origin, we substitute the resource by one coming from the currently selected theme. When there is a theme change, we propagate that to all windows to let them force-reload the app://theme.gaiamobile.org stylesheets.
- For security reasons, and because this will not be backward compatible with css variable themes, only certified apps can be themes (ie they need role="theme" in their manifest). Moreover, only apps that have the "themeable" permission can use themes. For now I locked that down to certified apps also.

Comment 47

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : settings hook-up

In which we map the settings to the gecko pref and dispatch the observer notification.

Comment 48

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : stylesheet reloading

David, I had to had a "force reload" flag since even if the uri don't change, we actually want to reload a different stylesheet. So it's touching a bunch of callers...

Comment 49

[[:fabrice] Fabrice Desré]

Attachment: dom/apps checks for role="theme"

In which we only let certified apps with role == "theme" install and update. Pretty sad to have to do that for a temporary solution...

Comment 50

[[:fabrice] Fabrice Desré]

Attachment: Part 4 : security checks and IPC

Hhooking up the security checks, and propagating the observer notification to child processes.

Comment 51

[[:fabrice] Fabrice Desré]

Attachment: Part 5 : tests

Make sure we don't let non certified apps be installed as themes.

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #48)
> Created attachment 8470380 [details] [diff] [review]
> Part 2 : stylesheet reloading
> 
> David, I had to had a "force reload" flag since even if the uri don't
> change, we actually want to reload a different stylesheet. So it's touching
> a bunch of callers...

Hrm.  This sounds like you're going down the same road that Firefox theme switching was going down back when we tried to get dynamic theme switching working well enough in Firefox and then gave up.

The basic problem I'm worried about is the one outlined in bug 226791 comment 52.  The approach I took to try to fix that failed because I couldn't figure out a way around the performance regression from bug 252703.


From comment 14 it sounds like the only entry points from non-theme to theme are style sheets.  That sounds good, but I'd like to confirm it since I'm not 100% sure.

Then the other question is:  what resources can a theme contain?  In other words, can these theme style sheets link to (a) other style sheets (b) images (c) other resources (e.g., SVG filters) whose contents might *also* change when the theme changes despite URLs being identical?  If so, then a patch of this sort isn't sufficient, because you need something to propagate those changes through the system and make URL equality checks fail.  (That's what I was trying to do in bug 252703.)


(Still, at least you're not using RDF and running into problems like bug 101968 comment 39!)

Comment 53

[[:fabrice] Fabrice Desré]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #52)

> Hrm.  This sounds like you're going down the same road that Firefox theme
> switching was going down back when we tried to get dynamic theme switching
> working well enough in Firefox and then gave up.
> 
> The basic problem I'm worried about is the one outlined in bug 226791
> comment 52.  The approach I took to try to fix that failed because I
> couldn't figure out a way around the performance regression from bug 252703.

Ha.. I wondered why we had no such live swapping for xul themes...

> From comment 14 it sounds like the only entry points from non-theme to theme
> are style sheets.  That sounds good, but I'd like to confirm it since I'm
> not 100% sure.

Yes, that's the case.

> Then the other question is:  what resources can a theme contain?  In other
> words, can these theme style sheets link to (a) other style sheets (b)
> images (c) other resources (e.g., SVG filters) whose contents might *also*
> change when the theme changes despite URLs being identical?  If so, then a
> patch of this sort isn't sufficient, because you need something to propagate
> those changes through the system and make URL equality checks fail.  (That's
> what I was trying to do in bug 252703.)

Yes, style sheets can potentially link to other css, images, fonts, etc. I'll take a look at the URL equality patch from 252703, but maybe we can try to:
- flush image / font caches before applying the new theme.
- since themes are certified, we could enforce that all these inner resources have unique urls. For instance, theme A would define:
.wifi-on {
  background-image-url: url(/themeA/images/wifi-on.png)
}
and theme B :
.wifi-on {
  background-image-url: url(/themeB/images/wifi-on.png)
}

I'm not fond of this solution since it's obviously fragile and will not scale once we want to allow 3rd party themes.

In any case I will update my test themes to include images and check what I can do.

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #53)
> Yes, style sheets can potentially link to other css, images, fonts, etc.
> I'll take a look at the URL equality patch from 252703, but maybe we can try
> to:
> - flush image / font caches before applying the new theme.

Ah, downloadable fonts.  So many new things on the Web since I last thought about this problem seriously. :-)

In any case, one reason that's not sufficient is things like style change handling.  When computed styles change, we compare the new and old styles to determine what to doabout the change (see the CalcDifference methods in layout/style/nsStyleStruct.cpp, the hints in layout/base/nsChangeHint.h, and the code that handles the hints in RestyleManager::ProcessRestyledFrames).  So if the EqualURIs function returns true for two background images, we're not going to do any of the work that we're supposed to do when the background-image changes.

> - since themes are certified, we could enforce that all these inner
> resources have unique urls. For instance, theme A would define:
> .wifi-on {
>   background-image-url: url(/themeA/images/wifi-on.png)
> }
> and theme B :
> .wifi-on {
>   background-image-url: url(/themeB/images/wifi-on.png)
> }
> 
> I'm not fond of this solution since it's obviously fragile and will not
> scale once we want to allow 3rd party themes.

Yeah, that would address that particular problem.  It's certainly not pleasant, though.

> In any case I will update my test themes to include images and check what I
> can do.

The interesting cases involve the two themes having a style rule that looks exactly the same -- i.e., produces the same URL, but where that URL now resolves to a different resource.

Comment 55

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8470379 [details] [diff] [review]
Part 1 : settings hook-up

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

::: b2g/chrome/content/settings.js
@@ +723,5 @@
> +  try {
> +    let doIt = Services.prefs.getBoolPref("dom.mozApps.themable");
> +    if (!doIt) {
> +      return;
> +    }

s/doIt/isThemeable

@@ +738,5 @@
> +      return;
> +    }
> +  } catch(e) {
> +    return;
> +  }

nit: I wonder if you don't want to do the try/catch around the whole block, instead of having 2 try/catch.

Comment 56

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8440545 [details] [diff] [review]
Bug-1011738-Adds-theme-section-in-settings-app_v2.patch

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

::: apps/settings/js/panels/themes/themes.js
@@ +55,5 @@
> +          var span = document.createElement('span');
> +          var manifest =
> +            new ManifestHelper(app.manifest || app.updateManifest);
> +          span.setAttribute('data-l10n-id', manifest.name);
> +          span.innerHTML = manifest.name;

Possible XSS since manifest.name is user controlled afaik? Not really an issue since theme apps will be certified initially, but any reason not to do the following? 

span.textContent = manifest.name;

Comment 57

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8470381 [details] [diff] [review]
dom/apps checks for role="theme"

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

::: dom/apps/src/AppsUtils.jsm
@@ +367,5 @@
>  
>    /**
> +    * Checks if the app role is allowed.
> +    * Only certified apps can be themes.
> +    */

Nit: I would align the asterisks on the second and subsequent lines to the first one on the first line à la Javadoc.

::: dom/apps/src/Webapps.jsm
@@ +3442,5 @@
> +    let app = {
> +      role: newManifest.role || "",
> +      appStatus: status
> +    };
> +    if (!AppsUtils.checkAppRole(app)) {

Nit: since checkAppRole only ever looks at the "role" and "appStatus" values, it'd be simpler for it to take "role" and "status" arguments, as you wouldn't have to create a fake "app" object here.

Comment 58

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8470383 [details] [diff] [review]
Part 5 : tests

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

::: dom/apps/tests/test_theme_role.html
@@ +51,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +/**
> +  * Install 2 apps from the same origin and uninstall them.

Nit: perhaps this is a remnant from another file you copy-pasted?

Comment 59

[Olle Klang]

After testing you patches with a default theme app (ie. all resourses copied from shared/style and put in a theme app) the css gets swapped out properly but no images gets loaded. As an example the settings app will not display the images for any switch or checkbox although the image links show up correctly in the code. 
Do you get the same?

Comment 60

[[:fabrice] Fabrice Desré]

(In reply to Olle Klang from comment #59)
> After testing you patches with a default theme app (ie. all resourses copied
> from shared/style and put in a theme app) the css gets swapped out properly
> but no images gets loaded. As an example the settings app will not display
> the images for any switch or checkbox although the image links show up
> correctly in the code. 
> Do you get the same?

Hi Olle, yes it's a known bug in the current patch, sorry. I have a local fix, but it's not yet letting us switch images when we switch themes. Hopefully I'll post a fix later today.

Comment 61

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : stylesheet reloading v2

David, this patch doesn't use app:// redirects since I don't believe we can do that in the 2.1 timeframe, but clears the image cache when we change theme. I feel this is acceptable since that's not a common occurrence.

Comment 62

[[:fabrice] Fabrice Desré]

Attachment: Part 4 : security checks and IPC v2

Rebased, and fixed an error when loading resources from a stylesheet.

Comment 63

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8474635 [details] [diff] [review]
Part 4 : security checks and IPC v2

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

::: caps/nsScriptSecurityManager.cpp
@@ +698,5 @@
> +            // have the webapps-manage permission but have the themeable one.
> +            // Resources from the theme origin are also allowed to load from
> +            // the theme origin (eg. stylesheets using images from the theme).
> +            nsAdoptingCString themeOrigin;
> +            themeOrigin = Preferences::GetCString("b2g.theme.origin");

Should probably file a followup on caching to avoid having to call all these different pref getters for every load. In nsCSPService and AppProtocolHandler too.

@@ +700,5 @@
> +            // the theme origin (eg. stylesheets using images from the theme).
> +            nsAdoptingCString themeOrigin;
> +            themeOrigin = Preferences::GetCString("b2g.theme.origin");
> +            nsAutoCString targetOrigin;
> +            targetBaseURI->GetPrePath(targetOrigin);

I think you should use nsPrincipal::GetOriginForURI here and everywhere else you use PrePath (It's different, it at least includes things like username).

@@ +713,4 @@
>              // In this case, we allow opening only if the source and target URIS
>              // are on the same domain, or the opening URI has the webapps
>              // permision granted
> +            if (!SecurityCompareURIs(sourceBaseURI, targetBaseURI) &&

You should get bholley to sign off on changes to this file.

::: content/base/src/nsCSPService.cpp
@@ +138,2 @@
>            nsAutoCString sourceOrigin;
>            aRequestOrigin->GetPrePath(sourceOrigin);

Hrm, this is weird...

::: netwerk/ipc/NeckoParent.cpp
@@ +583,5 @@
>          netErrorWhiteList = spec.Equals(NS_ConvertUTF16toUTF8(netErrorURI).get());
>        }
>      }
>  
> +    // Check is we load a resource from the shared theme url space.

Nit: s/is/if/

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +392,5 @@
> +
> +
> +  nsAdoptingCString themeOrigin;
> +  themeOrigin = Preferences::GetCString("b2g.theme.origin");
> +  if (Preferences::GetBool("dom.mozApps.themable") &&

Seems like all the changes here should only be attempted if this pref is true, so I'd check that first before getting origins, etc.

Comment 64

[[:fabrice] Fabrice Desré]

Attachment: Part 4 : security checks and IPC v3

Addressing review comments from Ben.

Bobby, can you look at the changes in nsScriptSecurityManager.cpp ?

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8474633 [details] [diff] [review]
Part 2 : stylesheet reloading v2

You need to rev NS_IDOCUMENT_IID.

OnAppThemeChanged should assert at the start that IsInnerWindow().

I don't understand why nsGlobalWindow::OnAppThemeChanged needs a
recursive call for subframes.  Aren't the subframes also observing
the same topic?

(That said, I'm not even sure why this is on the window at all, though
I suppose it makes things easy.)

Then, given that, I don't see why you need to add OnAppThemeChanged to
nsPIDOMWindow (though it's probably easily avoidable anyway by changing
the subframe handling code).  If I'm missing something, though, you need
to rev NS_PIDOMWINDOW_IID and you should add MOZ_OVERRIDE to the
declaration on nsGlobalWindow.  Better to just remove it, though, and
make it non-virtual.

You should add comments both to this method and to
nsChromeRegistry::RefreshWindow that they're basically copies of each
other.

+  // Get the DOM document.
+  nsCOMPtr<nsIDOMDocument> domDocument;
+  this->GetDocument(getter_AddRefs(domDocument));
+  if (!domDocument)
+    return;
+
+  nsCOMPtr<nsIDocument> document = do_QueryInterface(domDocument);
+  if (!document)
+    return;

Replace all this with mDoc.


>+  // Now notify the document that multiple sheets have been added and removed.
>+  document->UpdateStyleSheets(oldSheets, newSheets);

You should maintain a boolean that says whether you actually
replaced any style sheets, and only call UpdateStyleSheets when
you did.

(That's less of an issue for the nsChromeRegistry case since it's only
running on chrome windows.)

Could you file a followup bug on converting the long list of boolean
parameters to LoadSheetSync into a single flags parameter, which is
much more readable?

I'd like you to run the imgLoader changes by seth and the patch in general
by a DOM peer (probably bz, though others are ok if he's too busy).

I think there's some risk that issues with theme changing will come up
given this approach, though hopefully not at the release-blocking level;
it's probably worth figuring out a better approach involving something
more like redirects, and then reverting at least the image cache clearing
changes here.

r=dbaron with those changes, though

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Also, in general, our C++ style avoids "this->".  I'm pointing this out explicitly since you wrote two uses, but I told you to remove both of them for other reasons.)

Comment 67

[Bobby Holley (:bholley)]

Comment on attachment 8476317 [details] [diff] [review]
Part 4 : security checks and IPC v3

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

I'm about to head out on PTO, and I may not get to this. Boris, can you review the nsScriptSecurityManager.cpp stuff in my stead?

Comment 68

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8474633 [details] [diff] [review]
Part 2 : stylesheet reloading v2

I'm not terribly happy with the behavior this produces for stylesheets.

After this patch, if OnAppThemeChanged happens the document's stylesheets array will be updated, but the stylesheets pointed to by the actual <link> elements will not be changed, correct?  If someone then pokes at those <link> sheets things will likely behave pretty weirdly.

Furthermore, the old cached sheet is not actually getting cleared as far as I can tell (because the new load uses a different origin principal than the <link> load did) so moving the <link> in the DOM will likely apply _both_ sheets or something.

This doesn't handle the case of a theme change while sheets are loading, afaict; that would cause a cache hit in the loader but possibly get the wrong sheet data.

Finally, this serializes the sheet loads instead of allowing them to hit the disk in parallel.  Not sure how much of an issue that is in practice.

I would vastly prefer an implementation strategy where we drop the app:// sheets from the loading/complete sheets hashtables and then retrigger sheet loads for those <link> elements.  Though even that would race if there's an in-progress sheet load at theme switch time...

Comment 69

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8476317 [details] [diff] [review]
Part 4 : security checks and IPC v3

In the security manager, you should probably skip the new code if themeOrigin.IsEmpty(), no?

Similar in the CSP code.

>@@ -378,16 +382,33 @@ AppProtocolHandler::NewChannel(nsIURI* a

The indentation in the new code here has serious issues.  Please fix it.

r=me with those

Comment 70

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8476317 [details] [diff] [review]
Part 4 : security checks and IPC v3

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

Yeah, with bz's suggestions I think this is fine.

Comment 71

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : stylesheet reloading v3

Rewrote after discussing with bz. That ended up being less invasive than the previous version, and seems a bit faster on device (less visual flashing).

Seth, asking you to look at the imgLoader changes: we clear the caches when receiving the "app-theme-changed" notification to workaround the issue with comparing ImageValue(s) in the css engine.

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8478588 [details] [diff] [review]
Part 2 : stylesheet reloading v3

I'm happy if the other reviewers are.

(Though perhaps I'd rather have invested the effort on avoiding swapping the representation of a URL and using something redirect-like than fixing the stylesheet swapping, since it seems like it has a bigger effect on the API surface we expose to themes, and is thus more likely to give us compatibility problems.  Though I guess the issue being fixed here affects more the API exposed to apps being themed.)

Comment 73

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : stylesheet reloading v3

Minor update fixing a debug build error.

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8478825 [details] [diff] [review]
Part 2 : stylesheet reloading v3

>+++ b/content/base/src/nsDocument.cpp
>+using mozilla::CSSStyleSheet;

Why is this needed, given the "using namespace mozilla;" two lines above?

>@@ -1567,16 +1569,22 @@ nsDocument::nsDocument(const char* aCont
>+    os->AddObserver(this, "app-theme-changed", /* ownsWeak */ false);

This is ... rather questionable, given that "this" has a refcount of 0 at this point.  You're relying on AddObserver not addrefing it and then releasing it.

>@@ -1729,16 +1737,21 @@ nsDocument::~nsDocument()
>+    os->RemoveObserver(this, "app-theme-changed");

This is dead code: since the observer service holds a reference to us, ~nsDocument can't get called.

As in, this patch leaks all documents forever, unless I'm really missing something.

We need a better place to add/remove the observer.

Do we care about this for SVG image documents?  Or just for documents loaded in iframes?

>+nsDocument::OnAppThemeChanged()
>+  // Bail out if there is no theme support setup properly.

"set up"

>+    nsCOMPtr<nsIDOMNode> owningNode;
>+    sheet->GetOwnerNode(getter_AddRefs(owningNode));

  nsINode* owningNode = sheet->GetOwnerNode();

>+    // Get a DOM stylesheet link to check the href against the theme origin.
>+    nsCOMPtr<nsIDOMHTMLLinkElement> hLink = do_QueryInterface(owningNode);
>+    if (!hLink) {
>+      continue;
>+    }
>+    nsString href;
>+    hLink->GetHref(href);
>+    nsCOMPtr<nsIURI> uri;
>+    NS_NewURI(getter_AddRefs(uri), href);

How about:

  nsIURI* sheetURI = sheet->GetOriginalURI();
  if (!sheetURI) {
    continue;
  }

>+    nsAutoCString origin;
>+    nsPrincipal::GetOriginForURI(uri, getter_Copies(origin));
>+    if (!origin.Equals(themeOrigin)) {

Could we not just use nsContentUtils::GetUTFOrigin?  For our purposes here, it should be equivalent to GetOriginForURI, and not involve black magic like the latter.

>+++ b/content/base/src/nsStyleLinkElement.cpp
>       LoadStyleLink(thisContent, clonedURI, title, media, isAlternate,
>-                    GetCORSMode(), aObserver, &isAlternate);
>+                    GetCORSMode(), aObserver, &isAlternate, aForceUpdate);

No, this is wrong.  It'll mess up the existing callers that pass a true aForceUpdate to this method.  The meaning of aForceUpdate is "skip checking whether we already have this sheet on this element", not "ignore things in the CSS loader".

Again, what you want to do is explicitly remove the relevant hashtable entries from the CSS loader in nsDocument::OnAppThemeChanged, before you start reloading the sheets.

Comment 75

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : stylesheet reloading v4

Comment 76

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8479393 [details] [diff] [review]
Part 2 : stylesheet reloading v4

>+++ b/content/base/src/nsDocument.cpp
>+  if (!mScriptGlobalObject && aScriptGlobalObject) {

Why not just put the AddObserver call in the existing |if (aScriptGlobalObject)| block?

>+++ b/content/base/src/nsStyleLinkElement.cpp
>+    nsCOMPtr<nsIDocument> doc = thisContent->GetCrossShadowCurrentDoc();

Why not just nsIDocument*?

>+      doc->CSSLoader()->ObsoleteSheet(mStyleSheet->GetOriginalURI());

mStyleSheet might be null.  Even if it's not, mStyleSheet->GetOriginalURI() might be null.  Please null-check both before making this call.

r=me with those issues fixed.  Thanks for doing this!

Comment 77

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8479393 [details] [diff] [review]
Part 2 : stylesheet reloading v4

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

The imgLoader part looks good! I didn't look at the rest of the patch, but it seems to be in good hands.

Comment 78

[Johan Lorenzo [:jlorenzo]]

In 2.1, we'll only support Tako themes. There is no need to create sample themes for now.
As we don't have any screenshots about these themes, we're not able to create a suite like: https://moztrap.mozilla.org/manage/cases/?filter-suite=664.

We'll check the theme with the smoketests suite then.

Comment 79

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/53a548277a88
https://hg.mozilla.org/integration/b2g-inbound/rev/7c682fb2f547
https://hg.mozilla.org/integration/b2g-inbound/rev/cc01e00b86a5
https://hg.mozilla.org/integration/b2g-inbound/rev/c87e98b19ffa
https://hg.mozilla.org/integration/b2g-inbound/rev/0ec69047db59

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53a548277a88
https://hg.mozilla.org/mozilla-central/rev/7c682fb2f547
https://hg.mozilla.org/mozilla-central/rev/cc01e00b86a5
https://hg.mozilla.org/mozilla-central/rev/c87e98b19ffa
https://hg.mozilla.org/mozilla-central/rev/0ec69047db59

Comment 81

[Jason Smith [:jsmith]]

No QA verification for the same reasons called out in comment 78, so marking verified.

Comment 82

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8479393 [details] [diff] [review]
Part 2 : stylesheet reloading v4

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

The imgLoader part looks good! I didn't look at the rest of the patch, but it seems to be in good hands.

::: image/src/imgLoader.cpp
@@ +858,5 @@
>  NS_IMETHODIMP
>  imgCacheObserver::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aSomeData)
>  {
> +  if (strcmp(aTopic, "memory-pressure") == 0 ||
> +      strcmp(aTopic, "app-theme-changed") == 0) {

I really apologize for somehow not comprehending this when I originally reviewed this patch. This is 100% my bad; I should have spotted this. This *does not* achieve the goal you want to achieve.

You need to flush the cache, which removes the source data, not trigger a discard, which destroys the decoded data but keeps the same (presumably now wrong) source data around.

Did this somehow work for you? I don't really see how it could have...

I'll file a followup to reimplement this properly, as penance for wrongly r+'ing it. =)

Comment 83

[Seth Fowler [:seth] [:s2h]]

So I see that imgLoader::Observe clears the image cache when it receives "app-theme-changed". That's why this appeared to work - imgLoader::Observe took care of it for you. (imgLoader::Observer doesn't clear the chrome image cache, though. Hopefully you don't need that.)

So I guess no followup is needed, except to remove this unnecessary code...

Comment 84

[[:fabrice] Fabrice Desré]

That's correct, we don't need (and don't want) to clear the chrome image cache.