Bug 36664 – Add animation to progress element

• Component: DOM
• Status: RESOLVED FIXED

[Yael]

Indeterminate progress bar should animate, and on Mac OS even determinate progress bar should animate.
Add a timer and capability to animate the progress bar when needed.

[Yael]

Created attachment 51763 [details]
Patch for animation.

Add a timer to control the animation of the progress bar.
Remove an obsolete function RenderTheme::getNumberOfPixelsForProgressPosition.

[Alexey Proskuryakov]

What stops the animation when the page goes into b/f cache?

[Yael]

(In reply to comment #2)
> What stops the animation when the page goes into b/f cache?

RenderObject::willRenderContents() will return false when the document is in cache, and animation will not start again.

[Antti Koivisto]

Comment on attachment 51763 [details]
Patch for animation.

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.

[Yael]

Created attachment 52030 [details]
Adress Antti's comments in #4

[Antti Koivisto]

Some stuff I said in IRC about possible ways to clarify responsibilities between theme and the renderer.

[18:39] anttik: so how about this
[18:42] anttik: RenderProgress will have animating state
[18:43] anttik: which is independent of whether the animation is currently happening (since that depends on if it is visible or not)
[18:44] anttik: when entering animating state it records the start time
[18:44] anttik: animationProgress() is calculated from start time, duration and current time
[18:45] anttik: and is used as input for rendering
[18:47] anttik: if in animating state it will restart the animation timer on paint()
[18:48] anttik: actual changes in the progress bar state will trigger changes in and out of the animating state
[18:49] anttik: possibly querying style if a given progress bar state needs animation or not
[18:51] anttik: so paint() will have if (m_animating) m_startOneShot(m_repeatInterval)
[18:51] anttik: and start/stopAnimation() will do state transitions
[18:51] anttik: m_animationTimer.startOneShot()
[18:53] anttik: that should lead to a clear split of responsibility
[18:53] anttik: does that sound stupid?

[Yael]

Created attachment 52069 [details]
Patch 

Update the design as siggested on IRC (comment #6).
Some changes 
- Renamed startAnimation() to updateAnimationState() because it is not starting the animation timer anymore.
- Removed the check RenderObject::willRenderContents() because when a page is in the b/f cache, paint() is not called, so the timer will not start.

[Antti Koivisto]

Comment on attachment 52069 [details]
Patch 

Looks pretty good, a few comments:

>  #if ENABLE(PROGRESS_TAG)
> -bool RenderThemeQt::getNumberOfPixelsForProgressPosition(double position, int& progressSize) const
> +bool RenderThemeQt::animationIntervalAndDurationForProgressBar(RenderProgress* renderProgress, double& interval, double& duration) const

The style is to name functions that return values in parameters get* -> getAnimationIntervalAndDurationForProgressBar. Alternatively use multiple functions with single return value.

It would be better to return all values as parameters. Now it is unclear what the separate bool return means. I'm not sure if it is really needed either, 0 duration could signal the same thing (==don't animate).

+RenderProgress::~RenderProgress()
+{
+    m_animationTimer.stop();
+}
+

No need to add destructor for this, the timer will get deleted and stop automatically.

> +double RenderProgress::animationProgress()
> +{
> +    return m_animating ? (fmod((currentTime() - m_animationStartTime), m_animationDuration) / m_animationInterval) : 0;
> +}

Dividing by m_animationInterval is bit strange and seems to tie to the specific way the theme is implemented. It would be better to divide by m_animationDuration to get 0-1 progress range. The paint position can then be calculated in the theme by progress * option.rect.width().

> +void RenderProgress::updateAnimationState()
> +{
> +    m_animationTimer.stop();
> +    m_animating = theme()->animationIntervalAndDurationForProgressBar(this, m_animationInterval, m_animationDuration);
> +    if (m_animating)
> +        m_animationStartTime = currentTime();
> +}

Should this also start the timer if the state changed? Stopping the timer seems wrong if the state did not change.

>      double m_position;
> +    double m_animationStartTime;
> +    double m_animationInterval;

m_animationRepeatInterval perhaps?

[Yael]

Created attachment 52177 [details]
Patch updated based on comment #8.

[Antti Koivisto]

Comment on attachment 52177 [details]
Patch updated based on comment #8.

Nice, r=me

> +    m_animationTimer.stop();
> +    if (m_animating) {
> +        m_animationStartTime = currentTime();
> +        m_animationTimer.startOneShot(m_animationRepeatInterval);
> +    }

That could be

else
    m_animationTimer.stop();

(it is ok to start active timer again)

[Yael]

Landed in r56850.

[Yael]

*** Bug 36224 has been marked as a duplicate of this bug. ***