Closed Bug 1446831 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: XUL, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bgrins, Assigned: surkov)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [overhead:3K])

Attachments

(2 files, 1 obsolete file)

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.
Priority: -- → P5
Brian, what kind of overhead do you think this has per process?
Flags: needinfo?(bgrinstead)
Whiteboard: [overhead:?]
(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)
(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]
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.
(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.
Attached patch patch (obsolete) — Splinter Review
updated Brian's patch (make tests pass)
Attachment #9007997 - Flags: review?(bugs)
Depends on: 1488620
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)
(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?
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Attachment #9008269 - Flags: review?(bugs)
Attachment #9007997 - Attachment is obsolete: true
Attachment #9008269 - Flags: review?(bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/f9df80bf2bbd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Blocks: 1491762
You need to log in before you can comment on or make changes to this bug.