Stop preprocessing mobile/android/themes

RESOLVED FIXED in Firefox 45

Status

()

Firefox for Android
Build Config & IDE Support
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nalexander, Assigned: martianwars, Mentored)

Tracking

Trunk
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

(Whiteboard: [lang=css][good next bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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!
Hi Kalpesh,

Are you in getting your hands dirty with some CSS stuff.
Please let me know if you prefer some Java tasks
Flags: needinfo?(kalpeshk2011)
(Assignee)

Comment 2

3 years ago
Yeah I'll be interested, though I'll prefer java stuff. Where do I start?
Flags: needinfo?(kalpeshk2011) → needinfo?(vivekb.balakrishnan)
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.
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 4

3 years ago
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?
Flags: needinfo?(vivekb.balakrishnan)
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.
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 6

3 years ago
Alright I'll work on this bug :)
(Assignee)

Updated

3 years ago
Assignee: nobody → kalpeshk2011
(Assignee)

Comment 7

3 years ago
Created attachment 8685549 [details] [diff] [review]
cssfix.patch

It was quite cumbersome. I hope its correct
Attachment #8685549 - Flags: review?(vivekb.balakrishnan)
(Assignee)

Comment 8

3 years ago
How do I generate the correct command here ;- http://trychooser.pub.build.mozilla.org/ for this case?
I find it quite confusing.
Flags: needinfo?(vivekb.balakrishnan)
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.
Attachment #8685549 - Flags: review?(vivekb.balakrishnan) → a11y-review+
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8685549 - Flags: a11y-review+ → feedback+
(Assignee)

Comment 10

3 years ago
Created attachment 8687407 [details] [diff] [review]
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?
Attachment #8685549 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8687407 - Flags: review?(vivekb.balakrishnan)
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
Attachment #8687407 - Flags: review?(vivekb.balakrishnan) → feedback+
@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.
Flags: needinfo?(vivekb.balakrishnan) → needinfo?(nalexander)
(Assignee)

Comment 13

3 years ago
Created attachment 8687718 [details] [diff] [review]
cssfix.patch

Done with part 3, do let me know the errors
Attachment #8687407 - Attachment is obsolete: true
Attachment #8687718 - Flags: review?(vivekb.balakrishnan)
(Reporter)

Comment 15

3 years ago
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).
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
(Assignee)

Comment 16

3 years ago
@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?
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2eef9ad06663
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
Comment on attachment 8687718 [details] [diff] [review]
cssfix.patch

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

Already landed
Attachment #8687718 - Flags: review?(vivekb.balakrishnan)
You need to log in before you can comment on or make changes to this bug.