Bug 37308

Created attachment 52939 [details] patch v0

Attachment 52939 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/rendering/RenderThemeChromiumWin.h:91: Tab found; better to use spaces [whitespace/tab] [1] WebCore/rendering/RenderThemeChromiumWin.h:92: Tab found; better to use spaces [whitespace/tab] [1] WebCore/rendering/RenderThemeChromiumWin.h:93: Tab found; better to use spaces [whitespace/tab] [1] WebCore/rendering/RenderThemeChromiumWin.h:94: Tab found; better to use spaces [whitespace/tab] [1] WebCore/rendering/RenderThemeChromiumWin.cpp:42: Alphabetical sorting problem. [build/include_order] [4] WebCore/rendering/RenderThemeChromiumWin.cpp:669: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/rendering/RenderThemeChromiumWin.cpp:687: Missing spaces around / [whitespace/operators] [3] Total errors found: 7 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 52941 [details] v1; fix style violation

Change on Chromium side is: http://codereview.chromium.org/1596018

Comment on attachment 52941 [details] v1; fix style violation > --- a/WebCore/rendering/RenderThemeChromiumWin.cpp > +++ b/WebCore/rendering/RenderThemeChromiumWin.cpp > @@ -38,6 +38,7 @@ > #include "HTMLNames.h" > #include "MediaControlElements.h" > #include "RenderBox.h" > +#include "RenderProgress.h" > #include "RenderSlider.h" > #include "ScrollbarTheme.h" > #include "TransparencyWin.h" > @@ -653,4 +654,55 @@ bool RenderThemeChromiumWin::paintTextFieldInternal(RenderObject* o, > return false; > } > > +#if ENABLE(PROGRESS_TAG) > + > +static const double progressAnimationFrameRate = 0.033; > +static const double progressAnimationNumFrames = 60; How did you decide these values? Do they match to Windows' behavior? > --- a/WebKit/chromium/ChangeLog > +++ b/WebKit/chromium/ChangeLog > @@ -1,3 +1,19 @@ > +2010-04-08 MORITA Hajime <morrita@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Chromium] Support HTML5 <progress> element on Windows. > + https://bugs.webkit.org/show_bug.cgi?id=37308 > + > + Extended ChromiumBridge and WebThemeEngine interface > + to handle progress bar painting. implementation will > + shit on Chromium tree. I don't understand the last sentence. shit?

Created attachment 53905 [details] v2; fix to add rtl handling

