Bug 82459 - Enable animVal support for SVGAnimatedEnumeration
Summary: Enable animVal support for SVGAnimatedEnumeration
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: 12437 82885
Blocks: 41761
  Show dependency treegraph
 
Reported: 2012-03-28 06:16 PDT by Nikolas Zimmermann
Modified: 2012-04-03 03:26 PDT (History)
7 users (show)

See Also:


Attachments
Patch (208.21 KB, patch)
2012-03-28 07:00 PDT, Nikolas Zimmermann
no flags 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-03-28 06:16:03 PDT
Enable animVal support for SVGAnimatedEnumeration. This requires adding SVGAnimatedEnumerationAnimator, which is the _last_ missing animator.
This one is a bit trickier, as we need tests for every SVGAnimatedEnumeration object, that can get animated. I already took care of that.
Comment 1 Nikolas Zimmermann 2012-03-28 07:00:38 PDT
Created attachment 134286 [details]
Patch
Comment 2 Dirk Schulze 2012-03-28 10:53:55 PDT
Comment on attachment 134286 [details]
Patch

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

> Source/WebCore/ChangeLog:55
> +        We need to map the 'orient' attribute to a pair<SVGAngle, unsigned short> type, in order
> +        to track both orientAngle & orientType at the same type. Fortunately SVGAnimatedAngle
> +        is only used in the SVG DOM for SVGMarkerElements orientAngle property. We can directly
> +        switch SVGAnimatedAngleAnimator to the new pair<SVGAngle, unsigned short> type instead
> +        of having to introduce a special SVGAnimatedAngleAndEnumerationAnimator.

This is not part of this bug.

But can you please send the specific handling to the www-svg mailing list? Because the pre-draft version of the the proposal from Microsoft to CSS animations of presentation attributes does not reflect the handling of 'orientType' and 'orientAngle' correctly. Maybe this is something that needs to change in SVG 2.0.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:130
> +        // Animating from eg. auto to 90deg, or auto to 90deg.

You might want to use two different examples. I guess you meant 90deg to auto?

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:147
> +    template<typename AnimValType1, typename AnimValType2>

Don't we usually use A or B when we don't have other choices? I think 1 or 2 looks strange.

> LayoutTests/svg/animations/script-tests/svgenum-animation-3.js:31
> +    shouldBe("text.lengthAdjust.animVal", "SVGTextContentElement.LENGTHADJUST_SPACINGANDGLYPHS");
> +    shouldBe("text.lengthAdjust.baseVal", "SVGTextContentElement.LENGTHADJUST_SPACING");

