Closed Bug 586216 Opened 9 years ago Closed 9 years ago

Animated widgets have one timer per widget

Categories

(Toolkit :: XUL Widgets, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mstange, Assigned: bent.mozilla)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
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....
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
Yeah, that seems like the right approach to me....
Attached patch Patch, v1 (obsolete) — Splinter Review
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.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #465447 - Flags: review?(roc)
Attachment #465447 - Flags: review?(bzbarsky)
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.
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.
Attached patch Patch, v2 (obsolete) — Splinter Review
Ok, here we go. Thanks for the time Markus.
Attachment #465447 - Attachment is obsolete: true
Attachment #466390 - Flags: review?(roc)
Attachment #465447 - Flags: review?(roc)
Attachment #465447 - Flags: review?(bzbarsky)
--- 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 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.
Attachment #466390 - Attachment is obsolete: true
Attachment #466390 - Flags: review?(roc)
Attached patch Patch, v3 (obsolete) — Splinter Review
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.
Attachment #466494 - Flags: review?(roc)
New bug is 587876 for undetermined progress bars.
+  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?
(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;
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.
This is causing higher idle time CPU usage as per bug 584445
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Hey roc, review ping here? Want me to find another reviewer?
(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 :)
The main issue there is "Why do we need DEFAULT_ANIMATED_CONTENT_TIMEOUT?"
(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.
Why don't we just use PR_INT32_MAX, then?
Attached patch Patch, v3.1Splinter Review
Ok, now uses PR_UINT32_MAX
Attachment #466494 - Attachment is obsolete: true
Attachment #476908 - Flags: review?(roc)
Attachment #466494 - Flags: review?(roc)
Whiteboard: [has reviewed patch][ready to land]
Blocks: 584445
http://hg.mozilla.org/mozilla-central/rev/a0351ac4eb45
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has reviewed patch][ready to land]
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.