Closed Bug 1472616 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

This is required to fix the failure in bug 1421433.
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.
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?
Flags: needinfo?(paolo.mozmail)
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?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(florian)
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(florian)
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.
(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.
Flags: needinfo?(dao+bmo)
(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?
Flags: needinfo?(mconley)
> 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.
Depends on: 1473034
I'll be investigating this in bug 1473034.
Flags: needinfo?(mconley)
Comment on attachment 8989115 [details]
Bug 1472616 - Import all the static browser stylesheets from "browser-window.css".

https://reviewboard.mozilla.org/r/254186/#review261576
Attachment #8989115 - Flags: review?(dao+bmo) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17718af0d6a8
Import all the static browser stylesheets from "browser-window.css". r=dao
https://hg.mozilla.org/mozilla-central/rev/17718af0d6a8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1473598
You need to log in before you can comment on or make changes to this bug.