That surprises me. Shouldn't it be LENGTHADJUST_SPACINGANDGLYPHS? The duration is 4s, you animate from spacing to spacingAndGlyphs and have the attribute 'fill' set to 'freeze'. So baseVal should have the result of animVal after the animation, no?
Comment 3 Dirk Schulze 2012-03-28 13:25:35 PDT
(In reply to comment #2)

> > LayoutTests/svg/animations/script-tests/svgenum-animation-3.js:31
> > +    shouldBe("text.lengthAdjust.animVal", "SVGTextContentElement.LENGTHADJUST_SPACINGANDGLYPHS");
> > +    shouldBe("text.lengthAdjust.baseVal", "SVGTextContentElement.LENGTHADJUST_SPACING");
> 
> That surprises me. Shouldn't it be LENGTHADJUST_SPACINGANDGLYPHS? The duration is 4s, you animate from spacing to spacingAndGlyphs and have the attribute 'fill' set to 'freeze'. So baseVal should have the result of animVal after the animation, no?

This seems to be related to bug 82514. Would be great if this bug could be fixed so that we know that this patch does what we want it to.
Comment 4 Nikolas Zimmermann 2012-03-29 02:29:51 PDT
(In reply to comment #2)
> (From update of attachment 134286 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134286&action=review
> 
> > Source/WebCore/ChangeLog:55
> > +        We need to map the 'orient' attribute to a pair<SVGAngle, unsigned short> type, in order
> > +        to track both orientAngle & orientType at the same type. Fortunately SVGAnimatedAngle
> > +        is only used in the SVG DOM for SVGMarkerElements orientAngle property. We can directly
> > +        switch SVGAnimatedAngleAnimator to the new pair<SVGAngle, unsigned short> type instead
> > +        of having to introduce a special SVGAnimatedAngleAndEnumerationAnimator.
> 
> This is not part of this bug.
Sure it is :-) Half of this patch is about supporting the SVGAnimatedEnumeration orientType, which requires special treatment.

> But can you please send the specific handling to the www-svg mailing list? Because the pre-draft version of the the proposal from Microsoft to CSS animations of presentation attributes does not reflect the handling of 'orientType' and 'orientAngle' correctly. Maybe this is something that needs to change in SVG 2.0.
We need to discuss this first, I didn't read their approach.
 
> > Source/WebCore/svg/SVGAnimatedAngle.cpp:130
> > +        // Animating from eg. auto to 90deg, or auto to 90deg.
> 
> You might want to use two different examples. I guess you meant 90deg to auto?
It doesn't matter, any-angle to auto, or auto to any-angle?

 > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:147
> > +    template<typename AnimValType1, typename AnimValType2>
> 
> Don't we usually use A or B when we don't have other choices? I think 1 or 2 looks strange.
That does't look any better, nor is it descriptive, what is A or B? Clearly here two AnimVAlTypes are meant, like SVGAnimatedBoolean, SVGAnimatedEnumeration etc.
 
> > LayoutTests/svg/animations/script-tests/svgenum-animation-3.js:31
> > +    shouldBe("text.lengthAdjust.animVal", "SVGTextContentElement.LENGTHADJUST_SPACINGANDGLYPHS");
> > +    shouldBe("text.lengthAdjust.baseVal", "SVGTextContentElement.LENGTHADJUST_SPACING");
> 
> That surprises me. Shouldn't it be LENGTHADJUST_SPACINGANDGLYPHS? The duration is 4s, you animate from spacing to spacingAndGlyphs and have the attribute 'fill' set to 'freeze'. So baseVal should have the result of animVal after the animation, no?
The baseVal never changes as result of the animation. The animVal remains frozen, the baseVal is not affected.
Comment 5 Dirk Schulze 2012-03-29 06:49:25 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 134286 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134286&action=review
> > 
> > > Source/WebCore/ChangeLog:55
> > > +        We need to map the 'orient' attribute to a pair<SVGAngle, unsigned short> type, in order
> > > +        to track both orientAngle & orientType at the same type. Fortunately SVGAnimatedAngle
> > > +        is only used in the SVG DOM for SVGMarkerElements orientAngle property. We can directly
> > > +        switch SVGAnimatedAngleAnimator to the new pair<SVGAngle, unsigned short> type instead
> > > +        of having to introduce a special SVGAnimatedAngleAndEnumerationAnimator.
> > 
> > This is not part of this bug.
> Sure it is :-) Half of this patch is about supporting the SVGAnimatedEnumeration orientType, which requires special treatment.
Hehe, I was unclear. The requested discussion is not part of the bug. The orientType of course is.

> 
> > But can you please send the specific handling to the www-svg mailing list? Because the pre-draft version of the the proposal from Microsoft to CSS animations of presentation attributes does not reflect the handling of 'orientType' and 'orientAngle' correctly. Maybe this is something that needs to change in SVG 2.0.
> We need to discuss this first, I didn't read their approach.
That was the topic I asked you to put on www-svg and is unrelated to your patch :)

> 
> > > Source/WebCore/svg/SVGAnimatedAngle.cpp:130
> > > +        // Animating from eg. auto to 90deg, or auto to 90deg.
> > 
> > You might want to use two different examples. I guess you meant 90deg to auto?
> It doesn't matter, any-angle to auto, or auto to any-angle?
I know. But you should change your comment. Right now you have the examples: "auto to 90deg, or auto to 90deg". Which should be different examples for the developer who reads your code.

