641905 - Use native rendering for indeterminate progress bar (Windows)

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Windows XP.

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 519464 [details] [diff] [review]
Part 1 - Windows XP.

I'm going to add a second patch for Vista/7 and a third one to refactor.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Windows XP

Updating the patch to fix a warning.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Windows Vista/7

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Refactor and add comments

Comment 6

[Mounir Lamouri (:mounir)]

Doug, let me know if there is a place where I should move the constants I'm using.

Comment 7

[Doug Turner (:dougt)]

this is a pretty big change, we should get jmathies to look at it.

Comment 8

[Mounir Lamouri (:mounir)]

If you want to compare the rendering, please use this build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d171ffeea1be/ (before)
and this one:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3665ab582961/ (after)

And open this data url: data:text/html,<progress></progress>

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #8)
> If you want to compare the rendering, please use this build:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d171ffeea1be/
> (before)
> and this one:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3665ab582961/
> (after)
> 
> And open this data url: data:text/html,<progress></progress>

Is this supposed to be working (at all) on a nightly? All I currently get is a value w/a percent sign on example sites like w3schools.

Also, are these three patches all I need to get this working in a local build?

Comment 10

[Jim Mathies [:jimm]]

Also, what's the relationship between these patches and the patches in bug 641517?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> Also, what's the relationship between these patches and the patches in bug
> 641517?

They are wrote on top of them. Actually, feel free to steal the reviews from bug 641517 given that it might make reviewing the patches here easier.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> Is this supposed to be working (at all) on a nightly? All I currently get is a
> value w/a percent sign on example sites like w3schools.
> 
> Also, are these three patches all I need to get this working in a local build?

No, it's not supposed to work on a nightly. I think the easiest solution is to use the builds linked above. But if you want to do your own build, I would recommend using my patch queue which is available here:
https://hg.mozilla.org/users/mlamouri_mozilla.com/html5forms-patchqueue/

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Windows XP

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Windows Vista/7

Comment 15

[Jim Mathies [:jimm]]

I would suggest rolling the patches in bug 641905, bug 568825, and bug 641517 into a single set for review. I had to apply all of them to get to a point where I was reviewing the actual code you want to land. Once you put them together it's not that bug of a patch. Individual reviews on each patch are a little tough since every subsequent patch change the previous one in some way. This would cut down on the time reviewers have to spend on this.

Some comments on a rolled up change set:

+      aPart = IsIndeterminateProgress(stateFrame, eventStates)
+                ? -1 : nsUXThemeData::sIsVistaOrLater ? PP_FILL : PP_CHUNK;

The headers also define vertical constants for these. Do we plan to add support later for our vertical progress theme types? (NS_THEME_PROGRESSBAR_VERTICAL, NS_THEME_PROGRESSBAR_CHUNK_VERTICAL)

+    widgetRect.bottom -= nsUXThemeData::sIsVistaOrLater ? 4 : 11;

Doug asked about these too. Can we figure out why this overlaps? I'm guessing the hdc we get expands beyond the size of the progress. Seems rather odd that on two different os we get different overflow. I don't see anything in nsNativeThemeWin that could cause this, but I'm curious due to the differences.

+    /**
+     * Here, we draw the animated part of the progress bar.
+     * A progress bar has always an animated part on Windows Vista and later.
+     * On Windows XP, a progress bar has an animated part when in an
+     * indeterminated state.
+     * When the progress bar is indeterminated, no background is painted so we
+     * only see the animated part.
+     * When the progress bar is determinated, the animated part is a glow draw
+     * on top of the background (PP_FILL).
+     */

Please clean this up into a nice block comment.

+      static const PRInt32 overlayWidth = nsUXThemeData::sIsVistaOrLater ? 120
+                                                                         : 55;
+      static const double pixelsPerMillisecond = indeterminate ? 0.175 : 0.3;

Let's move these to defines and comment them explaining what they represent.

+//#define PP_FILLVERT           6
+//#define PP_PULSEOVERLAY       7
+#define PP_MOVEOVERLAY        8
+//#define PP_PULSEOVERLAYVERT   9
+//#define PP_MOVEOVERLAYVERT    8
+//#define PP_TRANSPARENTBAR     11
+//#define PP_TRANSPARENTBARVERT 12

Please remove the constants you don't need.

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> I would suggest rolling the patches in bug 641905, bug 568825, and bug 641517
> into a single set for review. I had to apply all of them to get to a point
> where I was reviewing the actual code you want to land. Once you put them
> together it's not that bug of a patch. Individual reviews on each patch are a
> little tough since every subsequent patch change the previous one in some way.
> This would cut down on the time reviewers have to spend on this.

