304147 - progressmeter in undetermined mode does not work in Mac OS X

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P4)

Description

[Daniel Beckham]

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.

Comment 1

[Daniel Beckham]

Attachment: Testcase showing the problem.

Comment 2

[Asaf Romano (gone)]

CCing Kevin.

Comment 3

[Asaf Romano (gone)]

(FYI, this affects Software Update)

Comment 4

[Stefan [:stefanh]]

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

Comment 5

[Stefan [:stefanh]]

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).

Comment 6

[Stefan [:stefanh]]

Confirming and moving to the right component.

Comment 7

[Adam Guthrie]

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

Comment 8

[Robert Kaiser]

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)

Comment 9

[Justin Dolske [:Dolske]]

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.)

Comment 10

[Shawn Wilsher :sdwilsh]

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

Comment 11

[Justin Dolske [:Dolske]]

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.

Comment 12

[Dave Townsend [:mossop]]

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

Comment 13

[Josh Aas]

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

Comment 14

[Neil Deakin]

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.

Comment 15

[Håkan Waara]

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.

Comment 16

[Josh Aas]

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

Comment 17

[Markus Stange [:mstange]]

Attachment: Fix v1.0: Use timer for periodical update

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).

Comment 18

[Markus Stange [:mstange]]

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 19

[Markus Stange [:mstange]]

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

marking as obsolete

Comment 20

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

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.

Comment 21

[Markus Stange [:mstange]]

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...

Comment 22

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

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.

Comment 23

[Markus Stange [:mstange]]

Attachment: Fix v2.0: Implement nsPeriodicalRedrawManager

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.

Comment 24

[Markus Stange [:mstange]]

Attachment: Fix v3.0: Use a mac-specific binding that changes an attribute

OK, that was easy.

Comment 25

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

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.

Comment 26

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

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?

Comment 27

[Markus Stange [:mstange]]

(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.

Comment 28

[Justin Dolske [:Dolske]]

(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.

Comment 29

[Asaf Romano (gone)]

There's no _undetermined_ progressmeter within the browser window.

Comment 30

[Shawn Wilsher :sdwilsh]

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

Comment 31

[Justin Dolske [:Dolske]]

(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.

Comment 32

[Håkan Waara]

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.

Comment 33

[Markus Stange [:mstange]]

(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.

Comment 34

[Shawn Wilsher :sdwilsh]

(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.

Comment 35

[Markus Stange [:mstange]]

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.

Comment 36

[Markus Stange [:mstange]]

Attachment: Fix v3.1: last-minute low-risk patch

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.

Comment 37

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

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 38

[Markus Stange [:mstange]]

Attachment: Fix v3.2: address review comments

Comment 39

[Neil Deakin]

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

Looks ok

Comment 40

[Damon Sicore (:damons)]

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

Comment 41

[Damon Sicore (:damons)]

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

re-request approval once risk question is addressed.

Comment 42

[Markus Stange [:mstange]]

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 43

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

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

a1.9=beltzner

Comment 44

[Asaf Romano (gone)]

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

Comment 45

[WD]

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

Comment 46

[Markus Stange [:mstange]]

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