Bug 85071 - Fix repetitions & by animation support for path animations
Summary: Fix repetitions & by animation support for path animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 41761 36729
  Show dependency treegraph
 
Reported: 2012-04-27 08:50 PDT by Nikolas Zimmermann
Modified: 2012-04-28 02:19 PDT (History)
2 users (show)

See Also:


Attachments
Patch (50.40 KB, patch)
2012-04-27 09:09 PDT, Nikolas Zimmermann
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-04-27 08:50:44 PDT
Currently accumulation isn't handled at all in SVGAnimatedPathAnimator, and by animations fallback to to-animations, which is against SMIL.
Comment 1 Nikolas Zimmermann 2012-04-27 09:09:18 PDT
Created attachment 139214 [details]
Patch
Comment 2 Antti Koivisto 2012-04-28 00:45:33 PDT
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.
Comment 3 Antti Koivisto 2012-04-28 00:48:08 PDT
(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.
Comment 4 Nikolas Zimmermann 2012-04-28 00:51:03 PDT
(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.
Comment 5 Antti Koivisto 2012-04-28 00:53:16 PDT
(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.
Comment 6 Nikolas Zimmermann 2012-04-28 01:26:44 PDT
Committed r115555: <http://trac.webkit.org/changeset/115555>
Comment 7 Antti Koivisto 2012-04-28 02:08:38 PDT
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.
Comment 8 Nikolas Zimmermann 2012-04-28 02:19:24 PDT
(In reply to comment #6)
> Committed r115555: <http://trac.webkit.org/changeset/115555>

Landed SVGPathBlender cleanups separated in r115558.