1208759 - Stop preprocessing mobile/android/themes

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Nick Alexander :nalexander [he/him]]

Bob Dylan famously sang, "The times, they are a changin'."  Indeed, the CSS times have changed: CSS has grown features that should allow us to stop preprocessing mobile/android/themes!  Huzzah!

Right now we have a defines.inc included in several CSS files, and we then use @variable@ to text substitute.  Let's do that in CSS.

Part 1: The actual number of consumed variables is low; we can purge most of defines.inc before moving forward.

Part 2: Then we can convert the defines.inc into a regular CSS file and use :root { --variable: value } to set the variables.  See https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables, and the existing examples in tree.  I see some non-standard "mozmm" units; I hope those can all be purged in Part 1.

Part 3: Then we @import the new defines.css and update all consumers to use var(--variable) in the consuming CSS.  See https://developer.mozilla.org/en/docs/Web/CSS/@import.

Part 4: test and profit!

Comment 1

[Vivek Balakrishnan[:vivek]]

Hi Kalpesh,

Are you in getting your hands dirty with some CSS stuff.
Please let me know if you prefer some Java tasks

Comment 2

[Kalpesh Krishna [:martianwars]]

Yeah I'll be interested, though I'll prefer java stuff. Where do I start?

Comment 3

[Vivek Balakrishnan[:vivek]]

Please see the bug description on how to solve this.
Feel free to NI me if you need some help.

Thanks for helping with this Kalpesh.

Comment 4

[Kalpesh Krishna [:martianwars]]

Well I'll go in the order Nick says. If I'm not wrong, I have to remove the unused variables in defines.inc right?

Comment 5

[Vivek Balakrishnan[:vivek]]

Yes. you can use http://mxr.mozilla.org/mozilla-central/ or "search in Path" feature in your favorite IDE to find used reference and then prune those that are not used.

Comment 6

[Kalpesh Krishna [:martianwars]]

Alright I'll work on this bug :)

Comment 7

[Kalpesh Krishna [:martianwars]]

Attachment: cssfix.patch

It was quite cumbersome. I hope its correct

Comment 8

[Kalpesh Krishna [:martianwars]]

How do I generate the correct command here ;- http://trychooser.pub.build.mozilla.org/ for this case?
I find it quite confusing.

Comment 9

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8685549 [details] [diff] [review]
cssfix.patch

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

@Kalpesh,

Thanks for going through the unused definitions and cleaning it up.
Can you start working on the 'part 2' mentioned in bug description which involves converting defines.inc to a css file.
While you are at it, use mm instead mozmm for the units.

Comment 10

[Kalpesh Krishna [:martianwars]]

Attachment: cssfix.patch

Hope this solves part 2. 
@vivek,
I wasn't very sure about how to work on the "filter" field so left it as a property. Do let me know if it needs to be changed. Also, could you answer my question regarding the try server in my previous comment?

Comment 11

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8687407 [details] [diff] [review]
cssfix.patch

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

Thanks Kalpesh,

That looks good. 
I am not sure about the filter definition but I am guessing that we can safely skip that.
Can you continue along and work on Part 3 of bug description?

As for as the try server, I usually select all the unit tests under Android.
For this case, I am not sure what is the right try configuration and unfortunately, this is not well documented.

::: mobile/android/themes/core/defines.css
@@ +20,2 @@
>  
> +	--margin_snormal: 0.64mm;	

nit: trailing space

Comment 12

[Vivek Balakrishnan[:vivek]]

@Nick: I assumed that we can safely skip the filter definition in the above review. 
Am I correct in my assumption. 
Also, can you please point me to some docs if any related to this.

Comment 13

[Kalpesh Krishna [:martianwars]]

Attachment: cssfix.patch

Done with part 3, do let me know the errors

Comment 14

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/2eef9ad06663483702a440eb62f131aea8fbe796
Bug 1208759 - Stop preprocessing mobile/android/themes. r=nalexander

Comment 15

[Nick Alexander :nalexander [he/him]]

There were some significant issues with this patch, which I addressed and pushed.  I think I've addressed these, and I tested about:logins (the input edit box, especially) and about:addons to make sure they looked reasonable.  

Kalpesh, thanks for digging into this issue, but your testing of this wasn't sufficient: it didn't compile (due to includes that needed to be removed), and as written, it didn't work (due to errors in the defines.css syntax).  You could compare the patch I pushed to the patch you uploaded to see the issues (some in jar.mn, some in the actual code).

Comment 16

[Kalpesh Krishna [:martianwars]]

@nalexander :- I guess I completely messed up the filter field. Wasn't too sure about that :( I'll try to take more care next time.

@vivek, @nalexander :- What could I work on next?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2eef9ad06663

Comment 18

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8687718 [details] [diff] [review]
cssfix.patch

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

Already landed

Comment 19

[Emilio Cobos Álvarez (:emilio)]

This change was pretty wrong... These things are exposed to content, see bug 1470709 for an example.

Why was this done without any kind of style system peer review? Who owns these stylesheets?

Comment 20

[Nick Alexander :nalexander [he/him]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)
> This change was pretty wrong... These things are exposed to content, see bug
> 1470709 for an example.

I had no idea.  That's pretty counter-intuitive.

> Why was this done without any kind of style system peer review?

Because this is really counter-intuitive?

 Who owns
> these stylesheets?

At this point, nobody, really.  sdaswani, I guess (Fennec engineering manager).  Do whatever is necessary to fix this obviously busted situation, preferably without re-introducing pre-processing to the build system.