586216 - Animated widgets have one timer per widget

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Markus Stange [:mstange]]

There are two types of animated widgets on OS X: progressbars and default buttons. These widgets are animated by their binding which uses setInterval and changes an attribute that triggers a redraw.

The problem is that these widgets don't share their timers. So once you have multiple default buttons visible (which shouldn't happen very often) or a default button and a progressbar, timers fire more frequently than necessary. Consequently, the main thread is woken up too often.

Comment 1

[Markus Stange [:mstange]]

Using mozRequestAnimationFrame() would share the timers, but it would mean that we increase the animation frequency for buttons from 10 FPS to 50 FPS... which seems counter-productive.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah.  What it feels like we want here is a solution which only requests a repaint when the widget is painted.  In other words, if you can't see the button or progressmeter, then it uses 0 cpu....

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could do this in the native theme code:
1) Keep a set of strong pointers to the elements of animated controls
2) When an animated control is drawn, add its element to the set
3) When the set changes from empty to non-empty, queue a timer
4) When the timer fires, invalidate frames for all elements in the set and clear the set

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that seems like the right approach to me....

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Here we go. This seems to work well on mac, I'll tryserver it once I hear whether you guys like this approach or not.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Your changes to the JS animation in progressmeter.xml should probably be in a separate patch. They also need to be a bit more elaborate; since the browser animation frame rate is unknown (and variable), you need to track the elapsed time and set the animation progress based on time rather than the number of events you've seen.

Instead of InvalidateExternal, make InvalidateOverflowRectExternal.

Otherwise looks great. Lose the printfs and put {} around your single-statement if bodies.

Comment 7

[Markus Stange [:mstange]]

Mac progressbars should update 30 times per second, not only 10. Can you add a parameter aMinimumFrameRate to QueueAnimatedContentForRefresh and reschedule the timer with the shorter timeout if it's already been scheduled with a longer one?

On OS X, non-indeterminate progressbars should animate, too. See bug 463695.

You don't need to call QueueAnimatedContentForRefresh on non-Mac platforms. Those platforms use the "progressmeter-undetermined" binding which already causes the right invalidations by moving XUL boxes.

Now that we're no longer using the step attribute to cause invalidations you can remove the corresponding code from nsNativeThemeCocoa::WidgetStateChanged; specifically, move the NS_THEME_PROGRESSBAR* case labels above *aShouldRepaint = PR_FALSE and remove the step condition in the long if at the end of the method. Then you can also remove the step atom from nsWidgetAtomList.h.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Ok, here we go. Thanks for the time Markus.

Comment 9

[Markus Stange [:mstange]]

--- a/toolkit/themes/pinstripe/global/global.css	
+++ a/toolkit/themes/pinstripe/global/global.css	
@@ -54,7 +54,7 @@ menulist > menupopup,
 }
 
 progressmeter {
-  -moz-binding: url("chrome://global/content/bindings/progressmeter.xml#progressmeter-periodic-redraw");
+  -moz-binding: url("chrome://global/content/bindings/progressmeter.xml#progressmeter");
 }

You can remove this rule completely. That binding is already set by xul.css.

--- a/widget/src/cocoa/nsNativeThemeCocoa.mm	
+++ a/widget/src/cocoa/nsNativeThemeCocoa.mm	

     case NS_THEME_BUTTON:
       if (IsDefaultButton(aFrame)) {
+        if (NS_FAILED(QueueAnimatedContentForRefresh(aFrame->GetContent(), 10))) {

NS_FAILED on a PRBool?

I don't think you addressed roc's comments.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 466390 [details] [diff] [review]
Patch, v2

(In reply to comment #9)
> I don't think you addressed roc's comments.

Um... Yeah, somehow I missed those. Yikes.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v3

Ok, this addresses mstange's and roc's comments. I'm moving changes to the progressbar-undetermined to a new bug as it's really a separate change.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

New bug is 587876 for undetermined progress bars.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  virtual void InvalidateOverflowRectExternal(const nsRect& aDamageRect)
+  { return Invalidate(aDamageRect); }

Lose the aDamageRect parameter and call InvalidateOverflowRect

Why do we a DEFAULT_ANIMATED_CONTENT_TIMEOUT?

I wonder how the Cocoa theme code tracks animation progress. Markus, do you know? Does it just look at the clock and give all widgets of the same type the same state at a given moment in time?

Comment 14

[Markus Stange [:mstange]]

(In reply to comment #13)
> I wonder how the Cocoa theme code tracks animation progress. Markus, do you
> know? Does it just look at the clock and give all widgets of the same type the
> same state at a given moment in time?

Exactly. For progressbars it looks like this:

  PRInt32 stepsPerSecond = inIsIndeterminate ? 60 : 30;
  PRInt32 milliSecondsPerStep = 1000 / stepsPerSecond;
  tdi.trackInfo.progress.phase = PR_IntervalToMilliseconds(PR_IntervalNow()) /
                                 milliSecondsPerStep % 16;

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, I wonder whether we should be using the refresh driver here instead of a timer. We might need to add an API to nsIPresShell or something to request a refresh and queue a callback to happen at that time.

Comment 16

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

This is causing higher idle time CPU usage as per bug 584445

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hey roc, review ping here? Want me to find another reviewer?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What about comment #15?

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #18)
> What about comment #15?

I asked you on irc and you said you didn't want a patch for that :)

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, what about comment #13? :-)

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The main issue there is "Why do we need DEFAULT_ANIMATED_CONTENT_TIMEOUT?"

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #21)
> The main issue there is "Why do we need DEFAULT_ANIMATED_CONTENT_TIMEOUT?"

Oh. Well, the way everything works right now the code saves the smallest timeout requested by any of the widgets. If we don't reset it after the timer fires then every future widget will get that smaller timeout value even if none of the widgets scheduled for that timeout need a smaller value.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't we just use PR_INT32_MAX, then?

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v3.1

Ok, now uses PR_UINT32_MAX

Comment 25

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a0351ac4eb45