Bug 82459

Created attachment 134286 [details] Patch

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?

(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.

(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.

(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 on attachment 134286 [details] Patch r=me. But see suggestions from previous comment before landing.

(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!

Committed r112813: <http://trac.webkit.org/changeset/112813>

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

Landed build fix for ENABLE(SVG) && !ENABLE(FILTERS) in r112837:<trac.webkit.org/changeset/112837>

(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.

Could you check what happened?

It was rolled out because it caused regression - http://trac.webkit.org/changeset/112863

(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.

(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.

(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.

Relanded in r113003 - closing this bug if the bots are happy.

Landed chromium build fix in r113005 & r113006. (signed vs. unsigned comparison issue)