Use a native rendering for vertical progress bar (Windows)

RESOLVED FIXED in mozilla6

Status

()

Core
Widget: Win32
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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?
(Assignee)

Comment 1

6 years ago
Created attachment 532220 [details] [diff] [review]
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.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #532220 - Flags: review?(jmathies)
(Assignee)

Comment 2

6 years ago
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 :)
Whiteboard: [needs review]

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
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

6 years ago
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.
(Assignee)

Comment 7

6 years ago
(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

6 years ago
Comment on attachment 532220 [details] [diff] [review]
Patch v1

r+ w/the code cleanup previously mentioned.
Attachment #532220 - Flags: review?(jmathies) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 532995 [details] [diff] [review]
Patch to checkin
Attachment #532220 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/cef02273a027
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.