Bug 82459

Summary: Enable animVal support for SVGAnimatedEnumeration
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: bank, krit, ossy, paroga, rakuco, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 12437, 82885    
Bug Blocks: 41761    
Attachments:
Description Flags
Patch none

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)