Move label, description, and other text CSS rules out of minimal-xul.css and into xul.css

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: bgrins, Assigned: surkov)

Tracking

(Blocks 3 bugs)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [overhead:3K])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
There are rules in minimal-xul.css (loaded in every content document) that I think can move into xul.css once the elements aren't used anymore by touchControls and the file picker.

Updated

a year ago
Priority: -- → P5
(Reporter)

Updated

10 months ago
Brian, what kind of overhead do you think this has per process?
Flags: needinfo?(bgrinstead)
Whiteboard: [overhead:?]
(Reporter)

Comment 3

10 months ago
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #2)
> Brian, what kind of overhead do you think this has per process?

Probably not much, but I have no idea. Maybe Emilio could estimate. Something else missing from the attached patch is that we can remove the html namespace from the file since the only time it's used is for `label html|span.accesskey`.
Flags: needinfo?(bgrinstead) → needinfo?(emilio)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

10 months ago
(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #2)
> > Brian, what kind of overhead do you think this has per process?
> 
> Probably not much, but I have no idea. Maybe Emilio could estimate.
> Something else missing from the attached patch is that we can remove the
> html namespace from the file since the only time it's used is for `label
> html|span.accesskey`.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1474793#c8 - the style sheet sizes for the sheets loaded in a fresh content process that only loaded about:blank:

14704 chrome://global/content/minimal-xul.css

Cam, how did you take that measurement? I'd be curious what it is with this patch applied.
Flags: needinfo?(emilio) → needinfo?(cam)
I applied this patch, then took an about:memory measurement:

diff --git a/layout/style/StyleSheet.cpp b/layout/style/StyleSheet.cpp
index 3ad1b99..e36558f 100644
--- a/layout/style/StyleSheet.cpp
+++ b/layout/style/StyleSheet.cpp
@@ -344,10 +344,15 @@ size_t
 StyleSheetInfo::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
 {
   size_t n = aMallocSizeOf(this);
-  n += Servo_StyleSheet_SizeOfIncludingThis(
+  size_t s =
+       Servo_StyleSheet_SizeOfIncludingThis(
       ServoStyleSheetMallocSizeOf,
       ServoStyleSheetMallocEnclosingSizeOf,
       mContents);
+  nsCString url;
+  mOriginalSheetURI->GetSpec(url);
+  printf("[%d] %d %s\n", (int)getpid(), (int)s, url.get());
+  n += s;
   return n;
 }
 
With your patch applied, I get:

11840 chrome://global/content/minimal-xul.css
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #6) 
> With your patch applied, I get:
> 
> 11840 chrome://global/content/minimal-xul.css

Kind of impressive given the size of the patch.
Whiteboard: [overhead:?] → [overhead:3K]
(Reporter)

Comment 8

10 months ago
Do we know what the overhead to loading an empty UA sheet is? Assuming that's non-zero: I've also thought about deleting minimal-xul.css altogether and splitting any relevant rules up into html.css / xul.css (duplicating any that are needed in each environment). For instance, AFAICT we never use <resizer> in the chrome UI so that could move directly to html.css. Others could be copy/pasted into both files.
(Reporter)

Comment 9

10 months ago
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Do we know what the overhead to loading an empty UA sheet is?

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1474793#c8, I guess noscript.css is about as close as we'd get to that, and  it's only 400 bytes. So probably not worth the maintenance cost to duplicate selectors.

Tracking work to remove more in-content XUL, and so hopefully more stuff out of minimal-xul.css in Bug 1446829.
(Assignee)

Comment 10

7 months ago
Posted patch patch (obsolete) — Splinter Review
updated Brian's patch (make tests pass)
Attachment #9007997 - Flags: review?(bugs)
(Reporter)

Updated

7 months ago
Depends on: 1488620
(Reporter)

Comment 11

7 months ago
Comment on attachment 9007997 [details] [diff] [review]
patch

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

::: toolkit/content/minimal-xul.css
@@ -28,4 @@
>    box-sizing: border-box;
>  }
>  
> -:root {

Post Bug 1488620 but pre Bug 1446830, I'm pretty sure we will still need these rules, right? In that case we will still be using a XUL label but not the XBL binding for it.
Comment on attachment 9007997 [details] [diff] [review]
patch

based on brian's comment, this needs some changes still
Attachment #9007997 - Flags: review?(bugs)
(Assignee)

Comment 13

7 months ago
(In reply to Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 9007997 [details] [diff] [review]

> > -:root {
> 
> Post Bug 1488620 but pre Bug 1446830, I'm pretty sure we will still need
> these rules, right? In that case we will still be using a XUL label but not
> the XBL binding for it.

oh, if this is style is required for xul:label rendering, then for sure. I will remove it. Should I file a follow up bug to keep this issue on track or is it better to add a comment to bug 1446830?
(Assignee)

Comment 14

7 months ago
Posted patch patchSplinter Review
Assignee: nobody → surkov.alexander
Attachment #9008269 - Flags: review?(bugs)
(Assignee)

Updated

7 months ago
Attachment #9007997 - Attachment is obsolete: true

Updated

7 months ago
Attachment #9008269 - Flags: review?(bugs) → review+

Comment 15

7 months ago
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9df80bf2bbd
move label, description css rules out of minimal-xul.css, patch=bgrins, surkov, r=smaug

Comment 16

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9df80bf2bbd
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(Assignee)

Updated

7 months ago
Blocks: 1491762
You need to log in before you can comment on or make changes to this bug.