[webkit-reviews] review denied: [Bug 49769] <progress> element is unsupported on Windows : [Attachment 76762] Patch.

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 49769: <progress> element is unsupported on Windows
https://bugs.webkit.org/show_bug.cgi?id=49769

Attachment 76762: Patch.
https://bugs.webkit.org/attachment.cgi?id=76762&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=76762&action=review

> WebCore/rendering/RenderThemeSafari.cpp:57
> +const Color progressBarBackgroundColor = Color(0x7f, 0x7f, 0x7f);
> +const Color progressBarForegroundColor = Color(0, 0, 0x7f);

These should be marked static so they'll have internal linkage. But they'll
also create static initializers and exit-time destructors, which we normally
try to avoid. It would be better to use DEFINE_STATIC_LOCAL to define these
inside the paintProgressBar function.

> WebCore/rendering/RenderThemeSafari.cpp:1232
> +bool RenderThemeSafari::paintProgressBar(RenderObject* o, const PaintInfo&
paintInfo, const IntRect& r)
> +{
> +    FloatRect widgetRect = r;
> +    RenderProgress* renderProgress = toRenderProgress(o);
> +    FloatRect valueRect;
> +    if (renderProgress->isDeterminate()) {
> +	   int dx = r.width() * renderProgress->position();
> +	   if (renderProgress->style()->isLeftToRightDirection())
> +	       valueRect = IntRect(r.x(), r.y(), dx, r.height());
> +	   else
> +	       valueRect = IntRect(r.x() + r.width() - dx, r.y(), dx,
r.height());
> +    }
> +
> +    paintInfo.context->fillRect(widgetRect, progressBarBackgroundColor,
ColorSpaceDeviceRGB);
> +    paintInfo.context->fillRect(valueRect, progressBarForegroundColor,
ColorSpaceDeviceRGB);

I think IntRect can be implicitly converted to FloatRect, so I don't think you
need the widgetRect variable. I also think valueRect could be an IntRect for
the same reason.

If you initialize valueRect to r then you could do things like
valueRect.setWidth(dx).

We generally prefer unabbreviated names, so "filledWidth" (or similar) would be
better than "dx".

It's good to have an implementation here so that something will show up in the
regression tests. Unfortunately, this implementation won't help us in our goal
of being able to share test results with Mac. But someone at Apple would need
to do that work.

> WebCore/rendering/RenderThemeSafari.cpp:1236
> +}
> +
> +#endif

Extra blank line before the if.

> WebCore/rendering/RenderThemeWin.cpp:162
> +static const double progressAnimationFrameRate = 0.033;

Maybe 1.0 / 30 would be clearer?

> WebCore/rendering/RenderThemeWin.cpp:1159
> +double RenderThemeWin::animationDurationForProgressBar(RenderProgress*)
const
> +{
> +    return progressAnimationFrameRate;
> +}

I think you're returning progressAnimationFrameRate here because you need to
return some non-0 value so that RenderProgress will run its animation timer,
but the actual duration doesn't matter because Windows uses a constant-speed
animation rather than a constant-duration animation. A comment to this effect
(or some other effect, if I'm wrong) would be helpful. And maybe choosing an
obviously bogus duration value would be better.

Since there's no animation on XP for determinate progress bars, or in Classic
mode, it seems like we should return 0 for both the repeat interval and the
animation duration in those cases. Otherwise the animation timer will be firing
needlessly.

> WebCore/rendering/RenderThemeWin.cpp:1165
> +static int computeAnimationProgress(int frameWidth, int objectWidth, int
pixelsPerSecond, double animatedSeconds)

This function name is a bit confusing, given how overloaded "progress" is.
RenderProgress::animationProgress returns a number between 0 and 1, while this
returns a width. Maybe it could be made clearer if it returned the rect for the
overlay. Something like this:

static RECT progressAnimationOverlayRect(const RECT& rectToTraverse, int
overlayWidth, int pixelsTraversedPerSecond, double secondsSinceAnimationBegan)

Returning a rect would also reduce some code duplication you currently have in
the calling function.

Adding a comment somewhere to explain that some progress bar styles have an
overlay that animates over top of the regular progress bar would also help.

> WebCore/rendering/RenderThemeWin.cpp:1176
> +    // There is no known API to obtain these numbers,
> +    // so they were taken from the Chrome implementation.

Does the Chrome implementation have comments about how they were chosen or
determined? If so it would be good to include those comments here.

> WebCore/rendering/RenderThemeWin.cpp:1183
> +    const int kDeteminateOverlayPixelsPerSecond = 300;
> +    const int kDeteminateOverlayWidth = 120;
> +    const int kIndeterminateOverlayPixelsPerSecond =  175;
> +    const int kVistaIndeterminateOverlayWidth = 120;
> +    const int kXPIndeterminateOverlayWidth = 55;
> +    // The thickness of the bar frame inside |valueRect|
> +    const int kXPBarPadding = 3;

We don't use "k" for constants like Chrome does. You're also missing a period
at the end of the comment.

Extra space before "175".

> WebCore/rendering/RenderThemeWin.cpp:1187
> +    LocalWindowsContext windowsContext(i.context, r, false);
> +    RECT widgetRect = r;
> +    HDC hdc = windowsContext.hdc();

You should declare these a bit closer to where they're first used.

> WebCore/rendering/RenderThemeWin.cpp:1191
> +	   int dx = r.width() * renderProgress->position();

Same comment as before about names like "dx".

> WebCore/rendering/RenderThemeWin.cpp:1200
> +    if (progressBarTheme()) {

Reversing this if and using an early-return in the Classic case would allow you
to un-indent a bunch of code in this function.

> WebCore/rendering/RenderThemeWin.cpp:1205
> +	       // We should care the direction here because PP_CNUNK painting

Typos: "We should care the direction" and "PP_CNUNK"

> WebCore/rendering/RenderThemeWin.cpp:1225
> +		   // On XP, progress bar is chunk-style and has no glossy
effect.

Missing a "the" before "progress bar".

> WebCore/rendering/RenderThemeWin.cpp:1226
> +		   // We need to shrink destination rect to fit the part inside
the bar

Missing a "the" after "shrink".

> WebCore/rendering/RenderThemeWin.cpp:1232
> +	   } else {

You could use an early-return here instead of an else.

> WebCore/rendering/RenderThemeWin.cpp:1237
> +	       int overlayWidth = !isRunningOnVistaOrLater() ?
kXPIndeterminateOverlayWidth : kVistaIndeterminateOverlayWidth;

I think this would be easier to read if you reversed it (i.e., get rid of the
!).

> WebCore/rendering/RenderThemeWin.h:129
> +    // Returns the repeat interval of the animation for the progress bar.
> +    virtual double animationRepeatIntervalForProgressBar(RenderProgress*)
const;
> +    // Returns the duration of the animation for the progress bar.
> +    virtual double animationDurationForProgressBar(RenderProgress*) const;

These comments don't add any information. They're just repeating the function
names.