Closed
Bug 641905
Opened 13 years ago
Closed 13 years ago
Use native rendering for indeterminate progress bar (Windows)
Categories
(Core :: Widget: Win32, defect)
Core
Widget: Win32
Tracking
()
VERIFIED
FIXED
mozilla6
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file, 8 obsolete files)
8.27 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•13 years ago
|
Summary: Use native rendering for indeterminate progress bar (Win) → Use native rendering for indeterminate progress bar (Windows)
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 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•13 years ago
|
||
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•13 years ago
|
||
Attachment #519472 -
Flags: review?(doug.turner)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #519479 -
Flags: review?(doug.turner)
Assignee | ||
Comment 6•13 years ago
|
||
Doug, let me know if there is a place where I should move the constants I'm using.
Comment 7•13 years ago
|
||
this is a pretty big change, we should get jmathies to look at it.
Assignee | ||
Updated•13 years ago
|
Attachment #519471 -
Flags: review?(doug.turner) → review?(jmathies)
Assignee | ||
Updated•13 years ago
|
Attachment #519472 -
Flags: review?(doug.turner) → review?(jmathies)
Assignee | ||
Updated•13 years ago
|
Attachment #519479 -
Flags: review?(doug.turner) → review?(jmathies)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 8•13 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>
Comment 9•13 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•13 years ago
|
||
Also, what's the relationship between these patches and the patches in bug 641517?
Assignee | ||
Comment 11•13 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•13 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•13 years ago
|
||
Attachment #519471 -
Attachment is obsolete: true
Attachment #519471 -
Flags: review?(jmathies)
Attachment #519650 -
Flags: review?(jmathies)
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #519472 -
Attachment is obsolete: true
Attachment #519472 -
Flags: review?(jmathies)
Attachment #519651 -
Flags: review?(jmathies)
Comment 15•13 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•13 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•13 years ago
|
||
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•13 years ago
|
Attachment #519679 -
Flags: review? → review?(jmathies)
Assignee | ||
Comment 18•13 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•13 years ago
|
||
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•13 years ago
|
||
Use constants.
Attachment #519936 -
Attachment is obsolete: true
Attachment #519936 -
Flags: review?(jmathies)
Attachment #519975 -
Flags: review?(jmathies)
Comment 21•13 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•13 years ago
|
Whiteboard: [needs review]
Assignee | ||
Updated•13 years ago
|
Whiteboard: [ready to land][waits for dependencies]
Assignee | ||
Comment 22•13 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/ddfa8bef5859
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6
Comment 23•13 years ago
|
||
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 24•13 years ago
|
||
The regression wasn't caused by these patches. Re-landed: http://hg.mozilla.org/mozilla-central/rev/638dbfdb7fc3
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 25•13 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•13 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•13 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.
Description
•