Last Comment Bug 641517 - Have a native rendering for <progress> on Windows Vista and Windows 7
: Have a native rendering for <progress> on Windows Vista and Windows 7
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
: 378276 (view as bug list)
Depends on: 568825 656592 711962
Blocks: 633207 641905 641942 642127
  Show dependency treegraph
 
Reported: 2011-03-14 08:45 PDT by Mounir Lamouri (:mounir)
Modified: 2011-12-19 06:56 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Use PP_FILL instead of PP_CHUNK (2.26 KB, patch)
2011-03-14 08:45 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Before/After screenshot (7.92 KB, image/png)
2011-03-15 06:41 PDT, Mounir Lamouri (:mounir)
no flags Details
Part 1 - Use PP_FILL instead of PP_CHUNK (2.36 KB, patch)
2011-03-15 10:07 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Add the animated glow (3.31 KB, patch)
2011-03-15 10:12 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 1 - Use PP_FILL instead of PP_CHUNK (3.13 KB, patch)
2011-03-17 11:52 PDT, Mounir Lamouri (:mounir)
jmathies: review+
Details | Diff | Splinter Review
Part 2 - Add the animated glow (4.28 KB, patch)
2011-03-17 11:53 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Add the animated glow (4.01 KB, patch)
2011-03-17 12:04 PDT, Mounir Lamouri (:mounir)
jmathies: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-03-14 08:45:17 PDT
Created attachment 519149 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK
Comment 1 Doug Turner (:dougt) 2011-03-14 20:41:27 PDT
what is the purpose of this change?

does it make sense to put up a before/after screenshot?
Comment 2 Mounir Lamouri (:mounir) 2011-03-15 06:16:15 PDT
The purpose is to use the correct Windows Vista/7 API for progress bars instead of the XP one. Our rendering of the progress bar on Windows Vista/7 isn't really native for the moment. You can see that by downloading a file on Windows Vista/7 and look at the rendering. Then, look at what a native progress bar look like.

I will try to put some screenshots...
Comment 3 Mounir Lamouri (:mounir) 2011-03-15 06:41:57 PDT
Created attachment 519391 [details]
Before/After screenshot

The difference might not be really obvious on the screenshot because there is an animated glow on progress bars on Windows Vista/7. You can see that by openning this data URL on Chrome: data:text/html,<progress value=1></progress>
Comment 4 Mounir Lamouri (:mounir) 2011-03-15 10:07:08 PDT
Created attachment 519435 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK

Fix a warning.
Comment 5 Mounir Lamouri (:mounir) 2011-03-15 10:12:29 PDT
Created attachment 519436 [details] [diff] [review]
Part 2 - Add the animated glow

Unfortunately, our code isn't great for animations. To prevent keeping too many static variables, I'm only using PR_IntervalNow(). That means the position of the glow when the page is loaded isn't guaranteed to be at the left of the element.
In addition, all progress bars will share the same glow position. That means if you create multiple progress bars with the same size, the glow will be perfectly synchronized (even if some progress bars are added later).

I will refactor this code and create proper constants when I will implement the indeterminate rendering.
Comment 6 Mounir Lamouri (:mounir) 2011-03-15 13:09:41 PDT
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 value=1></progress>
Comment 7 Mounir Lamouri (:mounir) 2011-03-17 11:52:32 PDT
Created attachment 519973 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK

Use consts and moving the review to Jim so he can review all the patches involved here.
Comment 8 Mounir Lamouri (:mounir) 2011-03-17 11:53:11 PDT
Created attachment 519974 [details] [diff] [review]
Part 2 - Add the animated glow
Comment 9 Mounir Lamouri (:mounir) 2011-03-17 12:04:53 PDT
Created attachment 519980 [details] [diff] [review]
Part 2 - Add the animated glow
Comment 10 Jim Mathies [:jimm] 2011-04-25 04:53:12 PDT
Comment on attachment 519973 [details] [diff] [review]
Part 1 - Use PP_FILL instead of PP_CHUNK

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

r+
Comment 11 Jim Mathies [:jimm] 2011-04-25 04:58:20 PDT
Comment on attachment 519980 [details] [diff] [review]
Part 2 - Add the animated glow

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

r+
Comment 13 Shawn Wilsher :sdwilsh 2011-05-09 16:12:09 PDT
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Comment 14 Mounir Lamouri (:mounir) 2011-05-10 07:01:17 PDT
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/66bf0a9f32b2
http://hg.mozilla.org/mozilla-central/rev/75ced66ef579
Comment 15 Jim Mathies [:jimm] 2011-05-24 14:05:03 PDT
*** Bug 378276 has been marked as a duplicate of this bug. ***
Comment 16 George Carstoiu 2011-07-20 06:55:53 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

Verified patch on Win7 and animation is present -  behavior is a bit different from what can be observed in Chrome, the animation(the glow) being slower on Firefox. Was that expected?
Comment 17 Mounir Lamouri (:mounir) 2011-07-20 09:18:23 PDT
(In reply to comment #16)
> Verified patch on Win7 and animation is present -  behavior is a bit
> different from what can be observed in Chrome, the animation(the glow) being
> slower on Firefox. Was that expected?

Our goal is certainly not to copy all Chrome bits...
Comment 18 George Carstoiu 2011-07-26 02:14:35 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110725 Firefox/8.0a1

Considering last comment, setting status to Verified Fixed.

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