User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 The progressmeter in undetermined mode does not animate in the Mac OS X build of Firefox. It only shows the barber pole background image and never moves. Reproducible: Always Steps to Reproduce: 1. Create a xul document with a <progressmeter mode="undetermined" /> tag in it. 2. View the xul document in Firefox Actual Results: The progressmeter never moves. Expected Results: The progress meter should oscilate.

CCing Kevin.

(FYI, this affects Software Update)

fwiw, same issue in SeaMonkey (mac) with Classic skin. Works in Modern theme...

Hmm, this is old. The issue exists in Mozilla 1.5. Note that if you press down with the mouse on the progressmeter it animates for a second. If you remove the 'moz-appearance: progressbar'it works fine in SeaMonkey (well, the image isn't that mac-osx-ish, unfortunately).

Confirming and moving to the right component.

*** Bug 356625 has been marked as a duplicate of this bug. ***

Hmm, from a quick look, pinstripe does not have the binding for undetermined progressmeter that win/gnomestripe have (just noted this while seeing that binding is in global.css instead of progressmeter.css)

I'm seeing this in a rather visible way in the Download Manager. For example, saving a VM image from out internal file server (http://fs/public/QA/Virtualization/). The progress bar is usually frozen, although mouse clicks in the DM window will usually make it play a few frames. When the file is downloading in the background, I'll sometimes see it animate a frame or two at random intervals, many seconds apart. (The download rate in MB/s is updating frequently, however.)

Are you sure you aren't seeing the perf issues from loading all your download history?

Yes, the DM window is open and populated, and the download rate is updating, so it's not a perf issue. Mossop also noticed that this is a problem even in the determinate state... The blue part of the progress bar grows as the download progresses, but the subtle wavy pattern in the bar is supposed to animate as well. I'm guessing it's the same root widget cause for both, although the determinate-state issue could be spun off if it's not.

What I'm seeing for the determined state is that on initially opening the DM window the wavy pattern animates like it does in other apps, then after a short time (5-10 seconds) it just grinds to a halt, but then will update a few frames whenever I move the mouse around the bar, press or release the mouse button and refocus the DM window. Closing then reopening the window starts the cycle again

I doubt this is a problem at the widget level, but marking blocking as a cocoa widget bug for now.

There isn't any code which tells the widget to redraw itself, thus is only shows animating when something on the window reflows/repaints. The solution is to either have Cocoa use a timer which redraws the progressmeter at the desired interval, or to create a mac-specific binding for the default theme which sets some attribute (say, a step attribute) on it, which nsNativeTheme::WidgetStateChanged will listen for and redraw as needed. The latter is probably better.

If we go with a timer solution, that could also be used to fix the bug about default buttons not throbbing... I looked it up a while ago, and there are OS calls we can use to redraw the button (and probably the progressmeter) properly. They just need to be called often enough. Other apps typically use an "idle timer" in the event loop.

Reversing my decision about this being a blocker, not a regression.

This patch does two things: 1. It makes the state of the animation dependant on the absolute time (PR_Now()). 2. It installs a timer that causes the progressbar to be redrawn periodically. I would have called frame->Redraw or ->Invalidate, but those methods can only be called from layout/ because of linking problems (according to nsIFrame.h), so I used frame->PresContext()->PresShell()->FrameNeedsReflow(frame, nsIPresShell::eTreeChange, NS_FRAME_IS_DIRTY).

I've just noticed that the patch is buggy. frame->PresContext() can hang/crash the browser when frame->GetStyleContext() is not a valid pointer. This happened to me when https://bugzilla.mozilla.org/attachment.cgi?id=70673 finished loading. Can somebody help me here?

Comment on attachment 306603 [details] [diff] [review] Fix v1.0: Use timer for periodical update marking as obsolete

You absolutely cannot hang on to frame pointers and expect them to still be valid at some random time in the future. Frames can be destroyed anytime. I agree with Neil in comment #14. The best approach is for toolkit progressbar code to use a timer to update the progress bar.

Yeah, (In reply to comment #20) > You absolutely cannot hang on to frame pointers and expect them to still be > valid at some random time in the future. Frames can be destroyed anytime. Yeah, I realized that as well, and noticed that that's what nsWeakFrame is for. But then the compiler became angry... I can't use nsWeakFrame from /widget/. > I agree with Neil in comment #14. The best approach is for toolkit progressbar > code to use a timer to update the progress bar. I'm about to agree with you. However, I'm currently working on a patch that introduces nsITheme::WidgetNeedsPeriodicalRedraw(widgetType) and a nsPeriodicalRedrawManager class (in layout/base), which I think is also quite nice, as it's very generic and could theoretically be used for any widget on any theme. But that patch is getting rather lengthy...

That sounds nice but it's probably overkill for this bug and for 1.9. It would be a good thing to have if and when platforms start doing more sparkly animated themes, but I don't know if that's going to happen. It doesn't seem to be happening just yet. nsWeakFrame is a tool of last resort, we should exhaust all other options before we think about using it.

FWIW, this is the overkill patch I mentioned. First I used a dynamically sized nsTArray<nsWeakFrame>, but that couldn't possibly work because nsIPresShell holds references to the nsWeakFrames. When the nsTArray changes its capacity, the array elements are moved, so nsIPresShell's references become invalid. That's why I implemented a linked list, which makes this patch rather long. I created the nsPeriodicalRedrawManager files by copying the files from nsFrameManager. nsFrameManager uses the same header inclusion / forward declaration workaround. Now I'll see what I can come up with using the toolkit progressbar code.

OK, that was easy.

Post-1.9 we might want to take another look at the nsPeriodicalRedrawManager, it might fit well with the animation hooks we'll have in compositor.

What stops the interval timer when the element is removed the document? I wonder what effect this might have on performance and why you use 30 fps in the timer and 60 in the theme code. Could we use the same value and make it small, e.g. 10?

(In reply to comment #25) > Post-1.9 we might want to take another look at the nsPeriodicalRedrawManager, > it might fit well with the animation hooks we'll have in compositor. OK, cool. (In reply to comment #26) > What stops the interval timer when the element is removed the document? I don't know. Doesn't it stop automatically? I copied the code of the non-mac undetermined progressbar binding, which doesn't stop the timer either. (And I've just noticed that I left a dump() lying in there...) > I wonder what effect this might have on performance I've just tested, it's not pretty. Viewing attachment 70673 [details] (5 progressbars) in my debug build results in a cpu usage of 20% (of one processor, it seems) on my 1.83 GHz Intel Core 2 Duo Macbook. With 40 progressbars it reaches 100%. However, I suppose it's still a lot better than the undetermined progressbars on other platforms. Those progressbars resize a <xul:spacer> and thus trigger a reflow, which is more expensive than what we're doing here (I think). > and why you use 30 fps in > the timer and 60 in the theme code. Could we use the same value and make it > small, e.g. 10? The FPS in the theme code control the actual speed of the animation. I came up with 30 and 60 by looking at other Cocoa apps. (Actually, I made a video, counted the number of stripes moved during one second (almost 4) and multiplied this with the number of pixels per stripe (16).) Choosing a value of 10 for the steps per second would make the animation slower, which would be a difference in perception to native cocoa apps. The FPS in the timer control the smoothness of the animation. If we make it smaller, the CPU load goes down, the animation isn't as smooth, but the speed of the animation is still the same. That's because I made the animation step a function of the actual time (PR_Now()), and not of the "predicted" time. I've just tested 10 FPS for the timer; it's way too jittery, and interestingly, CPU usage is still at almost 20%... With 24 FPS, it's somewhat smooth, but you can feel an uneasy difference to 30 FPS. I'd say we keep 30.

(In reply to comment #27) > > I wonder what effect this might have on performance > > I've just tested, it's not pretty. [...] Ouch. This could be a perf issue for Firefox -- there's a small progress bar down in the lower-right of the browser, shown for every page while loading. The download manger is also using progress bars (surprise), and it's probably not too unusual to have multiple downloads running. It might be a good idea to do a standalone talos run [http://wiki.mozilla.org/StandaloneTalos] to get a feel for potential Tp impact.

There's no _undetermined_ progressmeter within the browser window.

there can be in the download manager though - mind you it's not so common unless you are on windows.

(In reply to comment #29) > There's no _undetermined_ progressmeter within the browser window. Err, oops. I guess I started assuming back in comment 11 that a single fix would take care of the animations for both the determined and undetermined states, and wasn't following along closely.

I guess we could try the same approach to make OS X default buttons throbber as well? Hopefully there is a solution here that won't kill performance... See bug 206636.

(In reply to comment #31) > (In reply to comment #29) > > There's no _undetermined_ progressmeter within the browser window. > > Err, oops. I guess I started assuming back in comment 11 that a single fix > would take care of the animations for both the determined and undetermined > states, and wasn't following along closely. You're right. This patch animates progressbars both in undetermined and in normal mode. Sorry that I didn't mention it. (In reply to comment #30) > there can be in the download manager though - mind you it's not so common > unless you are on windows. I'd actually like it to be more common - during the start of the download, when there hasn't been any data received yet, it should be in undetermined mode. (Right now it's just empty in that state.) At least that's what other OS X apps do, and I find it very nice. (In reply to comment #28) > (In reply to comment #27) > Ouch. This could be a perf issue for Firefox -- there's a small progress bar > down in the lower-right of the browser, shown for every page while loading. However, while it will increase the CPU load, I don't think it will affect page load and rendering time very much. During page load, it's the internet connection that's the bottleneck; animating during that time won't change much. And during rendering, the timer won't fire anyway, so it shouldn't be much of a problem. > It might be a good idea to do a standalone talos run > [http://wiki.mozilla.org/StandaloneTalos] to get a feel for potential Tp > impact. I'll look into that. I'll also make another test with my "overkill patch" which could have better performance. (In reply to comment #32) > I guess we could try the same approach to make OS X default buttons throbber > as well? I had a quick look at the button code and noticed a problem; since bug 407093, we draw "push buttons" using NSCell, and I couldn't find a way to set an animation step attribute there. That bug might get complicated.

(In reply to comment #33) > I'd actually like it to be more common - during the start of the download, when > there hasn't been any data received yet, it should be in undetermined mode. > (Right now it's just empty in that state.) At least that's what other OS X apps > do, and I find it very nice. That almost never happens though unless you do lots of save link as downloads from the same server.

I've done a standalone talos run with the "tp" benchmark (page_load_test/pages/) and 20 cycles. Results: http://spreadsheets.google.com/pub?key=pMFobsg1RSsVxHAwwpEiKUA I don't really know what to make of the numbers. It looks like "Fix v3.0" ("toolkit patch") makes page load 4% slower, and "Fix v2.0" ("overkill patch") makes it faster... (In reply to comment #34) > That almost never happens though unless you do lots of save link as downloads > from the same server. Oh. You're right.

This is essentially the same as fix v3.0, just that the binding is only set for undetermined progressbars. So it shouldn't affect any benchmarks.

Is it really a good idea to repaint progressmeters on *any* attribute change? Would be more efficient to use PR_IntervalNow instead of PR_Now. The toolkit part should probably be reviewed by someone else, although it looks fine to me.

Comment on attachment 312480 [details] [diff] [review] Fix v3.2: address review comments Looks ok

What's the regression risk here? Approval request pending answer.

Comment on attachment 312480 [details] [diff] [review] Fix v3.2: address review comments re-request approval once risk question is addressed.

Comment on attachment 312480 [details] [diff] [review] Fix v3.2: address review comments The regression risk is very low. Performance shouldn't be affected because only undetermined progressbars are animated; these aren't used very often in Firefox chrome. I can't think of any other possible regressions.

Comment on attachment 312480 [details] [diff] [review] Fix v3.2: address review comments a1.9=beltzner

mozilla/toolkit/content/widgets/progressmeter.xml 1.12 mozilla/toolkit/themes/pinstripe/global/global.css 1.21 mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm 1.90 mozilla/widget/src/xpwidgets/nsWidgetAtomList.h 1.26

I'm not certain, but this patch may have introduced bug 434513 Forecastfox is causing high CPU usage in Firefox on OS X.

Yes, that's very likely - see bug 432028.