Closed
Bug 1446831
Opened 6 years ago
Closed 6 years ago
Move label, description, and other text CSS rules out of minimal-xul.css and into xul.css
Categories
(Core :: XUL, enhancement, P5)
Core
XUL
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)
59 bytes,
text/x-review-board-request
|
Details | |
3.88 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Priority: -- → P5
Reporter | ||
Updated•6 years ago
|
Blocks: memshrink-content
Comment 2•6 years ago
|
||
Brian, what kind of overhead do you think this has per process?
Flags: needinfo?(bgrinstead)
Whiteboard: [overhead:?]
Reporter | ||
Comment 3•6 years 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•6 years 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)
Comment 6•6 years ago
|
||
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)
Comment 7•6 years ago
|
||
(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•6 years 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•6 years 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•6 years ago
|
||
updated Brian's patch (make tests pass)
Attachment #9007997 -
Flags: review?(bugs)
Reporter | ||
Comment 11•6 years 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 12•6 years ago
|
||
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•6 years 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•6 years ago
|
||
Assignee: nobody → surkov.alexander
Attachment #9008269 -
Flags: review?(bugs)
Assignee | ||
Updated•6 years ago
|
Attachment #9007997 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9008269 -
Flags: review?(bugs) → review+
Comment 15•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9df80bf2bbd
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•