Bug 20057

Summary: Animate viewBox attribute in SVG
Product: WebKit Reporter: Bruce Rindahl <rindahl>
Component: SVGAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Enhancement CC: koivisto, krit, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
URL: http://www.lrcwe-data.com/DeepZoom.svg
Bug Depends on:    
Bug Blocks: 41761    
Attachments:
Description Flags
SVG file showing requested behavior
none
correction to previous attachment
none
Patch zimmermann: review+

Description Bruce Rindahl 2008-07-16 11:24:29 PDT
Request to support animation of the viewBox attribute of an svg element.  View the above URL in Opera 9.5 (there is a bug but you can see the desired effect) or IE with the Adobe SVG plugin.  Minimal test case to be attached
Comment 1 Bruce Rindahl 2008-07-16 11:27:41 PDT
Created attachment 22310 [details]
SVG file showing requested behavior

Open attachment and click on the red circle.  A zooming effect should be created because the viewBox is animated via script.  Repeated click result in continual zooming.  Works in IE/ASV and Opera(has a hiccup).
Comment 2 Bruce Rindahl 2008-07-16 12:42:38 PDT
Created attachment 22312 [details]
correction to previous attachment
Comment 3 Dirk Schulze 2011-06-22 02:33:13 PDT
Upload a patch to support viewBox animation soon.
Comment 4 Dirk Schulze 2011-06-22 03:19:34 PDT
Created attachment 98148 [details]
Patch
Comment 5 Nikolas Zimmermann 2011-06-22 03:42:51 PDT
Comment on attachment 98148 [details]
Patch

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

Great patch, r=me.

> Source/WebCore/svg/SVGAnimatedRect.cpp:36
> +    OwnPtr<SVGAnimatedType> animatedType = SVGAnimatedType::createRect(new FloatRect());

you can omit the braces after the new FloatRect.

> Source/WebCore/svg/SVGAnimatedRect.cpp:75
> +        newRect = percentage < 0.5f ? fromRect : toRect;

Again the .f, we don't need that anymore - the style guide says we should omit this.

> Source/WebCore/svg/SVGAnimatedRect.cpp:99
> +    return -1;

Does this affect a specific animation mode which won't work with rects?

> Source/WebCore/svg/SVGAnimatedType.cpp:129
> +        return String::number(m_data.rect->x()) + ' ' + String::number(m_data.rect->y()) + ' '
> +             + String::number(m_data.rect->width()) + ' ' + String::number(m_data.rect->height());

Great, that's exactly how we can construct strings like this efficiently with the new StringAppend operator+ trickery :-)
Comment 6 Dirk Schulze 2011-06-22 05:29:35 PDT
(In reply to comment #5)
> (From update of attachment 98148 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98148&action=review
> 
> Great patch, r=me.
> 
> > Source/WebCore/svg/SVGAnimatedRect.cpp:36
> > +    OwnPtr<SVGAnimatedType> animatedType = SVGAnimatedType::createRect(new FloatRect());
> 
> you can omit the braces after the new FloatRect.
> 
> > Source/WebCore/svg/SVGAnimatedRect.cpp:75
> > +        newRect = percentage < 0.5f ? fromRect : toRect;
> 
> Again the .f, we don't need that anymore - the style guide says we should omit this.
Really? I asked for it on your other reviews, but you never answered to the question. I was not aware of this change. Just thought this is the case for round numbers like 0 or 1. Isn't it a double if you omit the .f?

> 
> > Source/WebCore/svg/SVGAnimatedRect.cpp:99
> > +    return -1;
> 
> Does this affect a specific animation mode which won't work with rects?
Yes, paced animations. Just supported by Opera so far. The specification is vague about that.

> 
> > Source/WebCore/svg/SVGAnimatedType.cpp:129
> > +        return String::number(m_data.rect->x()) + ' ' + String::number(m_data.rect->y()) + ' '
> > +             + String::number(m_data.rect->width()) + ' ' + String::number(m_data.rect->height());
> 
> Great, that's exactly how we can construct strings like this efficiently with the new StringAppend operator+ trickery :-)

Hehe. I know. You told me to use it that way before ;)

Thanks for the review.
Comment 7 Dirk Schulze 2011-06-22 08:25:17 PDT
Committed r89431: <http://trac.webkit.org/changeset/89431>