> 
>  > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:147
> > > +    template<typename AnimValType1, typename AnimValType2>
> > 
> > Don't we usually use A or B when we don't have other choices? I think 1 or 2 looks strange.
> That does't look any better, nor is it descriptive, what is A or B? Clearly here two AnimVAlTypes are meant, like SVGAnimatedBoolean, SVGAnimatedEnumeration etc.
Also true. But you should follow similar examples in WebCore if there are any.

> 
> > > LayoutTests/svg/animations/script-tests/svgenum-animation-3.js:31
> > > +    shouldBe("text.lengthAdjust.animVal", "SVGTextContentElement.LENGTHADJUST_SPACINGANDGLYPHS");
> > > +    shouldBe("text.lengthAdjust.baseVal", "SVGTextContentElement.LENGTHADJUST_SPACING");
> > 
> > That surprises me. Shouldn't it be LENGTHADJUST_SPACINGANDGLYPHS? The duration is 4s, you animate from spacing to spacingAndGlyphs and have the attribute 'fill' set to 'freeze'. So baseVal should have the result of animVal after the animation, no?
> The baseVal never changes as result of the animation. The animVal remains frozen, the baseVal is not affected.
Just checked it and you are right. Animations do not influence baseVal, even with fill="freeze". Which makes kind of sense but could also be useful the other way around. Nevertheless the example is correct.
Comment 6 Dirk Schulze 2012-03-29 06:50:18 PDT
Comment on attachment 134286 [details]
Patch

r=me. But see suggestions from previous comment before landing.
Comment 7 Nikolas Zimmermann 2012-04-01 04:37:40 PDT
(In reply to comment #5)
> That was the topic I asked you to put on www-svg and is unrelated to your patch :)
Okay, we should have a chat about this on IRC before.

> > > > Source/WebCore/svg/SVGAnimatedAngle.cpp:130
> > > > +        // Animating from eg. auto to 90deg, or auto to 90deg.
> > > 
> > > You might want to use two different examples. I guess you meant 90deg to auto?
> > It doesn't matter, any-angle to auto, or auto to any-angle?
> I know. But you should change your comment. Right now you have the examples: "auto to 90deg, or auto to 90deg". Which should be different examples for the developer who reads your code.
LOL. I finally realized the two examples are the same ;-) I swear I read "auto to 90deg, or 90deg to auto" before! Will fix.

> > The baseVal never changes as result of the animation. The animVal remains frozen, the baseVal is not affected.
> Just checked it and you are right. Animations do not influence baseVal, even with fill="freeze". Which makes kind of sense but could also be useful the other way around. Nevertheless the example is correct.
Phew :-)

