Use native rendering for indeterminate progress bar (Windows)

VERIFIED FIXED in mozilla6

Status

()

Core
Widget: Win32
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Depends on: 1 bug)

Trunk
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

7 years ago
Summary: Use native rendering for indeterminate progress bar (Win) → Use native rendering for indeterminate progress bar (Windows)
(Assignee)

Comment 1

7 years ago
Created attachment 519464 [details] [diff] [review]
Part 1 - Windows XP.
(Assignee)

Comment 2

7 years ago
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.
Attachment #519464 - Attachment description: Part 1 - i → Part 1 - Windows XP.
Attachment #519464 - Attachment is patch: true
Attachment #519464 - Attachment mime type: application/octet-stream → text/plain
Attachment #519464 - Flags: review?(doug.turner)
(Assignee)

Comment 3

7 years ago
Created attachment 519471 [details] [diff] [review]
Part 1 - Windows XP

Updating the patch to fix a warning.
Attachment #519464 - Attachment is obsolete: true
Attachment #519464 - Flags: review?(doug.turner)
Attachment #519471 - Flags: review?(doug.turner)
(Assignee)

Comment 4

7 years ago
Created attachment 519472 [details] [diff] [review]
Part 2 - Windows Vista/7
Attachment #519472 - Flags: review?(doug.turner)
(Assignee)

Comment 5

7 years ago
Created attachment 519479 [details] [diff] [review]
Part 3 - Refactor and add comments
Attachment #519479 - Flags: review?(doug.turner)
(Assignee)

Comment 6

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

Comment 7

7 years ago
this is a pretty big change, we should get jmathies to look at it.
(Assignee)

Updated

7 years ago
Attachment #519471 - Flags: review?(doug.turner) → review?(jmathies)
(Assignee)

Updated

7 years ago
Attachment #519472 - Flags: review?(doug.turner) → review?(jmathies)
(Assignee)

Updated

7 years ago
Attachment #519479 - Flags: review?(doug.turner) → review?(jmathies)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review]
(Assignee)

Comment 8

7 years ago
If you want to compare the rendering, please use this build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-d171ffeea1be/ (before)
and this one:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-3665ab582961/ (after)

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

Updated

7 years ago
Blocks: 641942

Comment 9

7 years ago
(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/mlamouri@mozilla.com-d171ffeea1be/
> (before)
> and this one:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-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

7 years ago
Also, what's the relationship between these patches and the patches in bug 641517?
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

7 years ago
Created attachment 519650 [details] [diff] [review]
Part 1 - Windows XP
Attachment #519471 - Attachment is obsolete: true
Attachment #519471 - Flags: review?(jmathies)
Attachment #519650 - Flags: review?(jmathies)
(Assignee)

Comment 14

7 years ago
Created attachment 519651 [details] [diff] [review]
Part 2 - Windows Vista/7
Attachment #519472 - Attachment is obsolete: true
Attachment #519472 - Flags: review?(jmathies)
Attachment #519651 - Flags: review?(jmathies)

Comment 15

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

Comment 16

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

Comment 17

7 years ago
Created attachment 519679 [details] [diff] [review]
Patch v1

Folded patch.
Attachment #519479 - Attachment is obsolete: true
Attachment #519650 - Attachment is obsolete: true
Attachment #519651 - Attachment is obsolete: true
Attachment #519479 - Flags: review?(jmathies)
Attachment #519650 - Flags: review?(jmathies)
Attachment #519651 - Flags: review?(jmathies)
Attachment #519679 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #519679 - Flags: review? → review?(jmathies)
(Assignee)

Comment 18

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

Comment 19

7 years ago
Created attachment 519936 [details] [diff] [review]
Patch v1.1

Small tweak with the delay between two cycles.
Attachment #519679 - Attachment is obsolete: true
Attachment #519679 - Flags: review?(jmathies)
Attachment #519936 - Flags: review?(jmathies)
(Assignee)

Comment 20

7 years ago
Created attachment 519975 [details] [diff] [review]
Patch v1.2

Use constants.
Attachment #519936 - Attachment is obsolete: true
Attachment #519936 - Flags: review?(jmathies)
Attachment #519975 - Flags: review?(jmathies)
(Assignee)

Updated

7 years ago
Depends on: 642846

Comment 21

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

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

r+
Attachment #519975 - Flags: review?(jmathies) → review+

Updated

6 years ago
Whiteboard: [needs review]
(Assignee)

Updated

6 years ago
Whiteboard: [ready to land][waits for dependencies]
(Assignee)

Comment 22

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/ddfa8bef5859
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6

Updated

6 years ago
Depends on: 655860
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 24

6 years ago
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/638dbfdb7fc3
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
No longer depends on: 655860

Comment 25

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

Comment 26

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

6 years ago
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.