Hi, thank you for reviewing. I'm sorry for my slow response. > > +#if ENABLE(PROGRESS_TAG) > > + > > +static const double progressAnimationFrameRate = 0.033; > > +static const double progressAnimationNumFrames = 60; > > How did you decide these values? Do they match to Windows' behavior? I couldn't found "standard" behaviour for windows. The guideline doesn't mention marquee speed, (http://msdn.microsoft.com/en-us/library/aa511258.aspx) Nor default value of ProgressBar class on .NET framework looks unrealistic. > > + [Chromium] Support HTML5 <progress> element on Windows. > > + https://bugs.webkit.org/show_bug.cgi?id=37308 > > + > > + Extended ChromiumBridge and WebThemeEngine interface > > + to handle progress bar painting. implementation will > > + shit on Chromium tree. > > I don't understand the last sentence. shit? Ah, this didn't make sense... Fixed.

(In reply to comment #7) > Hi, thank you for reviewing. I'm sorry for my slow response. > > > > +#if ENABLE(PROGRESS_TAG) > > > + > > > +static const double progressAnimationFrameRate = 0.033; > > > +static const double progressAnimationNumFrames = 60; > > > > How did you decide these values? Do they match to Windows' behavior? > I couldn't found "standard" behaviour for windows. > The guideline doesn't mention marquee speed, > (http://msdn.microsoft.com/en-us/library/aa511258.aspx) > Nor default value of ProgressBar class on .NET framework looks unrealistic. So, please add a comment about why you chose 0.033 and 60, and add FIXME if you think we need to improve them. BTW, http://msdn.microsoft.com/en-us/library/bb760842(v=VS.85).aspx It seems the default interval is 30 msec. So progressAnimationFrameRate=0.033 is reasonable.

Comment on attachment 53905 [details] v2; fix to add rtl handling > +double RenderThemeChromiumWin::animationDurationForProgressBar(RenderProgress* o) const I'm not sure if the name 'o' is good for an object of RenderProgress. I would say renderProgress or something like this. > + if (o->position() < 0) I couldn't understand why this is necessary. If this is necessary, don't we need to modfify RenderThemeMac as well? Also, it would be nice to add a function which returns position()<0 in RenderProgress (maybe isIndeterminate or something like this?). Anyway, I guess we want some refactoring around this patch, but in a separated patch. > + int fillPart; > + IntRect fillRect; > + > + Unnecessary blank line(s). I guess we need no blanklines here. > + int dx = r.width() * renderProgress->animationProgress() * 2.0 - r.width(); Maybe we should use 2 instead of 2.0. (See Floating point literals in http://webkit.org/coding/coding-style.html) r- for Tamura-san's comment and style issues.

Created attachment 53968 [details] v3; took feedback

Hamaji-san, Tamura-san, thank you for reviewing. (In reply to comment #8) > So, please add a comment about why you chose 0.033 and 60, and add FIXME if you > think we need to improve them. Added a comment. > > BTW, > http://msdn.microsoft.com/en-us/library/bb760842(v=VS.85).aspx > It seems the default interval is 30 msec. So progressAnimationFrameRate=0.033 > is reasonable. Thank you for your investigation! I included this link in the comment. (In reply to comment #9) > > +double RenderThemeChromiumWin::animationDurationForProgressBar(RenderProgress* o) const > I'm not sure if the name 'o' is good for an object of RenderProgress. I would > say renderProgress or something like this. Fixed. > > > + if (o->position() < 0) > > I couldn't understand why this is necessary. If this is necessary, don't we > need to modfify RenderThemeMac as well? Progress bars on windows bar has an animation only when it is indeterminate. Progress bars on mac has animations both for determinate and for indeterminate. So we need this only for Windows port. > > + int fillPart; > > + IntRect fillRect; > > + > > + > > Unnecessary blank line(s). I guess we need no blanklines here. Fixed. > > > + int dx = r.width() * renderProgress->animationProgress() * 2.0 - r.width(); > > Maybe we should use 2 instead of 2.0. (See Floating point literals in > http://webkit.org/coding/coding-style.html) Fixed.

The patch seems outdated. I'm rebasing it now.

Comment on attachment 53968 [details] v3; took feedback > +++ b/WebCore/rendering/RenderThemeChromiumWin.cpp ... > +bool RenderThemeChromiumWin::paintProgressBar(RenderObject* o, const RenderObject::PaintInfo& i, const IntRect& r) > +{ > + RenderProgress* renderProgress = toRenderProgress(o); > + > + int fillPart; > + IntRect fillRect; > + if (renderProgress->isDeterminate()) { > + fillPart = PP_FILL; > + int dx = r.width() * renderProgress->position(); > + if (renderProgress->style()->direction() == RTL) > + fillRect = IntRect(r.x() + r.width() - dx, r.y(), dx, r.height()); > + else > + fillRect = IntRect(r.x(), r.y(), dx, r.height()); > + } else { > + fillPart = PP_MOVEOVERLAY; > + int dx = r.width() * renderProgress->animationProgress() * 2 - r.width(); > + fillRect = IntRect(r.x() + dx, r.y(), r.width(), r.height()); > + } > + > + WebCore::ThemePainter painter(i.context, r); nit: no need for the WebCore:: prefix when you are in the WebCore namespace. > + ChromiumBridge::paintProgressBar(painter.context(), > + PP_BAR, > + 0, > + r, > + fillPart, > + 0, > + fillRect); It looks like barState and fillState are not used. Should those parameters be excluded? Also, is there any reason to pass PP_BAR given that it is a constant? It seems like the callee would know to pass PP_BAR on to Windows. > +++ b/WebKit/chromium/public/win/WebThemeEngine.h ... > @@ -73,6 +73,10 @@ public: > virtual void paintTrackbar( > WebCanvas*, int part, int state, int classicState, > const WebRect&) = 0; > + > + virtual void paintProgressBar( > + WebCanvas*, int barPart, int barState, const WebRect& barRect, > + int fillPart, int fillState, const WebRect& fillRect) {} // FIXME: make pure virtual after implementation landed. > }; ^^^ we should actually make any embedder implemented methods non-pure. the old way was for them to be pure, but that makes it harder to change the API. the new way is to make methods like these non-pure.

Created attachment 54011 [details] v4; took the feedback, rebased

Darin, thank you for reviewing! I updated the patch. > > + WebCore::ThemePainter painter(i.context, r); > > nit: no need for the WebCore:: prefix when you are in the WebCore namespace. Fixed. > > + ChromiumBridge::paintProgressBar(painter.context(), > > + PP_BAR, > > + 0, > > + r, > > + fillPart, > > + 0, > > + fillRect); > > It looks like barState and fillState are not used. Should those > parameters be excluded? Also, is there any reason to pass PP_BAR > given that it is a constant? It seems like the callee would know > to pass PP_BAR on to Windows. Agreed and fixed. I try to align args to other APIs. But it didn't make sense in this case. > > + virtual void paintProgressBar( > > + WebCanvas*, int barPart, int barState, const WebRect& barRect, > > + int fillPart, int fillState, const WebRect& fillRect) {} // FIXME: make pure virtual after implementation landed. > > }; > > ^^^ we should actually make any embedder implemented methods non-pure. > the old way was for them to be pure, but that makes it harder to change > the API. the new way is to make methods like these non-pure. I see. So I removed the FIXME. We might need to export notImplemented() for such case.

(In reply to comment #11) > > > + if (o->position() < 0) > > > > I couldn't understand why this is necessary. If this is necessary, don't we > > need to modfify RenderThemeMac as well? > Progress bars on windows bar has an animation only when it is indeterminate. > Progress bars on mac has animations both for determinate and for indeterminate. > So we need this only for Windows port. I remember determinate progress bar on Windows is also animated. A white gloss run from left to right. Can you implement it? (In reply to comment #15) > > > + virtual void paintProgressBar( > > > + WebCanvas*, int barPart, int barState, const WebRect& barRect, > > > + int fillPart, int fillState, const WebRect& fillRect) {} // FIXME: make pure virtual after implementation landed. > > > }; > > > > ^^^ we should actually make any embedder implemented methods non-pure. > > the old way was for them to be pure, but that makes it harder to change > > the API. the new way is to make methods like these non-pure. > I see. So I removed the FIXME. > We might need to export notImplemented() for such case. Removing FIXME is worse. In this case, you had better commit the Chromium part first, then commit this change.

(In reply to comment #16) > Removing FIXME is worse. > In this case, you had better commit the Chromium part first, then commit this > change. This comment was wrong. Please ignore it.

Hi, thank you for reviewing! > I remember determinate progress bar on Windows is also animated. A white gloss > run from left to right. Can you implement it? It seems that animating-determinate-progress-bar is available only on Windows 7, not on Vista. So I'll keep it not animated at this time, and will implement animation when I get Windows 7.

(In reply to comment #18) > It seems that animating-determinate-progress-bar is available only on Windows > 7, not on Vista. > So I'll keep it not animated at this time, and will implement animation when I > get Windows 7. It is available on Vista too. Anyway it's ok to handle it later. It's good to file a bug entry for it now. The code looks ok.

Comment on attachment 54011 [details] v4; took the feedback, rebased OK.

Committed r59217: <http://trac.webkit.org/changeset/59217>

Why is progress bar painting being done with calls to the Chrome bridge? Is there some reason this needs to be done outside WebKit?

(In reply to comment #22) > Why is progress bar painting being done with calls to the Chrome bridge? Is there some reason this needs to be done outside WebKit? Progress bar appearance depends to a desktop theme. In Chromium port, we can't access desktop themes in WebKit. This is a Chromium-specific issue. So RenderThemeWin.cpp can have rendering code for native progress bar.