656909 - Use a native rendering for vertical progress bar (Windows)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Mounir Lamouri (:mounir)]

I will try to have a render that looks like the horizontal one but vertical.
Jim, do you know of any Windows app using vertical progress bars?

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

As said in a comment, it doesn't look as nice as horizontal progress bars. I suppose not a lot of people are using vertical progress bars...
There is also an overflow on the right but I will try to fix that in another bug.

Comment 2

[Mounir Lamouri (:mounir)]

By the way, Jim, it's the last patch blocking vertical progress bar to be pushed in the tree. It would be great if you can review it before Firefox 6 branching :)

Comment 3

[Jim Mathies [:jimm]]

Shouldn't we mark this as dependent of bug 656284? I'd really like to see that fixed before we land more progress meter work.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> Shouldn't we mark this as dependent of bug 656284? I'd really like to see
> that fixed before we land more progress meter work.

I don't think we should worry about that. This patch isn't making the issue in bug 656284 worse or better. I think you can review it safely and I might wait for bug 656284 to be fixed before pushing. I want both patches to be in Firefox 6.

Comment 5

[Mounir Lamouri (:mounir)]

Jim, now that bug 656284 is fixed, I'm going to push the vertical progress bar support on trunk. It would be nice if you could make sure the review of the patch is done this week so I can push it before Firefox 6 branching.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 532220 [details] [diff] [review]
Patch v1

Review of attachment 532220 [details] [diff] [review]:
-----------------------------------------------------------------

Do the tests you've written previously cover this new progress alignment?

::: widget/src/windows/nsNativeThemeWin.cpp
@@ +1590,5 @@
> +                                       ? indeterminate
> +                                         ? kProgressVerticalIndeterminateOverlaySize
> +                                         : kProgressVerticalOverlaySize
> +                                       : kProgressHorizontalVistaOverlaySize
> +                                     : kProgressHorizontalXPOverlaySize;

This is getting hard to read, can you break this out into a set of bracketed if statements.

@@ +1633,2 @@
>                                   state, &overlayRect, &clipRect);
>      }

Same thing here.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> Comment on attachment 532220 [details] [diff] [review] [review]
> Patch v1
> 
> Review of attachment 532220 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Do the tests you've written previously cover this new progress alignment?

There is no windows specific test because on Windows Vista and 7 the progress element is animated thus can't be used in a reftest.
But they are tests covering that for non-native progress bars.


> ::: widget/src/windows/nsNativeThemeWin.cpp
> @@ +1590,5 @@
> > +                                       ? indeterminate
> > +                                         ? kProgressVerticalIndeterminateOverlaySize
> > +                                         : kProgressVerticalOverlaySize
> > +                                       : kProgressHorizontalVistaOverlaySize
> > +                                     : kProgressHorizontalXPOverlaySize;
> 
> This is getting hard to read, can you break this out into a set of bracketed
> if statements.

Sure, will do.

> @@ +1633,2 @@
> >                                   state, &overlayRect, &clipRect);
> >      }
> 
> Same thing here.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 532220 [details] [diff] [review]
Patch v1

r+ w/the code cleanup previously mentioned.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch to checkin

Comment 10

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/cef02273a027