I'll land this soon, thanks for the review!
Comment 8 Nikolas Zimmermann 2012-04-01 05:09:41 PDT
Committed r112813: <http://trac.webkit.org/changeset/112813>
Comment 10 Patrick R. Gansterer 2012-04-02 01:25:35 PDT
Landed build fix for ENABLE(SVG) && !ENABLE(FILTERS) in r112837:<trac.webkit.org/changeset/112837>
Comment 11 Csaba Osztrogonác 2012-04-02 04:22:00 PDT
(In reply to comment #9)
> There are some crash related this patch on Lion, QT WK2 and GTK.
> Here are the crash logs: http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r112813%20(5981)/svg/animations/svgenum-animation-8-crash-log.txt
> http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r112813%20(5981)/svg/animations/svgenum-animation-13-crash-log.txt

Reopen because it caused regression.
Comment 12 Csaba Osztrogonác 2012-04-02 04:24:11 PDT
Could you check what happened?
Comment 13 Csaba Osztrogonác 2012-04-02 05:17:52 PDT
It was rolled out because it caused regression - http://trac.webkit.org/changeset/112863
Comment 14 Nikolas Zimmermann 2012-04-02 09:48:14 PDT
(In reply to comment #12)
> Could you check what happened?
I can't reproduce this at all on my Lion machine :-(
Thanks for rolling it out! I'll investigate.
Comment 15 Nikolas Zimmermann 2012-04-02 09:51:26 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > Could you check what happened?
> I can't reproduce this at all on my Lion machine :-(
Fortunately there is valgrind - trunk works very well with WebKit on Lion these days:
==53202== Invalid read of size 4
==53202==    at 0x462B5A1: WebCore::SVGComponentTransferFunctionElement::transferFunction() const (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x46645E5: WebCore::SVGFEComponentTransferElement::build(WebCore::SVGFilterBuilder*, WebCore::Filter*) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x4391508: WebCore::RenderSVGResourceFilter::buildPrimitives(WebCore::SVGFilter*) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x4391EDA: WebCore::RenderSVGResourceFilter::applyResource(WebCore::RenderObject*, WebCore::RenderStyle*, WebCore::GraphicsContext*&, unsigned short) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x4384B46: WebCore::SVGRenderingContext::prepareToRenderSVGContent(WebCore::RenderObject*, WebCore::PaintInfo&, WebCore::SVGRenderingContext::NeedsGraphicsContextSave) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x43F137B: WebCore::SVGRenderingContext::SVGRenderingContext(WebCore::RenderObject*, WebCore::PaintInfo&, WebCore::SVGRenderingContext::NeedsGraphicsContextSave) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x43C007A: WebCore::SVGRenderingContext::SVGRenderingContext(WebCore::RenderObject*, WebCore::PaintInfo&, WebCore::SVGRenderingContext::NeedsGraphicsContextSave) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x439E734: WebCore::RenderSVGShape::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x42626F3: WebCore::RenderBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x439BAE9: WebCore::RenderSVGRoot::paintReplaced(WebCore::PaintInfo&, WebCore::IntPoint const&) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x4362D64: WebCore::RenderReplaced::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x391CF36: WebCore::InlineBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&, int, int) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==  Address 0x1012747b0 is 0 bytes inside a block of size 2 alloc'd
==53202==    at 0xC743: malloc (vg_replace_malloc.c:266)
==53202==    by 0x499C1A: WTF::fastMalloc(unsigned long) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore)
==53202==    by 0x49055C3: WebCore::SVGAnimatedStaticPropertyTearOff<unsigned short>::ContentType* WebCore::SVGAnimatedTypeAnimator::constructFromBaseValue<WebCore::SVGAnimatedStaticPropertyTearOff<unsigned short> >(WTF::Vector<WebCore::SVGAnimatedProperty*, 0ul> const&) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x4904F7A: WebCore::SVGAnimatedEnumerationAnimator::startAnimValAnimation(WTF::Vector<WebCore::SVGAnimatedProperty*, 0ul> const&) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x4619209: WebCore::SVGAnimateElement::resetToBaseValue(WTF::String const&) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x456A016: WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime) (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x4569B71: WebCore::SMILTimeContainer::begin() (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x463AC33: WebCore::SVGDocumentExtensions::startAnimations() (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x33FFE70: WebCore::Document::implicitClose() (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x36CDB3A: WebCore::FrameLoader::checkCallImplicitClose() (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x36CD924: WebCore::FrameLoader::checkCompleted() (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
==53202==    by 0x36CC612: WebCore::FrameLoader::finishedParsing() (in /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)

That explains the flakiness and is pretty evil.
Comment 16 Nikolas Zimmermann 2012-04-03 02:03:29 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Could you check what happened?
> > I can't reproduce this at all on my Lion machine :-(
> Fortunately there is valgrind - trunk works very well with WebKit on Lion these days:
> ==53202== Invalid read of size 4

Ok, it turns out the fix for this is actually simple: s/unsigned short/unsigned/. With that applied, the invalid read is gone. I hope this is enough to make it work everywhere. I assumed before that each enum size would fit in an unsigned short, though that's up to the compiler. Let's see.
Comment 17 Nikolas Zimmermann 2012-04-03 02:44:13 PDT
Relanded in r113003 - closing this bug if the bots are happy.
Comment 18 Nikolas Zimmermann 2012-04-03 03:26:54 PDT
Landed chromium build fix in r113005 & r113006. (signed vs. unsigned comparison issue)