Bug 85071 – Fix repetitions & by animation support for path animations

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Currently accumulation isn't handled at all in SVGAnimatedPathAnimator, and by animations fallback to to-animations, which is against SMIL.

[Nikolas Zimmermann]

Created attachment 139214 [details]
Patch

[Antti Koivisto]

Comment on attachment 139214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139214&action=review

r=me

> Source/WebCore/svg/SVGAnimatedPath.cpp:134
> +        SVGPathParserFactory::self()->addToSVGPathByteStream(animatedPath, toPath, repeatCount);

SVGPathParserFactory singleton doesn't add anything (except some overhead for passing the this pointer), addToSVGPathByteStream and pals could be free functions and the whole SVGPathParserFactory type removed.

> Source/WebCore/svg/SVGPathBlender.cpp:392
> -        if (m_fromSource->hasMoreData() != m_toSource->hasMoreData())
> -            return false;
> -        if (!m_fromSource->hasMoreData() || !m_toSource->hasMoreData())
> +        if (fromSourceHadData) {
> +            if (m_fromSource->hasMoreData() != m_toSource->hasMoreData())
> +                return false;
> +            if (!m_fromSource->hasMoreData() || !m_toSource->hasMoreData())
> +                break;
> +        } else if (!m_toSource->hasMoreData())
>              break;
>      }
>      return true;

You should replace all instances of 'break' in this function with 'return true'. It will be shorter and read better.

[Antti Koivisto]

(In reply to comment #2)
> You should replace all instances of 'break' in this function with 'return true'. It will be shorter and read better.

        case PathSegCurveToQuadraticAbs:
            return blendCurveToQuadraticSegment();

etc.

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 139214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139214&action=review
> 
> r=me
> 
> > Source/WebCore/svg/SVGAnimatedPath.cpp:134
> > +        SVGPathParserFactory::self()->addToSVGPathByteStream(animatedPath, toPath, repeatCount);
> 
> SVGPathParserFactory singleton doesn't add anything (except some overhead for passing the this pointer), addToSVGPathByteStream and pals could be free functions and the whole SVGPathParserFactory type removed.
As discussed on IRC, I'll turn SVGPathParserFactory into free functions, it also  needs a rename, it might be a factory internally, but doesn't act as such to the outside.


> > > Source/WebCore/svg/SVGPathBlender.cpp:392
> >              break;
> >      }
> >      return true;
> 
> You should replace all instances of 'break' in this function with 'return true'. It will be shorter and read better.
Will do.

[Antti Koivisto]

(In reply to comment #3)
> (In reply to comment #2)
> > You should replace all instances of 'break' in this function with 'return true'. It will be shorter and read better.
> 
>         case PathSegCurveToQuadraticAbs:
>             return blendCurveToQuadraticSegment();
> 
> etc.

Ah forget it. I misread.

[Nikolas Zimmermann]

Committed r115555: <http://trac.webkit.org/changeset/115555>

[Antti Koivisto]

Comment on attachment 139214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139214&action=review

> Source/WebCore/svg/SVGPathBlender.cpp:149
> -    float fromY;
> +    float fromY = 0;
>      float toY;

It would be less confusing to initialize these both. Optimization is unlikely to be meaningful. Or at least add a comment explaining it is not a mistake.

[Nikolas Zimmermann]

(In reply to comment #6)
> Committed r115555: <http://trac.webkit.org/changeset/115555>

Landed SVGPathBlender cleanups separated in r115558.