[webkit-reviews] review denied: [Bug 37308] [Chromium] Support HTML5 <progress> element on Windows. : [Attachment 53968] v3; took feedback

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 37308: [Chromium] Support HTML5 <progress> element on Windows.
https://bugs.webkit.org/show_bug.cgi?id=37308

Attachment 53968: v3; took feedback
https://bugs.webkit.org/attachment.cgi?id=53968&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ 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.