Closed Bug 1470842 Opened 6 years ago Closed 6 years ago

Add "widgets.css" as an author stylesheet for migrating "components.css"

Categories

(Toolkit :: Themes, enhancement, P1)

Product:
Toolkit
Component:
Themes
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(2 files)

Bug 1470842 - Add "widgets.css" as an author stylesheet for migrating "components.css".
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Assert global.css is loaded.
3.62 KB, patch
Details | Diff | Splinter Review 
This was originally part of the build in bug 1463820, and is a prerequisite to the migration work tracked by bug 1470830.
Talos and screenshots from bug 1463820 comment 31 look good, just running regular tests now in case this trips something else like unreferenced file checks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22ea361ee22fb1e66f6913f2947aed5c68dec2a5
Comment on attachment 8987455 [details]
Bug 1470842 - Add "widgets.css" as an author stylesheet for migrating "components.css".

https://reviewboard.mozilla.org/r/252696/#review259198

::: toolkit/content/widgets.css:6
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ===== widgets.css =====================================================
> +   == Styles ported from XBL <resources>, loaded in every XUL document.

Note this isn't technically true - with the patch as-is the sheet is only ever loaded in XUL documents that import global.css - which I learned isn't every document:
- about:config didn't used to load it as per Bug 1420166)
- various test documents don't load it

If we want it to *actually* load everywhere then it would probably make sense to load it in nsDocumentViewer when we import other sheets. Note that this causes an assertion "Style set already has document sheets?" at  https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/base/nsDocument.cpp#2521. So alternatively the sheet could maybe be added in that function instead (or the assertion could be changed) - Emilio should know if that would make sense.

If we are thinking to do that anyway, it may make more sense to actually load global.css everywhere (and stick it in the nsLayoutStylesheetCache instead of widgets.css) then continue to @import widget.css from global.css. That would let us remove a ton of explicit global.css links / xml stylesheets through the tree and also make sure we don't end up in weird cases where one is loaded but not the other.
Attachment #8987455 - Flags: review?(emilio)
Actually, the review request is more of a needinfo at this point.
Flags: needinfo?(emilio)
Attachment #8987455 - Flags: review?(emilio)
If that's possible, loading the stylesheet without having to import it explicitly makes sense to me, just like we do for the Custom Elements scripts. We may have to remove all the references around the tree to avoid loading it twice, for example:

https://dxr.mozilla.org/mozilla-central/search?q=global%2Fskin%2F"
Loading them in nsDocumentViewer is not great. Document sheets are assumed to also be in nsIDocument::mStyleSheets. We do support such sheets without nodes (like the ones that come from Link: headers). It's not clear how that interacts with this sort of hardcoded sheet. I guess it doesn't really matter as long as it's chrome-only.

Is this worth the convenience? I'm not convinced it would but icbw.
Flags: needinfo?(emilio)
Comment on attachment 8987455 [details]
Bug 1470842 - Add "widgets.css" as an author stylesheet for migrating "components.css".

https://reviewboard.mozilla.org/r/252696/#review259310

