[webkit-reviews] review denied: [Bug 37308] [Chromium] Support HTML5 <progress> element on Windows. : [Attachment 53905] v2; fix to add rtl handling

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji 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 53905: v2; fix to add rtl handling
https://bugs.webkit.org/attachment.cgi?id=53905&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
> +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.