I agree that the three patches could be merged: that would make things easier. Though, I don't really like big patches doing everything.

> Some comments on a rolled up change set:
> 
> +      aPart = IsIndeterminateProgress(stateFrame, eventStates)
> +                ? -1 : nsUXThemeData::sIsVistaOrLater ? PP_FILL : PP_CHUNK;
> 
> The headers also define vertical constants for these. Do we plan to add support
> later for our vertical progress theme types? (NS_THEME_PROGRESSBAR_VERTICAL,
> NS_THEME_PROGRESSBAR_CHUNK_VERTICAL)

Yes.

> +    widgetRect.bottom -= nsUXThemeData::sIsVistaOrLater ? 4 : 11;
> 
> Doug asked about these too. Can we figure out why this overlaps? I'm guessing
> the hdc we get expands beyond the size of the progress. Seems rather odd that
> on two different os we get different overflow. I don't see anything in
> nsNativeThemeWin that could cause this, but I'm curious due to the differences.

Actually, I realized that these values change depending of some context. For example, the PP_CHUNK has an overflow of 11 when draw on the entire frame but only 2 when draw for the indeterminated state. The same way, PP_MOVEOVERLAY has an overflow of 2 if draw alone (indeterminate state) but no overflow when draw after PP_FILL (which has an overflow of 4). That's really weird.

> +    /**
> +     * Here, we draw the animated part of the progress bar.
> +     * A progress bar has always an animated part on Windows Vista and later.
> +     * On Windows XP, a progress bar has an animated part when in an
> +     * indeterminated state.
> +     * When the progress bar is indeterminated, no background is painted so we
> +     * only see the animated part.
> +     * When the progress bar is determinated, the animated part is a glow draw
> +     * on top of the background (PP_FILL).
> +     */
> 
> Please clean this up into a nice block comment.

To me, it's already a "nice comment block". What do you mean?

> +      static const PRInt32 overlayWidth = nsUXThemeData::sIsVistaOrLater ? 120
> +                                                                         : 55;
> +      static const double pixelsPerMillisecond = indeterminate ? 0.175 : 0.3;
> 
> Let's move these to defines and comment them explaining what they represent.

Is there a place recommended to add these constants?

> +//#define PP_FILLVERT           6
> +//#define PP_PULSEOVERLAY       7
> +#define PP_MOVEOVERLAY        8
> +//#define PP_PULSEOVERLAYVERT   9
> +//#define PP_MOVEOVERLAYVERT    8
> +//#define PP_TRANSPARENTBAR     11
> +//#define PP_TRANSPARENTBARVERT 12
> 
> Please remove the constants you don't need.

I've added them because I will need some of them later and it's a pain to found the values...

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Folded patch.

Comment 18

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> +    widgetRect.bottom -= nsUXThemeData::sIsVistaOrLater ? 4 : 11;
> 
> Doug asked about these too. Can we figure out why this overlaps? I'm guessing
> the hdc we get expands beyond the size of the progress. Seems rather odd that
> on two different os we get different overflow. I don't see anything in
> nsNativeThemeWin that could cause this, but I'm curious due to the differences.

BTW, it has nothing to do with the OS's but with PP_CHUNK and PP_FILL. We actually use PP_CHUNK prior vista and PP_FILL after.

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Small tweak with the delay between two cycles.

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

Use constants.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 519975 [details] [diff] [review]
Patch v1.2

Review of attachment 519975 [details] [diff] [review]:

r+

Comment 22

[Mounir Lamouri (:mounir)]

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

Comment 23

[Shawn Wilsher :sdwilsh]

Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.

Comment 24

[Mounir Lamouri (:mounir)]

The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/638dbfdb7fc3

Comment 25

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Do you have some steps to reproduce in order for me to verify whether this issue was fixed or not?

I know it's something to do with data:text/html,<progress></progress> - but not quite sure what.

Thanks!

Comment 26

[Mounir Lamouri (:mounir)]

(In reply to comment #25)
> Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0
> 
> Do you have some steps to reproduce in order for me to verify whether this
> issue was fixed or not?
> 
> I know it's something to do with data:text/html,<progress></progress> - but
> not quite sure what.

data:text/html,<progress></progress> should show an indeterminate progress bar, that means a progress bar with a moving chunk. Details depending on your Windows version.

Comment 27

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110725 Firefox/8.0a1

Considering last comment, setting status to Verified Fixed. Thanks Mounir!