::: layout/style/nsLayoutStylesheetCache.cpp:352
(Diff revision 1)
>                 &mSVGSheet, eAgentSheetFeatures, eCrash);
>    if (XRE_IsParentProcess()) {
>      // We know we need xul.css for the UI, so load that now too:
>      XULSheet();
>      XULComponentsSheet();
> +    XULWidgetsSheet();

Why is this needed at all? This is an author sheet, we don't need to know about it from C++, right?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Loading them in nsDocumentViewer is not great. Document sheets are assumed
> to also be in nsIDocument::mStyleSheets. We do support such sheets without
> nodes (like the ones that come from Link: headers). It's not clear how that
> interacts with this sort of hardcoded sheet. I guess it doesn't really
> matter as long as it's chrome-only.
> 
> Is this worth the convenience? I'm not convinced it would but icbw.

I thought there could also be a perf win by including it in nsLayoutStylesheetCache - is that true, and if so is that independent of whether we load the sheet via xml stylesheet or through nsDocumentViewer?
(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> > Loading them in nsDocumentViewer is not great. Document sheets are assumed
> > to also be in nsIDocument::mStyleSheets. We do support such sheets without
> > nodes (like the ones that come from Link: headers). It's not clear how that
> > interacts with this sort of hardcoded sheet. I guess it doesn't really
> > matter as long as it's chrome-only.
> > 
> > Is this worth the convenience? I'm not convinced it would but icbw.
> 
> I thought there could also be a perf win by including it in
> nsLayoutStylesheetCache - is that true, and if so is that independent of
> whether we load the sheet via xml stylesheet or through nsDocumentViewer?

Putting it in nsLayoutStylesheetCache would only do something useful if you actually load it only from C++, otherwise you'd just get a fresh copy, since we don't look at that cache from the CSS loader. We do look at other cache though, at the XUL prototype cache[1]. The perf difference between nsLayoutStylesheetCache vs. just loading the stylesheet via XML (that is, accounting for the XUL prototype cache) should be nothing, or close to nothing.

But nsLayoutStylesheetCache + loading the stylesheet from XML (what this patch is doing) you'd get one copy on each cache, which would potentially be a regression.
Comment on attachment 8987455 [details]
Bug 1470842 - Add "widgets.css" as an author stylesheet for migrating "components.css".

https://reviewboard.mozilla.org/r/252696/#review259320

OK. While I still think it would be more convenient and probably a good idea to make global.css load in all XUL docs (given that we don't consistently load it everywhere), that seems like a separate discussion. The smoothest path forward seems like reverting any C++ changes to this patch and and load widgets.css directly from global.css (updating the header comment in widgets.css to reflect that it's loaded as an author sheet anywhere global.css is loaded).
Attachment #8987455 - Flags: review?(bgrinstead) → review-
(In reply to Brian Grinstead [:bgrins] from comment #11)
> The smoothest
> path forward seems like reverting any C++ changes to this patch and and load
> widgets.css directly from global.css (updating the header comment in
> widgets.css to reflect that it's loaded as an author sheet anywhere
> global.css is loaded).

Didn't this cause a Talos regression when you tried it in bug 1463820?

https://reviewboard.mozilla.org/r/246612/diff/1/
Flags: needinfo?(bgrinstead)
(In reply to :Paolo Amadini from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > The smoothest
> > path forward seems like reverting any C++ changes to this patch and and load
> > widgets.css directly from global.css (updating the header comment in
> > widgets.css to reflect that it's loaded as an author sheet anywhere
> > global.css is loaded).
> 
> Didn't this cause a Talos regression when you tried it in bug 1463820?
> 
> https://reviewboard.mozilla.org/r/246612/diff/1/

There were two changes in between the patch with the regressions and the one without.

1) Originally I was doing `@import("menu.css") @import("splitter.css") ... etc` in global.css and the new version did just @import("components.css") from global.css and did the `@import("menu.css") @import("splitter.css") ... etc` from there
2) Continued to load components.css through nsLayoutStylesheetCache (although not in nsDocumentViewer).

Based on Comment 9, (2) shouldn't have helped anything - so if it does we'll need to investigate why.

So it was either (1) - which is what your patch does anyway, or there was some weird noise with the talos tests or a change on the platform in between the versions.

Can you make a test build that moves most/all of the XBL resources into widgets.css (similar to https://reviewboard.mozilla.org/r/246196/diff/5#index_header) and confirm we don't see big regressions?
Flags: needinfo?(bgrinstead)
You could also do a talos push with the c++ changes reverted to compare to one with the C++ changes to isolate and confirm it isn't helping. Make sure both are full builds though - comparing artifact and non-artifact builds on talos doesn't work right.
The try run is not complete yet, but I do already see some regressions on Linux comparing the version with the C++ changes to the one without them:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c63266f8af0d413d5448e988c4f8b5ccb4c01ec2&newProject=try&newRevision=cee9be5acb577d3876f2c4dc35d5452bf3020115&framework=1&showOnlyImportant=1

As always, it's difficult to be confident in the Talos results. Do you think these are real regressions?
Flags: needinfo?(emilio)
Flags: needinfo?(bgrinstead)
On the other hand, I don't see the same regressions when comparing the base revision with the one without the cache, so it might be noise...

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=51fbeda4c7c325d42e8066197d5d2a6f4b7f6bdc&newProject=try&newRevision=cee9be5acb577d3876f2c4dc35d5452bf3020115&framework=1
(In reply to :Paolo Amadini from comment #17)
> On the other hand, I don't see the same regressions when comparing the base
> revision with the one without the cache, so it might be noise...
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=51fbeda4c7c325d42e8066197d5d2a6f
> 4b7f6bdc&newProject=try&newRevision=cee9be5acb577d3876f2c4dc35d5452bf3020115&
> framework=1

Yeah if this comparison ends up looking good then I think we are good to go without the style cache. If we don't have any problems with this one then I wouldn't expect issues when migrating individual sheets.
Flags: needinfo?(bgrinstead)
I agree with comment 18 :)
Flags: needinfo?(emilio)
Comment on attachment 8987455 [details]
Bug 1470842 - Add "widgets.css" as an author stylesheet for migrating "components.css".

https://reviewboard.mozilla.org/r/252696/#review260060
Attachment #8987455 - Flags: review?(bgrinstead) → review+
Would it be relatively easy to assert any time a XUL document gets loaded without global.css present? I'm thinking we should get a bug filed to make sure every doc in tree loads global.css since we are going to be assuming it's always loaded.

We may as well also unify how they are referenced while we are at it:

- "chrome://global/skin/" Number of results: 254 - https://searchfox.org/mozilla-central/search?q=%22chrome%3A%2F%2Fglobal%2Fskin%2Fglobal.css%22&path=
- "chrome://global/skin/global.css" Number of results: 39 - https://searchfox.org/mozilla-central/search?q=%22chrome%3A%2F%2Fglobal%2Fskin%2Fglobal.css%22&path=
Flags: needinfo?(emilio)
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8b5b48b0cb
Add "widgets.css" as an author stylesheet for migrating "components.css". r=bgrins
Sure, something like this should do.

chrome://extensions/content/dummy.xul doesn't load it, from a quick test :)
Flags: needinfo?(emilio)
See Also: → 1471667
https://hg.mozilla.org/mozilla-central/rev/fc8b5b48b0cb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Emilio, is that something you intend to uplift to beta or can we let it ride the trains and update the tracking flags accordingly. Thanks
Flags: needinfo?(emilio)
301 Paolo, which is the one who fixed it. I just added another patch that I got asked for, is debug-only, and which got moved to a different bug :)
Flags: needinfo?(emilio) → needinfo?(paolo.mozmail)
No uplift needed, this is new work. Thanks for the reminder!
status-firefox62: affected → wontfix
Flags: needinfo?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: