[webkit-reviews] review denied: [Bug 36664] Add animation to progress element : [Attachment 51763] Patch for animation.

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Yael <yael.aharon at nokia.com>'s
request for review:
Bug 36664: Add animation to progress element
https://bugs.webkit.org/show_bug.cgi?id=36664

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
Looks generally good. I think you should consider doing animations in other way
than incrementing frame counter (see below) so r- for now.

>  #if ENABLE(PROGRESS_TAG)
> -bool RenderThemeQt::getNumberOfPixelsForProgressPosition(double position,
int& progressSize) const
> +double RenderThemeQt::animationIntervalForProgressBar(bool determinate)
const
>  {
> -    progressSize = 65536 * position;
> -    return false;
> +    // FIXME: Use hardcoded value until
http://bugreports.qt.nokia.com/browse/QTBUG-9171 is fixed.
> +    // Use the value from windows style which is 10 fps.
> +    if (determinate)
> +	   return -1;
> +    return 0.1;
>  }

Either 0 (real interval can't be 0) or std::numeric_limits<double>::infinity()
would be more natural ways to signal "no animation interval" than slightly
random -1.

>  #if ENABLE(PROGRESS_TAG)
> -    // Helper method for optimizing the paint area of the progress bar.
> -    // If supported, it returns number of pixels needed to draw the progress
bar up to the progress position.
> -    // progressSize is the value that is passed back to RenderTheme during
drawing.
> -    virtual bool getNumberOfPixelsForProgressPosition(double position, int&
progressSize) const;
> +    // Returns the interval for the progress bar animation, or -1 if no
animation is required.
> +    virtual double animationIntervalForProgressBar(bool determinate) const;
>  #endif

Enums are generally preferred over magic bools since it is hard to tell what is
going on from the call site.

> > ===================================================================
> --- WebCore/rendering/RenderObject.h	(revision 56630)
> +++ WebCore/rendering/RenderObject.h	(working copy)
> @@ -788,6 +788,8 @@ protected:
>      void paintOutline(GraphicsContext*, int tx, int ty, int w, int h, const
RenderStyle*);
>      void addPDFURLRect(GraphicsContext*, const IntRect&);
>  
> +    bool willRenderContents();
> +
>      virtual IntRect viewRect() const;

Could be public and next to willRenderImage() in the header


> +void RenderProgress::animationFrameTimerFired(Timer<RenderProgress>*)
> +{
> +    m_animationFrame++;

Usually animation in WebKit is tracked in terms of progress on timeline. Doing
animation by increment frame count is not optimal. If you increase the
animation frame rate it will also cause the animation to run faster.
RenderMedia fade is an example of this type of animation in an existing similar
type of control element.

> +    repaintRectangle(contentBoxRect());
> +
> +    IntRect r(contentBoxRect());
> +}

This line does nothing.