[Webkit-unassigned] [Bug 49769] <progress> element is unsupported on Windows

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49769


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76762|review?                     |review-
               Flag|                            |




--- Comment #12 from Adam Roben (aroben) <aroben at apple.com>  2011-01-19 12:29:09 PST ---
(From update of attachment 76762)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.