484935 - videocontrols should use display:none on hidden items to minimize CPU usage

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Spun off from bug 483841, since the patch there partially fixed the problem but in a different, more generic way.

On my MBP, a video that's finished buffering and not doing anything causes ~2-3% CPU usage according to Activity Monitor. With this patch, CPU usage drops to 0.5-1%.

Comment 1

[Neil Deakin]

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

>-.controlBar {
>-     visibility: hidden;
>-     opacity: 0.0;
>+.controlBar[hidden] {
>+    display: none;

Why would the default hidden (from xul.css) not apply?


>-                    // Use .visibility to ignore mouse clicks when hidden.
>+                    // Trigger CSS suppression of mouse clicks and throbber CPU usage.
>                     if (fader.isVisible)
>-                        fader.element.style.visibility = "visible";
>+                        fader.element.removeAttribute("hidden");
>                     else
>-                        fader.element.style.visibility = "hidden";
>+                        fader.element.setAttribute("hidden", "true");

The hidden property is implemented in C++ so doesn't have the xbl and security issues that other properties sometimes have.

Thus, the whole block above can be replaced with:

fader.element.hidden = !fader.isVisible;

>-                fader.element.style.visibility = "visible";
>-                fader.element.style.opacity = 1.0;
>+                fader.element.removeAttribute("hidden");

Similar here

Comment 2

[Justin Dolske [:Dolske]]

(In reply to comment #1)

> >+.controlBar[hidden] {
> >+    display: none;
> 
> Why would the default hidden (from xul.css) not apply?

Oh, hrm, I wasn't even thinking about XUL having .hidden. Duh.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Turns out I can't set .hidden without triggering the security checks, so I'm still using it as an attribute.

Comment 4

[Neil Deakin]

Comment on attachment 369155 [details] [diff] [review]
Patch v.2

Bah, ok. I guess this will have to do.

Comment 5

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/85bdf13ea382

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 369155 [details] [diff] [review]
Patch v.2

a191=beltzner

Comment 7

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/df2705e294b9

Comment 8

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090507 Shiretoko/3.5b5pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090507 Minefield/3.6a1pre