Bug 39211 – [Chromium] Windows: Speed of indeterminate progress bar should be constant

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Hajime Morrita]

On Chromium, animation speed for indeterminate progress bar changes varies by the size of the bar.
On windows, indeterminate progress bar shows animation should be constant speed 
Regardless the size of the bar.

[Hajime Morrita]

Created attachment 56229 [details]
patch v0

[Hajime Morrita]

Kent-san, could you review this one?

[Kent Tamura]

Comment on attachment 56229 [details]
patch v0

WebCore/rendering/RenderThemeChromiumWin.cpp:664
 +  static const double progressAnimationNumFrames = 60;
If this value is not used anymore, remove it.


WebCore/rendering/RenderThemeChromiumWin.cpp:665
 +  static const double progressIndeterminateOverlayDelta =  175;
Could you add a comment why this value is 175 please?
The symbol name wasn't clear to me at first glance.  I recommend to make it contain "DeltaPerFrame" or "PixelsPerFrame".


WebCore/rendering/RenderThemeChromiumWin.cpp:667
 +  static const int progressIndeterminateOverlayWidth = 120;
ditto

[Kent Tamura]

(In reply to comment #3)
> WebCore/rendering/RenderThemeChromiumWin.cpp:667
>  +  static const int progressIndeterminateOverlayWidth = 120;
> ditto

I meant ditto for adding a comment.
The symbol name is ok.

[Hajime Morrita]

Created attachment 56232 [details]
patch v1

[Hajime Morrita]

Hi kent-san, thank you for reviewing!

(In reply to comment #3)
> (From update of attachment 56229 [details])
> WebCore/rendering/RenderThemeChromiumWin.cpp:664
>  +  static const double progressAnimationNumFrames = 60;
> If this value is not used anymore, remove it.
Fixed.

> 
> 
> WebCore/rendering/RenderThemeChromiumWin.cpp:665
>  +  static const double progressIndeterminateOverlayDelta =  175;
> Could you add a comment why this value is 175 please?
> The symbol name wasn't clear to me at first glance.  I recommend to make it contain "DeltaPerFrame" or "PixelsPerFrame".
Fixed to rename it to progressIndeterminateOverlayPixelsPerSecond.

> 
> 
> WebCore/rendering/RenderThemeChromiumWin.cpp:667
>  +  static const int progressIndeterminateOverlayWidth = 120;
Fixed to remove underling blank line to indicate comment coverage.

[Kent Tamura]

Comment on attachment 56232 [details]
patch v1

OK.

[Hajime Morrita]

Comment on attachment 56232 [details]
patch v1

Clearing flags on attachment: 56232

Committed r59635: <http://trac.webkit.org/changeset/59635>

[Hajime Morrita]

All reviewed patches have been landed.  Closing bug.