520614 - Fix usage of vbox in videocontrols.xml

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Bas Schouten (:bas.schouten)]

Currently the vbox in videocontrols.xml fills the entire video area (except the timeline bar). This means that if the background image update (like with the rotating throbber), the entire background is invalidated for each frame of the animation, causing significant composition overhead on larger video controller sizes.

Comment 1

[Justin Dolske [:Dolske]]

Enn's suggestion here was to center the box w/o using flex.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This shrinks the statusIcon box. At least, if I add a style rule to give it a border, it's tight around the throbber image instead of filling the whole <video> content area.

Bas: Would be useful if you could test with this patch and doublecheck that it fixes the problem you found.

Comment 3

[Neil Deakin]

Comment on attachment 405620 [details] [diff] [review]
Patch v.1

> .statusOverlay {
>+    -moz-box-align: center;
>+    -moz-box-pack: center;

Is there a reason to do this instead of using align/pack attributes?

Comment 4

[Justin Dolske [:Dolske]]

No, just seemed like since it was more of a presentational thing vs. structural, that having it in the CSS was better.

Comment 5

[Neil Deakin]

Comment on attachment 405620 [details] [diff] [review]
Patch v.1

The "presentational thing vs. structural" nonsense doesn't apply to xul.

What's relevant is whether a theme would want to override the values. In this case, other themes won't pick up this change and since this has a notable performance impact, it seems like something we'd want to have always applied.

That said, it doesn't matter too much to me which is used in this case, so the patch is ok as is if you like.

Comment 6

[Alfred Kayser]

As a long-time themer, I agree with the patch, and with comment #3, use the align and pack attributes to center the icon, and this will also remove the need to add -moz-box-pack/align to all themes.

Comment 7

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/900079760c48

Comment 8

[[e-mail]]

Comment on attachment 405620 [details] [diff] [review]
Patch v.1

>         <stack flex="1">
>+            <vbox flex="1" class="statusOverlay" hidden="true">
Flex on a stack child makes no sense either.

[Unusual mix of 2- and 4-space indent in this file...]