User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110519 Firefox/6.0a1 Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110519 Firefox/6.0a1 The progress bar shouldn't have any gaps within its container. The indeterminate progress bar is also implemented incorrectly. Reproducible: Always Steps to Reproduce: 1. Download add-on from Add-ons for Firefox 2. Watch the progress bar

(In reply to comment #2) > Created attachment 534258 [details] > Better Comparison Let split the gaps and the "fading edges" problem up.

(In reply to comment #3) > (In reply to comment #2) > > Created attachment 534258 [details] > > Better Comparison > > Let split the gaps and the "fading edges" problem up. filed bug 658885

(In reply to comment #0) > The indeterminate progress bar is also implemented incorrectly. Was is better with Firefox 4? I mean, is that a regression in Nightly?

As far as I can tell, Firefox 4 doesn't have a native indeterminate style for XUL progress bars on Windows. So, the fix here would consist of using <html:progress> for <xul:progressmeter> and thus having a native style.

IsIndeterminateProgress(stateFrame, eventStates) always returns false for indeterminate xul meters. I think that's the main problem.

The frame layout is different between HTML and XUL - I can put a patch together that fixes this, but I'm not 100% it'll be right. Need to chat with roc about what's going on here. HTML top: [nsBlockFrame] state: [nsProgressFrame] frame: [nsBlockFrame] XUL: top: [nsProgressMeterFrame] state: [nsStackFrame] frame: [nsLeafBoxFrame] nsIFrame* topFrame = stateFrame->GetParent(); nsIFrame* stateFrame = aFrame->GetParent(); (aFrame)

ditching some debug code.

classic mode support

cleanup and testing on xp.

I don't know if this part of your patch, but could you consider removing the redundant information shown whilst the indeterminate bar is active, like during addon download it says "Unknown time remaining".

(In reply to ... from comment #17) > I don't know if this part of your patch, but could you consider removing the > redundant information shown whilst the indeterminate bar is active, like > during addon download it says "Unknown time remaining". File a separate bug on that. I'm not working on surrounding content, I'm just fixing issues with the meter.

Not a complete cleanup, but it's a start.

Comment on attachment 600800 [details] [diff] [review] meter overhaul v.1 Neil, requesting sr from you on the changes to progressmeter.xml. Also, curious if you could take a look at the IsProgressMeterFilled function. I'm not sure if the constants used there are safe. I looked at data from various nsIFrame methods for similar values that I could query, but didn't find anything useful. I assume these are due to differences in css styling, but I couldn't find the css that was responsible either, so I just set the values I needed based on manual testing.

It would help if I could actually get the patch to apply...

(In reply to Jim Mathies [:jimm] from comment #23) > Created attachment 600821 [details] [diff] [review] > base rollup This also has the corners patch from bug 392672. There are test cases in bug 726144 for testing.

Comment on attachment 600799 [details] [diff] [review] prelim cleanup v.1 Review of attachment 600799 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsNativeThemeWin.cpp @@ +707,5 @@ > + return; > + RECT offset; > + if (!IsAppThemed()) > + offset = buttonData[CAPTION_CLASSIC].hotPadding[button]; > + else if (WinUtils::GetWindowsVersion() == WinUtils::WINXP_VERSION) nit: I know this isn't introduced by this patch but I think we should be checking for Windows 2003 or Win XP here. Or better yet just < Vista.

Comment on attachment 600800 [details] [diff] [review] meter overhaul v.1 Review of attachment 600800 [details] [diff] [review]: ----------------------------------------------------------------- r+ with minor comments addressed, the questions about IsProgressMeterFilled are probably fine just want to know for my information. ::: widget/windows/nsNativeThemeWin.cpp @@ +844,5 @@ > +} > + > +/* > + * IsProgressMeterFilled - Guesses if a progress meter is at 100% fill based > + * on a comparison of the width or height of the meter and its parent track. You say "guesses". Is it possible that this function will return invalid information? For example maybe if the DPI settings are adjusted in Windows? If the wrong information is returned what is the effect? @@ +984,5 @@ > + > + RECT adjWidgetRect, adjClipRect; > + adjWidgetRect = *aWidgetRect; > + adjClipRect = *aClipRect; > + if (WinUtils::GetWindowsVersion() != WinUtils::WINXP_VERSION) { < vista here. I think these checks should almost never be == nor != @@ +986,5 @@ > + adjWidgetRect = *aWidgetRect; > + adjClipRect = *aClipRect; > + if (WinUtils::GetWindowsVersion() != WinUtils::WINXP_VERSION) { > + // Adjust clipping out by one pixel. XP progress meters are inset, > + // Vista+ are not. // Adjust clipping out by one pixel. XP and Windows 2003 progress meters are inset, // Vista+ are not.

(In reply to Brian R. Bondy [:bbondy] from comment #26) > Comment on attachment 600800 [details] [diff] [review] > meter overhaul v.1 > > Review of attachment 600800 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with minor comments addressed, the questions about IsProgressMeterFilled > are probably fine just want to know for my information. > > ::: widget/windows/nsNativeThemeWin.cpp > @@ +844,5 @@ > > +} > > + > > +/* > > + * IsProgressMeterFilled - Guesses if a progress meter is at 100% fill based > > + * on a comparison of the width or height of the meter and its parent track. > > You say "guesses". Is it possible that this function will return invalid > information? For example maybe if the DPI settings are adjusted in Windows? > If the wrong information is returned what is the effect? > Good question, I'm not sure where those numbers come from. I looked at the css and various nsIFrame values but didn't find anything that correlated. Hence the sr from Neil since he understands layout much better than I do. will update the rest, thanks!

Note worst case scenario if IsProgressMeterFilled fails - we get a pulse on completed meters on Vista+, which we have already, and on XP we get partial chunks drawn while the meter progresses. Purely cosmetic side effects.

Comment on attachment 600800 [details] [diff] [review] meter overhaul v.1 Sorry for the delay; I knew that #ifdef was the wrong way of turning off the XBL animation, but I couldn't figure out what the correct way was, until I stumbled across some checkins and put two and two together; what you need to do is to remove the CSS override for progressmeter in winstripe's global.css. I see an equality test against WINXP_VERSION. Will this fail on Server 2003? Anyway, to get to the point, I take it that before Vista, you only draw the animation whenever the progressmeter is indeterminate while after Vista you draw it unless the progress is 100%? In that case, you just need to compare the result of the GetProgressValue and GetProgressMax functions.

(In reply to neil@parkwaycc.co.uk from comment #29) > Comment on attachment 600800 [details] [diff] [review] > meter overhaul v.1 > > Sorry for the delay; I knew that #ifdef was the wrong way of turning off the > XBL animation, but I couldn't figure out what the correct way was, until I > stumbled across some checkins and put two and two together; what you need to > do is to remove the CSS override for progressmeter in winstripe's global.css. That worked. Updated patch posted. > I see an equality test against WINXP_VERSION. Will this fail on Server 2003? Isn't server 2003 hard coded to classic? I'll look for issues there but I'm not too worried about it. > Anyway, to get to the point, I take it that before Vista, you only draw the > animation whenever the progressmeter is indeterminate while after Vista you > draw it unless the progress is 100%? On Vista and up, when you have a non-indeterminate progress, there is a pulse glow that moves over the underlying bar. I use IsProgressMeterFilled to determine if the pulse needs to be rendered. // Glow or pulse effects rendering if (!indeterminate && WinUtils::GetWindowsVersion() >= WinUtils::VISTA_VERSION) { // get the right part based on os and orientation PRInt32 overlayPart = GetProgressOverlayStyle(vertical); if (!IsProgressMeterFilled(aFrame, aWidgetRect, aAppUnits, vertical)) { .. This doesn't apply to indeterminates on xp, which are always rendered, see the chunk of code above - // Indeterminate rendering if (indeterminate) { .. > In that case, you just need to compare > the result of the GetProgressValue and GetProgressMax functions. Ah, I'll try this. I see we have these over in the macos code.

> > In that case, you just need to compare > > the result of the GetProgressValue and GetProgressMax functions. > > Ah, I'll try this. I see we have these over in the macos code. Actually according to the comments these calls don't work on xul elements, so I'll still have to hard code constants for those. :/

Comment on attachment 614575 [details] [diff] [review] meter overhaul v.2 Review of attachment 614575 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/winstripe/global/progressmeter.css @@ +43,5 @@ > > /* ::::: progressmeter ::::: */ > > progressmeter { > + /*-moz-appearance: progressbar; themelib now handles this. */ > what you need to do is to remove the CSS override for progressmeter in winstripe's global.css. So this didn't work at all. It basically turned off themed rendering. Not sure if I've overridden the right thing though. The only indication these are progress meters are moz appearance styles.

- reverted back to the original progressmeter.xul changes - added html progress completion checking and moved the macos routines down in nsNativeTheme. pushed to try for mac builds.

Comment on attachment 614599 [details] [diff] [review] meter overhaul v.3 Passed try on mac.

(In reply to Jim Mathies from comment #33) > ::: toolkit/themes/winstripe/global/progressmeter.css > > what you need to do is to remove the CSS override for progressmeter in winstripe's global.css. > So this didn't work at all. It basically turned off themed rendering. Not > sure if I've overridden the right thing though. Yeah, the wrong file name is a big giveaway ;-)

(In reply to Jim Mathies from comment #31) > (In reply to comment #29) > > I see an equality test against WINXP_VERSION. Will this fail on Server 2003? > Isn't server 2003 hard coded to classic? Ah, so we still want to use the XBL undetermined progressmeter in that case?

Oh, and what about Vista/7's equivalent of Classic (also for Server 2008/R2)?

Comment on attachment 614599 [details] [diff] [review] meter overhaul v.3 >- false, GetProgressValue(aFrame), >- GetProgressMaxValue(aFrame), aFrame); >+ false, nsNativeTheme::GetProgressValue(aFrame), >+ nsNativeTheme::GetProgressMaxValue(aFrame), aFrame); [Moving a function to the base class shouldn't change the way you call it] >-nsNativeThemeCocoa::GetProgressValue(nsIFrame* aFrame) ... >- return (double)CheckIntAttr(aFrame, nsGkAtoms::value, 0); Isn't this the code that handles XUL progressmeters?

(In reply to neil@parkwaycc.co.uk from comment #39) > Comment on attachment 614599 [details] [diff] [review] > meter overhaul v.3 > > >- false, GetProgressValue(aFrame), > >- GetProgressMaxValue(aFrame), aFrame); > >+ false, nsNativeTheme::GetProgressValue(aFrame), > >+ nsNativeTheme::GetProgressMaxValue(aFrame), aFrame); > [Moving a function to the base class shouldn't change the way you call it] I'm calling it from a function, so I changed these to static methods. > > >-nsNativeThemeCocoa::GetProgressValue(nsIFrame* aFrame) > ... > >- return (double)CheckIntAttr(aFrame, nsGkAtoms::value, 0); > Isn't this the code that handles XUL progressmeters? It doesn't work for xul meters.

(In reply to neil@parkwaycc.co.uk from comment #38) > Oh, and what about Vista/7's equivalent of Classic (also for Server 2008/R2)? The classic mode rendering code is a little farther down in the patch, it doesn't pass through DrawProgressMeter. I'll break this up into separate chunks so it's easier to review.

(In reply to neil@parkwaycc.co.uk from comment #36) > (In reply to Jim Mathies from comment #33) > > ::: toolkit/themes/winstripe/global/progressmeter.css > > > what you need to do is to remove the CSS override for progressmeter in winstripe's global.css. > > So this didn't work at all. It basically turned off themed rendering. Not > > sure if I've overridden the right thing though. > Yeah, the wrong file name is a big giveaway ;-) progressmeter[mode="undetermined"] { -moz-binding: url("chrome://global/content/bindings/progressmeter.xml#progressmeter-undetermined"); } Ah, doh. This is what I was looking for.

This was the right fix, thanks for your patience!

that's it!

(In reply to Jim Mathies from comment #40) > (In reply to comment #39) > > >- false, GetProgressValue(aFrame), > > >- GetProgressMaxValue(aFrame), aFrame); > > >+ false, nsNativeTheme::GetProgressValue(aFrame), > > >+ nsNativeTheme::GetProgressMaxValue(aFrame), aFrame); > > [Moving a function to the base class shouldn't change the way you call it] > I'm calling it from a function, so I changed these to static methods. nsNativeThemeCocoa inherits from nsNativeTheme, but it looks like you forgot to move the declarations, so you worked around it by forcing the compiler to look on the base class... > > >-nsNativeThemeCocoa::GetProgressValue(nsIFrame* aFrame) > > ... > > >- return (double)CheckIntAttr(aFrame, nsGkAtoms::value, 0); > > Isn't this the code that handles XUL progressmeters? > It doesn't work for xul meters. Well, that's a bit of an unhelpful statement; it was surely supposed to work, otherwise the Mac code wouldn't be using it. What value do you actually get?

minor fix

(In reply to Jim Mathies [:jimm] from comment #43) > Created attachment 614736 [details] [diff] [review] > part1 - disable xul xbl bindings > > This was the right fix, thanks for your patience! So unfortunately while this is correct, it exposes some sort of a xul progress meter bug where the base widgetrect we get in the theme lib is exactly half the size of the real progress. :/

Comment on attachment 614738 [details] [diff] [review] part3 - nativethemewin cleanup and helpers Maybe in the XUL case it might be that GetProgress(Max)Value needs a different frame than the one your old code uses to gets the size manually?

Hide a remnant of the old way of doing things. This fixes the indeterminate xul clipping problem.

Comment on attachment 614755 [details] [diff] [review] part6 - hide progress-remainder spacer bah that broke xul determinate meters.

(In reply to neil@parkwaycc.co.uk from comment #51) > Comment on attachment 614738 [details] [diff] [review] > part3 - nativethemewin cleanup and helpers > > Maybe in the XUL case it might be that GetProgress(Max)Value needs a > different frame than the one your old code uses to gets the size manually? Might be. Odd though it seems as though the mac code always passes in the frame handed in to the render call. I'm surprised that works for xul meters. I'll experiment around some more.

Comment on attachment 614736 [details] [diff] [review] part1 - disable xul xbl bindings I've reverted back to the ifdef'ing for now as this doesn't seem to work. Proper widths on various elements don't get initialized right unless we run through the script in this binding.

updated to use the nsNativeTheme static routines. Switching out parent frames seems to address this.

reposting the complete set rebased to latest mc.

That's it. parts 1, 2, 3, 4, 5, 6 are part of this bug. I could just roll them all together here but I've already got reviews in the other bugs.

Comment on attachment 614803 [details] [diff] [review] part 2 - xbl binding ifdefing I really want the XUL code paths to match the HTML code paths, because that reduces the maintenance nightmare. Mac native theme code draws the entire progress meter at once, so it doesn't actually care what the internal frame structure of the progress meter is. In particular this affects the way that it retrieves the value and maximum. However I see you are keeping the border and chunk separate, which is a reasonable thing to do if that's the best way to draw them on Windows. Obviously when you have a determined progress meter the layout code calculates the correct chunk size which makes things easy for you. For an HTML progress meter the layout code helpfully gives you a completely filled chunk which you then apply your animation to. I believe we should arrange this to happen to XUL progress meter as well. Once this is done we can remove the binding override from winstripe's global.css and then you will see a full size undetermined progress chunk. However I think there are still some code tweaks required because your undetermined progress code assumes different frame layouts for XUL and HTML.

Can we land what we have here a file follow ups? The point of these patches is to make progress meters look right on windows. They accomplish this. I'm deep in win8 work and don't have the bandwidth to revisit this work in depth at this time.

If you don't want my review, then don't ask for it.

A quick summary for those interested: 2-4: contains design flaws or coding errors 5: can't really test sorry 6: looks good to go

(In reply to neil@parkwaycc.co.uk from comment #70) > If you don't want my review, then don't ask for it. I totally respect your opinion and your review feedback. I'm just trying to finish up this work because I've been pulled off onto other things. Our current implementation on mc (and as such what our users experience) is totally broken. The patches here improve that situation substantially. I understand you would like to see additional work in this area, but I'm not a fan of holding up landings that make improvements on existing functionality if the work is solid and safe and works. At the end of the day we're dealing with cosmetic issues here and code that doesn't get touched a lot. There aren't a lot of maintenance issues to deal with. I would love to spend the next two weeks overhauling this work so that it meets your expectations. :) But I don't have the time for that right now. I would however love to see our progress meters rendering correctly - hence my interest in landing this work.

(In reply to comment #71) > 5: can't really test sorry OK, so I think I managed to apply the patches to Windows XP, which uses this codepath, right? According to the legend in the attachment to bug 726144, partial chunks should not be rendered. This did not appear to be the case, particularly noticably on the RTL rendering. (Sadly I forgot to compare against the rendering without the patches. Oops.)

D'oh, I forgot to apply part 8. Silly me.

So, apart from the whole chunks only improvement, XP Luna progressmeters appear to be little changed by these patches, although I have to admit that I actually prefer the speed of the old indeterminate meter, speaking of which, I doubt that themes would appreciate progressmeter.xml being different on Windows.

(In reply to neil@parkwaycc.co.uk from comment #75) > So, apart from the whole chunks only improvement, XP Luna progressmeters > appear to be little changed by these patches, although I have to admit that > I actually prefer the speed of the old indeterminate meter, speaking of > which, I doubt that themes would appreciate progressmeter.xml being > different on Windows. The patch in bug 745447 looks like it will remove the need for the ifdef in progressmeter.xml. If that works, we should be good to go here. Overall the improvements these patches provide across all Windows version are substantial. There's a nice test case in bug 726144 if you would like to compare current nightly behavior to a build with these patches applied.

With the forgotten changes to nsNativeThemeCocoa.h which avoids some unnecessary changes to nsNativeThemeCocoa.mm

Simplified helpers now that bug 745447 is fixed. Note: Because I don't use mq, I had to hand-craft the patch, so it may be corrupt. Sorry about that.

Thanks for the updates. Your part 3 and 4 look to be the same patch. With bug 745447 landed, I think part 2 can go away now. I'll put this all together and for some testing. If this is working with your new patches and part 2 clipped, should I kick off reviews again, or do we need more changes?

This is a version of part 3 that I ran past try, sadly I mistyped the bug number in my try push so the results got lost but it did compile OK.

Comment on attachment 614806 [details] [diff] [review] part 4 - nativethemewin cleanup and helpers Unobsoleting this because I seem to have obsoleted it by mistake.

(In reply to Jim Mathies from comment #80) > Thanks for the updates. Your part 3 and 4 look to be the same patch. Sorry about that. > With bug 745447 landed, I think part 2 can go away now. No, it needs the new part 2 patch. And of course because I forgot about this bug all the patches have bitrotted :-(

Comment on attachment 659557 [details] [diff] [review] part 3 - nativetheme / cocoa changes Including this one. D'oh!

(In reply to Jim Mathies from comment #80) > If this is working with your new patches and part 2 clipped, should I kick off reviews again, or do we need more changes? Except for bitrot, I don't think there are any more changes. I now have a Windows 8 Preview VM so I can even test the Aero progress bars too.

Hey guys, what's the status here?

(In reply to ben turner [:bent] from comment #87) > Hey guys, what's the status here? There's a bug to track down in the indeterminate rendering with current mc and a couple patches still need reviews from neil. Plus I imagine there's some bitrot to deal with.

I tried updating these patches to trunk, but I get this crash attempting to draw the undetermined progress meter (either HTML or XUL). (Why can't we draw the undetermined progress meter as a full meter with the activity pulse?)

(In reply to neil@parkwaycc.co.uk from comment #89) > Created attachment 717877 [details] [diff] [review] > Crash > > I tried updating these patches to trunk, but I get this crash attempting to > draw the undetermined progress meter (either HTML or XUL). Yeah there's a problem accessing the overlay images in the array they are held in. I haven't had time to look at it in depth. > (Why can't we draw the undetermined progress meter as a full meter with the > activity pulse?) By full meter do you mean 100% filled? I suppose you could do that but I think it would just look like a 100% meter. ;)

I rebased the patches to a recent m-c tip. However, the resulting changeset crashes out on my VM, so I made some changes of my own just to get this moving. This works for me on Windows 7. If it helps, I can provide: * an hg export of the existing attachments, rebased, but without the simplifications due to bug 745447, OR * an hg export of the attachments with the simplifications, OR * an hg export of this attachment as 9 separate patches

Looks like you just clipped out bug 642846, which was the patch that had the problem. Does that sound right?

a try build we can run against the tests case in bug 726144 would be great. Thanks for picking this up Neil.

Comment on attachment 718946 [details] [diff] [review] Something I got to work Review of attachment 718946 [details] [diff] [review]: ----------------------------------------------------------------- clearing my feedback flag since I think jimm will provide feedback.

(In reply to Jim Mathies from comment #92) > Looks like you just clipped out bug 642846, which was the patch that had the > problem. Does that sound right? I worked from the combined patch, so it's not quite the same, but I can rework it as if that patch had been removed if you prefer. (In reply to Jim Mathies from comment #93) > a try build we can run against the tests case in bug 726144 would be great. I'd already run those tests on Windows 7 of course. I tested them on Windows XP and they were all fine except for the undetermined XUL progressmeter, which didn't erase itself. I came up with a hack that fixes that based on the previous undetermined behaviour of having unknown transparency instead of opacity.

Comment on attachment 718946 [details] [diff] [review] Something I got to work sounds good to me if you're comfortable with your patch. we can iron out the nicer indeterminate issue in a follow up bug.

This is a rollup of all the patches in one hg export (sadly hg [q]import won't split them, you have to split them manually yourself first). Try run: https://tbpl.mozilla.org/?tree=Try&rev=d0e27f59a029 Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-d0e27f59a029/try-win32/ The biggest issue with this patch is part 8, which doesn't work properly with small progressmeters (the old patch 8 accidentally worked with indeterminate meters, which is why you didn't notice in the test case).

Try run for d0e27f59a029 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=d0e27f59a029 Results (out of 1 total builds): success: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-d0e27f59a029

http://hg.mozilla.org/integration/mozilla-inbound/rev/fbdbdebeb0dd http://hg.mozilla.org/integration/mozilla-inbound/rev/ca5350cf3b1d http://hg.mozilla.org/integration/mozilla-inbound/rev/00f8d9c10f6b http://hg.mozilla.org/integration/mozilla-inbound/rev/d353df0caf90 http://hg.mozilla.org/integration/mozilla-inbound/rev/41ea14d7f14e http://hg.mozilla.org/integration/mozilla-inbound/rev/4936f2b632ae I accidentally labelled part 6 as part 5 though...

https://hg.mozilla.org/mozilla-central/rev/fbdbdebeb0dd https://hg.mozilla.org/mozilla-central/rev/ca5350cf3b1d https://hg.mozilla.org/mozilla-central/rev/00f8d9c10f6b https://hg.mozilla.org/mozilla-central/rev/d353df0caf90 https://hg.mozilla.org/mozilla-central/rev/41ea14d7f14e https://hg.mozilla.org/mozilla-central/rev/4936f2b632ae

IMHO there's no reason to leave this bug open, the only attachment that didn't get checked in is part 7 which is from bug 642846 anyway.