1820549 - Remove flicker exception for Firefox View button in browser_tabclose.js and other performance tests

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P2)

Description

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

Adding an exception to the browser_tabclose.js test. Looking at the two frames showing the flicker (attached), this appears to be from the Firefox View icon. The regressor, bug 1815229, changed some image load timings. It's strange how this is only failing on beta, but given how close we are to the merge, I landed a patch in bug 1819084 to add an exception and filed this bug to investigate a proper fix.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

This is not about image load timings, see the last patch in the regressing bug.

This is expected because the Firefox View icon is an asynchronously decoded PNG, and nsImageBoxFrame used to eagerly decode all images. Now <xul:image> is only decoded when visible, but the async-ness of this is what that test reveals.

Comment 2

[:Gijs (he/him)]

I still think we would like to avoid this for both this and any other toolbar buttons we would want to convert to HTML img instead of XUL. Emilio, is there currently a primitive for this, for chrome code? And/or, is there a principled reason that we wouldn't want to do this? I Understand it would force some things to be sync that aren't right now - but then, they've always worked that way for the browser window so I can't imagine it'd make things any worse than they are today?

From the frontend perspective, these tests exist because ideally we want to paint the complete window in 1 go, instead of having things pop into existence (especially where that causes layout to adjust sizes, which I think isn't the case here). Maybe Florian can say more about this and/or how we'd feel about loading all icons async instead of sync...

Comment 3

[Emilio Cobos Álvarez [:emilio]]

So load timing (and thus sizing etc) is the same. What causes this is async-decoding (which doesn't block the load event).

We could either use <img decoding="sync"> (which would mean losing the ability of setting the icon via CSS, you'd need to use src or srcset or so), or we could add some sort of internal property a la -moz-force-broken-image-icon which controlled this via CSS or something.

Comment 4

[Florian Quèze [:florian]]

(In reply to :Gijs (he/him) from comment #2)

From the frontend perspective, these tests exist because ideally we want to paint the complete window in 1 go, instead of having things pop into existence (especially where that causes layout to adjust sizes, which I think isn't the case here). Maybe Florian can say more about this and/or how we'd feel about loading all icons async instead of sync...

Agreed with what you wrote. From a perceived performance perspective, having things that move around while loading is the worst. Having things that appear (directly at the right size and position) in chunks is not as bad, but still pretty bad if it's slow enough to be perceivable. If an icon appears a very short amount of time after the window (eg. one frame or so later), and the background color of the area matched the background of the icon, it's mostly imperceptible for a user. But afaik we have no way to enforce the "it will appear one frame later" part, so for our automated tests we have to assume we are in the perceptible case of the icon appearing after a noticeable delay.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

I believe Tim might be fixing this with bug 1820709.

Comment 6

[:Gijs (he/him)]

Can we just drop the exception from browser_tabclose.js now, and/or maybe other ones? Or is there more to do after 1820709? (I confess to not fully understanding the finer points!)

Comment 7

[Emilio Cobos Álvarez [:emilio]]

If they pass without the exception, then sure.

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Taking a guess at priority & severity, feel free to adjust

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1820549 - try removing fxview icon exception from perf flicker tests r=mconley

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/815543e39c0f
try removing fxview icon exception from perf flicker tests r=mconley

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/815543e39c0f

Comment 12

[:Gijs (he/him)]

Updating summary for what we actually did here.