1472616 - Import all the static browser stylesheets from "browser-window.css"

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[:Paolo Amadini]

This is required to fix the failure in bug 1421433.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1472616 - Import all the static browser stylesheets from "browser-window.css".

This prevents a specific failure in the "browser_startup_flicker.js" test on Windows 10 only, surfaced by bug 1421433. The compact theme stylesheet is kept separate because it can be disabled independently.

Review commit: https://reviewboard.mozilla.org/r/254186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254186/

Comment 2

[:Paolo Amadini]

Screenshots on the combined patches:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=b6a7fd6c065a8b984c48491136e29078649fbeab&newProject=try&newRev=1a3ac44fd8d9330c2c89b58a61bfd9c64a45de81

The only difference I noticed is that the favicon for the pinned home tab sometimes is loaded in the new version when it wasn't before. This is the expected behavior when running outside of MozScreenshots, so it may just be an intermittent.

Comment 3

[Dão Gottwald [:dao]]

It's not clear to me that this is the right solution to the failure. What exactly is causing the failure in the first place?

Comment 4

[:Paolo Amadini]

Yeah, given what Florian said in bug 1421433 comment 14 and 16, it may be some sort of timing issue. Since I can't reproduce this locally even on Windows 10, it's going to be hard to debug.

I've already spent a day to set up the build on Windows 10 to no avail, and I'd like to start landing the CSS changes so we can catch regressions earlier. Dão, Florian, as an alternative to landing this patch, would you be fine with disabling the test on Windows 10 for now?

Comment 5

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #4)

> Florian, as an alternative to landing this patch, would you be
> fine with disabling the test on Windows 10 for now?

No, losing startup flicker coverage for the platform where we have the most users isn't an option, sorry.

Comment 6

[:Paolo Amadini]

The most effective alternative I see is then to land this patch, because it solves the issue, it causes no performance regression, and unblocks a multitude of CSS updates that we have to land near the beginning of the cycle.

I can file a bug to record that the issue exists and have it investigated, but I don't think I'll have the time to do it myself.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #6)
> The most effective alternative I see is then to land this patch, because it
> solves the issue, it causes no performance regression, and unblocks a
> multitude of CSS updates that we have to land near the beginning of the
> cycle.
> 
> I can file a bug to record that the issue exists and have it investigated,
> but I don't think I'll have the time to do it myself.

Well, just having the bug filed isn't going to help much, somebody will need to work on it. Not being allowed to use a standard way to link stylesheets, with no clear understanding why, is no good place to be in especially with a multitude of CSS updates coming.

Comment 8

[:Paolo Amadini]

(In reply to Dão Gottwald [::dao] from comment #7)
> Not being allowed to use a standard way to link stylesheets,
> with no clear understanding why, is no good place to be

I share this concern, actually. The reality is that we are already in this situation, and the fact we've been delayed is a proof of how this is no good place to be. The workaround in this bug, however, doesn't make things better or worse in this regard.

With or without this patch, there is a possibility that any upcoming CSS change could trigger the test failure. In this light, I agree we should start working on this issue soon. This is a problem that may hopefully have an easy solution, but may also require weeks to figure out. I've already spent days to set up a Windows 10 test environment to no avail, since I can still not reproduce the failure locally.

Maybe we can parallelize the work here, so I could focus on getting the CSS changes in the tree while someone else investigates the failure. Mike, do you think someone from the Performance team could start looking into this startup flicker test issue?

Dão, if someone was assigned to work on the test issue, would you be fine with landing this workaround in parallel to unblock the CSS changes?

Comment 9

[Dão Gottwald [:dao]]

> Dão, if someone was assigned to work on the test issue, would you be fine
> with landing this workaround in parallel to unblock the CSS changes?

Yep.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

I'll be investigating this in bug 1473034.

Comment 11

[:Paolo Amadini]

Thank you Mike! That really helps.

Try for browser-chrome already looks good, also with the patches in the dependent bugs:

Bug 1472616: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f276e112c669848d04fecfdef310f86fe74b340
Bug 1421433: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c76ad3868b620c37755176055f75ef0cd7707e7e
Bug 1472153: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56fd3c31884427e1b9a81f60575566eae2309cf2

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8989115 [details]
Bug 1472616 - Import all the static browser stylesheets from "browser-window.css".

https://reviewboard.mozilla.org/r/254186/#review261576

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17718af0d6a8
Import all the static browser stylesheets from "browser-window.css". r=dao

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/17718af0d6a8