641517 - Have a native rendering for <progress> on Windows Vista and Windows 7

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Use PP_FILL instead of PP_CHUNK

Comment 1

[Doug Turner (:dougt)]

what is the purpose of this change?

does it make sense to put up a before/after screenshot?

Comment 2

[Mounir Lamouri (:mounir)]

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)]

Attachment: 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)]

Attachment: Part 1 - Use PP_FILL instead of PP_CHUNK

Fix a warning.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: 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)]

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 value=1></progress>

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: 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)]

Attachment: Part 2 - Add the animated glow

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Add the animated glow

Comment 10

[Jim Mathies [:jimm]]

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]]

Comment on attachment 519980 [details] [diff] [review]
Part 2 - Add the animated glow

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

r+

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/f8ab655d503c
http://hg.mozilla.org/mozilla-central/rev/4a7b8019b83f

Comment 13

[Shawn Wilsher :sdwilsh]

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

Comment 14

[Mounir Lamouri (:mounir)]

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 16

[George Carstoiu]

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)]

(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]

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

Considering last comment, setting status to Verified Fixed.