Last Comment Bug 656909 - Use a native rendering for vertical progress bar (Windows)
: Use a native rendering for vertical progress bar (Windows)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
: Jim Mathies [:jimm]
Mentors:
Depends on: 638540
Blocks: 633207
  Show dependency treegraph
 
Reported: 2011-05-13 07:17 PDT by Mounir Lamouri (:mounir)
Modified: 2011-05-17 09:43 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (10.09 KB, patch)
2011-05-13 07:27 PDT, Mounir Lamouri (:mounir)
jmathies: review+
Details | Diff | Splinter Review
Patch to checkin (10.15 KB, patch)
2011-05-17 09:38 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-05-13 07:17:12 PDT
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) 2011-05-13 07:27:23 PDT
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.
Comment 2 Mounir Lamouri (:mounir) 2011-05-13 07:28:30 PDT
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] 2011-05-13 07:49:18 PDT
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) 2011-05-13 09:18:49 PDT
(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) 2011-05-17 06:10:05 PDT
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] 2011-05-17 06:42:28 PDT
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) 2011-05-17 06:48:08 PDT
(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] 2011-05-17 07:28:01 PDT
Comment on attachment 532220 [details] [diff] [review]
Patch v1

r+ w/the code cleanup previously mentioned.
Comment 9 Mounir Lamouri (:mounir) 2011-05-17 09:38:02 PDT
Created attachment 532995 [details] [diff] [review]
Patch to checkin
Comment 10 Mounir Lamouri (:mounir) 2011-05-17 09:43:26 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/cef02273a027

Note You need to log in before you can comment on or make changes to this bug.