Bug 39211

Summary: [Chromium] Windows: Speed of indeterminate progress bar should be constant
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
patch v0
none
patch v1 none

Hajime Morrita
Reported 2010-05-17 02:45:06 PDT
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.
Attachments
patch v0 (2.87 KB, patch)
2010-05-17 03:07 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
patch v1 (3.03 KB, patch)
2010-05-17 03:58 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2010-05-17 03:07:28 PDT
Created attachment 56229 [details] patch v0
Hajime Morrita
Comment 2 2010-05-17 03:09:40 PDT
Kent-san, could you review this one?
Kent Tamura
Comment 3 2010-05-17 03:27:52 PDT
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
Comment 4 2010-05-17 03:30:41 PDT
(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
Comment 5 2010-05-17 03:58:38 PDT
Created attachment 56232 [details] patch v1
Hajime Morrita
Comment 6 2010-05-17 04:00:41 PDT
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 7 2010-05-17 04:38:57 PDT
Comment on attachment 56232 [details] patch v1 OK.
Hajime Morrita
Comment 8 2010-05-17 18:29:07 PDT
Comment on attachment 56232 [details] patch v1 Clearing flags on attachment: 56232 Committed r59635: <http://trac.webkit.org/changeset/59635>
Hajime Morrita
Comment 9 2010-05-17 18:29:16 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.