Bug 61368

Created attachment 94624 [details] Patch First patch for comments an discussions, not for landing. I need to add tests before I upload a patch for review.

Comment on attachment 94624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94624&action=review Looks good to me. > Source/WebCore/svg/SVGAnimateElement.cpp:186 > + case AnimatedNumberList: > + case AnimatedLengthList: Why this reorering? You should keep it in alphabetical order, no? > Source/WebCore/svg/SVGAnimateElement.cpp:545 > + m_animatedType->length() = new SVGLength(lengthModeForAnimatedLengthAttribute(attributeName()), baseString); This looks weird... > Source/WebCore/svg/SVGAnimatedLengthAnimator.h:42 > + type->length() = new SVGLength(m_lengthMode, string); This looks weird... > Source/WebCore/svg/SVGAnimatedType.h:39 > + SVGLength*& length() Why don't you return SVGLength& here? aka. return *m_data.length; > Source/WebCore/svg/SVGAnimationElement.cpp:163 > + for (unsigned i = 0; i < m_values.size(); ++i) > + m_values[i] = m_values[i].stripWhiteSpace(); Looks unrelated. > Source/WebCore/svg/SVGAnimator.h:34 > + return adoptPtr(new SVGAnimatedLengthAnimator(contextElement)); you should still add the switch here, and only implement case AnimatedLength/default: return adoptPtr(new SVGAnimatedLengthAnimator...

Created attachment 95258 [details] Patch

Attachment 95258 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1 Source/WebCore/svg/SVGAnimatedLength.h:26: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/svg/SVGAnimatedLength.h:46: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedLength.h:48: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedLength.h:49: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedLength.h:51: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedLength.h:51: The parameter name "animated" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedTypeAnimator.h:36: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedTypeAnimator.h:37: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39: The parameter name "animated" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 10 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #4) > Attachment 95258 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1 > > Source/WebCore/svg/SVGAnimatedLength.h:26: Alphabetical sorting problem. [build/include_order] [4] Testing correct order. But because the order was already there, I expect build problems. > Source/WebCore/svg/SVGAnimatedLength.h:46: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] removed locally. > Source/WebCore/svg/SVGAnimatedLength.h:48: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/svg/SVGAnimatedLength.h:49: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/svg/SVGAnimatedLength.h:51: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/svg/SVGAnimatedLength.h:51: The parameter name "animated" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:36: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:37: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39: The parameter name "to" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39: The parameter name "animated" adds no information, so it should be removed. [readability/parameter_name] [5] > Total errors found: 10 in 67 files I think these names are useful. Otherwise it is hard to differ between the arguments.

Comment on attachment 95258 [details] Patch Attachment 95258 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8748263

Comment on attachment 95258 [details] Patch Attachment 95258 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8747312

Comment on attachment 95258 [details] Patch Attachment 95258 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8740622

Comment on attachment 95258 [details] Patch Attachment 95258 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8743525

Comment on attachment 95258 [details] Patch Attachment 95258 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8748273

Comment on attachment 95258 [details] Patch Attachment 95258 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8736876

Created attachment 95261 [details] Patch

Comment on attachment 95261 [details] Patch Attachment 95261 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8667063

(In reply to comment #13) > (From update of attachment 95261 [details]) > Attachment 95261 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/8667063 Hm. Not sure why Gtk fails.

Created attachment 95264 [details] Patch

Another comment. Can you add a test with keyboard inputs? would be great.

(In reply to comment #16) > Another comment. Can you add a test with keyboard inputs? would be great. Wrong bug, sorry.

Comment on attachment 95264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95264&action=review It's a great start, I still have some comments though that lead to r-: > Source/WebCore/ChangeLog:11 > + > + The goal is to move the main animation logic for SVG units from SVGAnimateElement into the corresponding SVGAnimated* files. > + Starting with SVGLength on this patch. SVGAnimateElement will just work with the generic type SVGAnimatedType. SVGAnimator > + coordinates the connection from SVGAnimatedType to the SVG unit animators like SVGAnimatedLengthAnimator in SVGAnimatedLength.h. > + I think you should make it much more clear, that you're going away from any->string->any operation mode for animations, but rather directly animate "any" w/o string roundtrips. > Source/WebCore/svg/SVGAnimateElement.cpp:125 > +static inline SVGLengthMode lengthModeForAnimatedLengthAttribute(const QualifiedName& attrName) Does this method really belong here? How about moving it to SVGLength as static function? > Source/WebCore/svg/SVGAnimateElement.cpp:448 > + m_animator = SVGAnimator::create(AnimatedLength, targetElement); This is just called, once right? We're not recreating SVGAnimator on every animation step anymore, right? Can we assert here, that m_animator is null? Are we properly destructing the animator at some point? > Source/WebCore/svg/SVGAnimateElement.cpp:449 > + static_cast<SVGAnimatedLengthAnimator*>(m_animator.get())->setSVGLengthMode(lengthModeForAnimatedLengthAttribute(attributeName())); I'm wondering whether this logic could be folded into the SVGAnimator::create code, by just passing the attributeName() as another constructor parameter. If other types don't need to know the attribute name they can just ignore the third ctor param... > Source/WebCore/svg/SVGAnimateElement.cpp:492 > + m_animator = SVGAnimator::create(AnimatedLength, targetElement); > + static_cast<SVGAnimatedLengthAnimator*>(m_animator.get())->setSVGLengthMode(lengthModeForAnimatedLengthAttribute(attributeName())); Ditto. > Source/WebCore/svg/SVGAnimateElement.cpp:545 > + m_animatedType = SVGAnimatedType::create(AnimatedLength); > + m_animatedType->setLength(new SVGLength(lengthModeForAnimatedLengthAttribute(attributeName()), baseString)); How about adding SVGAnimatedType::createLength() which takes another LengthMode and const String& parameter, and hides this code? > Source/WebCore/svg/SVGAnimateElement.cpp:584 > + // TODO: Distance calculation should be done in SVGAnimatedTypeAnimator. Can't you fix this directly, and add a calculateDistance method? Or is it too much work, to move from from the current approach? > Source/WebCore/svg/SVGAnimatedLength.cpp:36 > + OwnPtr<SVGAnimatedType> type = SVGAnimatedType::create(AnimatedLength); > + type->setLength(new SVGLength(m_lengthMode, string)); Using SVGAnimatedType::createLength(m_lengthMode, string) seems better here. See suggestion above. > Source/WebCore/svg/SVGAnimatedLength.cpp:60 > +void SVGAnimatedLengthAnimator::calculateAnimatedValue(SVGSMILElement* smilElement, float percentage, unsigned repeat, s/repeat/repeatCount/ ? > Source/WebCore/svg/SVGAnimatedLength.cpp:62 > + bool fromPropertyInherits, bool toPropertyInherits) s/fromPropertyInherits/inheritFromValue/ ? > Source/WebCore/svg/SVGAnimatedLength.cpp:82 > + SVGLength tempFrom = SVGLength(m_lengthMode, fromLengthString); These temporary objects aren't good, I'd say try following: static inline SVGLength& sharedSVGLength(LengthMode mode, const String& valueAsString) { DEFINE_STATIC_LOCAL(SVGLength, sharedLength, ()); sharedLength.setValueAsString(mode, valueAsString); return sharedLength; } Of course you'll need to add setValueAsString(LengthMode, const String&) which does the same as the ctor SVGLength(LengthMode, const String&). This way you can just use: fromLength = sharedSVGLength(m_lengthMode, fromLengthString).value(m_contextElement); here, and avoid the cost of creating/destructing a SVGLength object. > Source/WebCore/svg/SVGAnimatedLength.cpp:89 > + SVGLength tempTo = SVGLength(m_lengthMode, toLengthString); > + toLength = tempTo.value(m_contextElement); Ditto. > Source/WebCore/svg/SVGAnimatedLength.cpp:106 > + animated->length().setValue(number, m_contextElement, ec); I'd store SVGLength& animatedLength = animated->length(); in a local variable on top. > Source/WebCore/svg/SVGAnimatedLength.h:52 > + virtual void calculateAnimatedValue(SVGSMILElement*, float percentage, unsigned repeat, > + OwnPtr<SVGAnimatedType>& fromValue, OwnPtr<SVGAnimatedType>& toValue, OwnPtr<SVGAnimatedType>& animatedValue, > + bool fromPropertyInherits, bool toPropertyInherits); Same parameter renamings should be done here. > Source/WebCore/svg/SVGAnimatedType.h:50 > + void setLength(SVGLength* length) > + { > + ASSERT(m_type == AnimatedLength); > + m_data.length = length; > + } If you add a createLength() function here, you can completely remove setLength(). > Source/WebCore/svg/SVGAnimatedType.h:57 > + You have to manually destruct the object here, the SVGAnimatedType dtor is missing, which invokes the correct operator delete, based on the m_type. Currently you're leaking. > Source/WebCore/svg/SVGAnimationElement.cpp:163 > + for (unsigned i = 0; i < m_values.size(); ++i) > + m_values[i] = m_values[i].stripWhiteSpace(); Why is this needed? > LayoutTests/svg/animations/script-tests/svglength-animation-LengthModeWidth.js:37 > + shouldBeCloseEnough("rect.width.baseVal.value", "200", 0.01); I'd remove the baseVal checks from all tests, only check the animVal, otherwhise they are wrong as soon as we implement animVal support for real.

(In reply to comment #18) > (From update of attachment 95264 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95264&action=review > > It's a great start, I still have some comments though that lead to r-: > > > Source/WebCore/ChangeLog:11 > > + > > + The goal is to move the main animation logic for SVG units from SVGAnimateElement into the corresponding SVGAnimated* files. > > + Starting with SVGLength on this patch. SVGAnimateElement will just work with the generic type SVGAnimatedType. SVGAnimator > > + coordinates the connection from SVGAnimatedType to the SVG unit animators like SVGAnimatedLengthAnimator in SVGAnimatedLength.h. > > + > > I think you should make it much more clear, that you're going away from any->string->any operation mode for animations, but rather directly animate "any" w/o string roundtrips. We stil do the any->string->any operation. This is just another step toward avoiding this operation. But sure, I can comment about this. > > > Source/WebCore/svg/SVGAnimateElement.cpp:125 > > +static inline SVGLengthMode lengthModeForAnimatedLengthAttribute(const QualifiedName& attrName) > > Does this method really belong here? How about moving it to SVGLength as static function? Will try it. > > > Source/WebCore/svg/SVGAnimateElement.cpp:448 > > + m_animator = SVGAnimator::create(AnimatedLength, targetElement); > > This is just called, once right? We're not recreating SVGAnimator on every animation step anymore, right? > Can we assert here, that m_animator is null? Are we properly destructing the animator at some point? Will do it. > > > Source/WebCore/svg/SVGAnimateElement.cpp:449 > > + static_cast<SVGAnimatedLengthAnimator*>(m_animator.get())->setSVGLengthMode(lengthModeForAnimatedLengthAttribute(attributeName())); > > I'm wondering whether this logic could be folded into the SVGAnimator::create code, by just passing the attributeName() as another constructor parameter. > If other types don't need to know the attribute name they can just ignore the third ctor param... with moving lengthModeForAnimatedLengthAttribute we could do that, yes. > > > Source/WebCore/svg/SVGAnimateElement.cpp:492 > > + m_animator = SVGAnimator::create(AnimatedLength, targetElement); > > + static_cast<SVGAnimatedLengthAnimator*>(m_animator.get())->setSVGLengthMode(lengthModeForAnimatedLengthAttribute(attributeName())); > > Ditto. > > > Source/WebCore/svg/SVGAnimateElement.cpp:545 > > + m_animatedType = SVGAnimatedType::create(AnimatedLength); > > + m_animatedType->setLength(new SVGLength(lengthModeForAnimatedLengthAttribute(attributeName()), baseString)); > > How about adding SVGAnimatedType::createLength() which takes another LengthMode and const String& parameter, and hides this code? Ok. > > > Source/WebCore/svg/SVGAnimateElement.cpp:584 > > + // TODO: Distance calculation should be done in SVGAnimatedTypeAnimator. > > Can't you fix this directly, and add a calculateDistance method? Or is it too much work, to move from from the current approach? No. This doesn't work right now. At this point the animator was not created and we can't move this operation to SVGAnimatedTypeAnimator. I'll fix this after the moving patches. > > > Source/WebCore/svg/SVGAnimatedLength.cpp:36 > > + OwnPtr<SVGAnimatedType> type = SVGAnimatedType::create(AnimatedLength); > > + type->setLength(new SVGLength(m_lengthMode, string)); > > Using SVGAnimatedType::createLength(m_lengthMode, string) seems better here. See suggestion above. Yes. > > > Source/WebCore/svg/SVGAnimatedLength.cpp:60 > > +void SVGAnimatedLengthAnimator::calculateAnimatedValue(SVGSMILElement* smilElement, float percentage, unsigned repeat, > > s/repeat/repeatCount/ ? Ok. I'll do that. > > > Source/WebCore/svg/SVGAnimatedLength.cpp:62 > > + bool fromPropertyInherits, bool toPropertyInherits) > > s/fromPropertyInherits/inheritFromValue/ ? This is not quite correct. The boolean just informs if the property can inherit or not. > > > Source/WebCore/svg/SVGAnimatedLength.cpp:82 > > + SVGLength tempFrom = SVGLength(m_lengthMode, fromLengthString); > > These temporary objects aren't good, I'd say try following: > static inline SVGLength& sharedSVGLength(LengthMode mode, const String& valueAsString) > { > DEFINE_STATIC_LOCAL(SVGLength, sharedLength, ()); > sharedLength.setValueAsString(mode, valueAsString); > return sharedLength; > } Ok. I'll give it a try. > > Of course you'll need to add setValueAsString(LengthMode, const String&) which does the same as the ctor SVGLength(LengthMode, const String&). > This way you can just use: > fromLength = sharedSVGLength(m_lengthMode, fromLengthString).value(m_contextElement); > here, and avoid the cost of creating/destructing a SVGLength object. > > > Source/WebCore/svg/SVGAnimatedLength.cpp:89 > > + SVGLength tempTo = SVGLength(m_lengthMode, toLengthString); > > + toLength = tempTo.value(m_contextElement); > > Ditto. > > > Source/WebCore/svg/SVGAnimatedLength.cpp:106 > > + animated->length().setValue(number, m_contextElement, ec); > > I'd store SVGLength& animatedLength = animated->length(); in a local variable on top. Ok. > > > Source/WebCore/svg/SVGAnimatedLength.h:52 > > + virtual void calculateAnimatedValue(SVGSMILElement*, float percentage, unsigned repeat, > > + OwnPtr<SVGAnimatedType>& fromValue, OwnPtr<SVGAnimatedType>& toValue, OwnPtr<SVGAnimatedType>& animatedValue, > > + bool fromPropertyInherits, bool toPropertyInherits); > > Same parameter renamings should be done here. I try to find a better name. > > > Source/WebCore/svg/SVGAnimatedType.h:50 > > + void setLength(SVGLength* length) > > + { > > + ASSERT(m_type == AnimatedLength); > > + m_data.length = length; > > + } > > If you add a createLength() function here, you can completely remove setLength(). > > > Source/WebCore/svg/SVGAnimatedType.h:57 > > + > > You have to manually destruct the object here, the SVGAnimatedType dtor is missing, which invokes the correct operator delete, based on the m_type. > Currently you're leaking. Oh, good catch. > > > Source/WebCore/svg/SVGAnimationElement.cpp:163 > > + for (unsigned i = 0; i < m_values.size(); ++i) > > + m_values[i] = m_values[i].stripWhiteSpace(); > > Why is this needed? I described it in the ChangeLog. Take this example: values=" 3px ; 4px;5px" This would be splitted into the following strings: " 3px", " 4px" and "5px". For SVGLength just the last value is correct. So we have to delete all whitespaces before and after the value. This was done in SVGAnimateElement for every value in the past. Even for "from", "to" and "by". But that is incorrect. If we have from=" 4px", the parser should treat this as a failure, like we do it for x=" 4px". > > > LayoutTests/svg/animations/script-tests/svglength-animation-LengthModeWidth.js:37 > > + shouldBeCloseEnough("rect.width.baseVal.value", "200", 0.01); > > I'd remove the baseVal checks from all tests, only check the animVal, otherwhise they are wrong as soon as we implement animVal support for real. Correct. Thats why I'd leave them in. This way we have of course to change a lot of tests once we differ between them, but we will have a lot of tests that make sure that they are set correctly. Btw. we do it that way on all animation tests.

Comment on attachment 95264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95264&action=review Uploading a new patch which addresses your comments above today. >>> Source/WebCore/svg/SVGAnimatedLength.cpp:62 >>> + bool fromPropertyInherits, bool toPropertyInherits) >> >> s/fromPropertyInherits/inheritFromValue/ ? > > This is not quite correct. The boolean just informs if the property can inherit or not. I think that the naming is meaningful enough. I don't think that it needs a change. >> Source/WebCore/svg/SVGAnimatedLength.cpp:106 >> + animated->length().setValue(number, m_contextElement, ec); > > I'd store SVGLength& animatedLength = animated->length(); in a local variable on top. I do it, but I don't think that it makes much sense, since it is called once anyway.

Created attachment 96878 [details] Patch

Comment on attachment 96878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96878&action=review Next round of comments, sorry: > Source/WebCore/ChangeLog:8 > + The goal is to move the main animation logic for each SVG unit from SVGAnimateElement into the corresponding SVGAnimated* files. s/SVG unit/SVG primitive datatype/ (after all 'SVGLength' is not a 'unit'). > Source/WebCore/ChangeLog:20 > + We don't support animations between different units right now: from="20px" to="20%" for example. The second benefit is, that we do not > + rewrite the parsing code for parsing the values in the attributes 'from', 'to' and 'values'. I don't understand the second benefit "that we do not rewrite the parsing code for parsing the values" ? > Source/WebCore/ChangeLog:21 > + The main benefit: Animate SVG values directly will be easier to implement, once moving the code was finished. s/Animate/Animated/. I think this is confusing -- you are referring to animVal support here, and how it's now easier to implement it as we're actually animating SVGLengths instead of number+units, so we could use that later on as "animVal' for the SVG DOM, right? Can you please rephrase this. > Source/WebCore/ChangeLog:23 > + The current patch is starting with SVGLength. SVGAnimator coordinates the connection from SVGAnimatedType to the SVG unit animators The current patch converts SVGLength to the new concept. (The patch is startin... is like "Dieser Zug endet hier" ;-) > Source/WebCore/ChangeLog:24 > + The current patch is starting with SVGLength. SVGAnimator coordinates the connection from SVGAnimatedType to the SVG unit animators > + like SVGAnimatedLengthAnimator in SVGAnimatedLength.h. SVGAnimator coordinates the connection? > Source/WebCore/svg/SVGAnimateElement.cpp:-74 > - else if (parse.endsWith("px") || parse.endsWith("pt") || parse.endsWith("em")) > - unitLength = 2; What is this about?? > Source/WebCore/svg/SVGAnimateElement.cpp:458 > + if (!m_animator) > + m_animator = SVGAnimator::create(targetElement, m_animatedAttributeType, attributeName()); > + m_animator->calculateFromAndByValues(m_fromType, m_toType, fromString, byString); Can't you move these in an ensureAnimator() method? so you could use ensureAnimator()->calculateFromAndByValues(...) > Source/WebCore/svg/SVGAnimateElement.cpp:509 > + m_animatedType = SVGAnimatedType::createLength(new SVGLength(SVGLength::lengthModeForAnimatedLengthAttribute(attributeName()), baseString)); This detail should be hidden, the SVGLength construction, it should be encapsulated. Suggestion: m_animatedType = SVGAnimatedLength::createLength(SVGLength::lengthModeForAnimatedLengthAttribute(attributeName()), baseString). Then SVGAnimatedLength::createLength does what you do now. That just hides the "new" and "delete". We want to avoid to expose that we're initentionally _Not_ using smart pointers for this SVGLength object, as it lives within an union. EEEK I've just seen you have "PassOwnPtr<SVGAnimatedType> SVGAnimatedLengthAnimator::constructFromString(const String& string)". Why don't you just use that? m_animatedType = m_animator->constructFromString(baseString)? > Source/WebCore/svg/SVGAnimateElement.cpp:573 > + if (!m_animator) > + m_animator = SVGAnimator::create(targetElement, m_animatedAttributeType, attributeName()); return ensureAnimator()->calculateDistance(this, fromString, toString); > Source/WebCore/svg/SVGAnimatedType.h:36 > + static PassOwnPtr<SVGAnimatedType> createLength(SVGLength* length) > + { > + return adoptPtr(new SVGAnimatedType(AnimatedLength, length)); > + } This should be removed. > Source/WebCore/svg/SVGAnimatedType.h:40 > + delete m_data.length; Add the same assertion regarding m_type here, as longs as we don't support other types. > Source/WebCore/svg/SVGAnimatedType.h:66 > + // FIXME: More SVGUnits need to be added step by step. s/SVGUnits/SVG primitive types/ > Source/WebCore/svg/SVGAnimationElement.cpp:163 > + for (unsigned i = 0; i < m_values.size(); ++i) > + m_values[i] = m_values[i].stripWhiteSpace(); I'd still love to see a comment here, best with a spec link. > Source/WebCore/svg/SVGAnimator.h:28 > +class SVGAnimator { I think SVGAnimator sounds way too generic how about "SVGAnimatedTypeAnimatorFactory" or short "SVGAnimatorFactory" > Source/WebCore/svg/SVGLength.cpp:140 > + setValueAsString(valueAsString, ec); ASSERT(!ec) ? what to do if an exception is raised? is that possible? > Source/WebCore/svg/SVGLength.h:91 > + void setValueAsString(const String&, SVGLengthMode); Hm, for consistency - pass on the Ec?

Comment on attachment 96878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96878&action=review >> Source/WebCore/svg/SVGAnimateElement.cpp:-74 >> - unitLength = 2; > > What is this about?? I commented in the ChangeLog. This is ainmation the parsing code for SVGLength values :-) >> Source/WebCore/svg/SVGAnimateElement.cpp:458 >> + m_animator->calculateFromAndByValues(m_fromType, m_toType, fromString, byString); > > Can't you move these in an ensureAnimator() method? > so you could use > ensureAnimator()->calculateFromAndByValues(...) Can be an inline function, yes. But ensureAnimator would still need the argument m_animatedAttributeType, otherwise we would call determineAnimatedAttributeType twice. The first time to get the correct animation typ, the second time for the animator. We can remove it, once all svg units are supported natively. >> Source/WebCore/svg/SVGAnimateElement.cpp:509 >> + m_animatedType = SVGAnimatedType::createLength(new SVGLength(SVGLength::lengthModeForAnimatedLengthAttribute(attributeName()), baseString)); > > This detail should be hidden, the SVGLength construction, it should be encapsulated. > Suggestion: > m_animatedType = SVGAnimatedLength::createLength(SVGLength::lengthModeForAnimatedLengthAttribute(attributeName()), baseString). > > Then SVGAnimatedLength::createLength does what you do now. That just hides the "new" and "delete". We want to avoid to expose that we're initentionally _Not_ using smart pointers for this SVGLength object, as it lives within an union. > > EEEK I've just seen you have "PassOwnPtr<SVGAnimatedType> SVGAnimatedLengthAnimator::constructFromString(const String& string)". > Why don't you just use that? > > m_animatedType = m_animator->constructFromString(baseString)? The animator may does not exist. Have to check if all necessary values exist. Investigate in this. To constructFromString: it also calls createLength(SVGLength::lengthModeForAnimatedLengthAttribute(attributeName()), baseString) in the background. (see comment about createLength later) >> Source/WebCore/svg/SVGAnimatedType.h:36 >> + } > > This should be removed. Why removed? This is still needed? we call it in constructFromString as well. So we still need it. Or do you suggest something different? >> Source/WebCore/svg/SVGAnimationElement.cpp:163 >> + m_values[i] = m_values[i].stripWhiteSpace(); > > I'd still love to see a comment here, best with a spec link. sure >> Source/WebCore/svg/SVGAnimator.h:28 >> +class SVGAnimator { > > I think SVGAnimator sounds way too generic how about "SVGAnimatedTypeAnimatorFactory" or short "SVGAnimatorFactory" Hopefully the last renaming. It's not fun to edit all build systems ;) >> Source/WebCore/svg/SVGLength.cpp:140 >> + setValueAsString(valueAsString, ec); > > ASSERT(!ec) ? what to do if an exception is raised? is that possible? It is not possible that we fail here. The string is generated from the computedValue of an existing CSS property (for supporting inheritance). so we can assert here. >> Source/WebCore/svg/SVGLength.h:91 >> + void setValueAsString(const String&, SVGLengthMode); > > Hm, for consistency - pass on the Ec? Would not be interesting for our needs. I wouldn't pass an EC. See comment above. It's just used to get a SVGLength from a computed CSS property value.

Created attachment 96884 [details] Patch

Comment on attachment 96884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96884&action=review Almost there.... but the ChangeLog is so confusing, I'd like to see a new version. > Source/WebCore/ChangeLog:15 > + This and the next patches will just fix the number + unit parsing. We will work with the SVG units directly internaly. typo: internally. > Source/WebCore/ChangeLog:18 > + There are some benefits with the new infrastructure. At first, the current parsing in the animation code just respects a few units (5 for SVGlength). typo: SVGLength. > Source/WebCore/ChangeLog:22 > + We don't support animations between different units right now: from="20px" to="20%" for example. The second benefit is, that we do not > + rewrite the parsing code for parsing the values in the attributes 'from', 'to' and 'values', see parseNumberValueAndUnit in SVGAnimateElement. > + The main benefit: Because we use SVG primitive datatype for animations, it will be easier to support animVal and therefore can avoid converting values > + to string. I still understand the ChangeLog, also it's not using proper english. "rewrite the parsing code for parsing the values"?? I also don't know whether we're supporting anims between different units now with your patch or not. Please post a new ChangeLog at least... > Source/WebCore/svg/SVGAnimateElement.cpp:453 > + ensureAnimator(m_animatedAttributeType)->calculateFromAndByValues(m_fromType, m_toType, fromString, byString); Why does everyone have to pass m_animatedAttirbuteType?? Just let ensureAnimator read m_animatedAttributeType directly. > Source/WebCore/svg/SVGAnimatorFactory.h:29 > + WTF_MAKE_FAST_ALLOCATED; This class is never allocated. I'd remove this and rather provide an unimplemented private constructor, so no-one can ever create an object of type SVGAnimatorFactory.

Created attachment 96919 [details] Patch

Comment on attachment 96919 [details] Patch Attachment 96919 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8837036

Comment on attachment 96919 [details] Patch Attachment 96919 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8837040

Created attachment 96927 [details] Patch

Comment on attachment 96927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96927&action=review Great patch, r=me with some final comments: > Source/WebCore/ChangeLog:8 > + For SMIL animations of attributes or CSS properties in SVG we have a lot of transformations of the values. At first the target element When running SMIL animations within SVG, we unnecessarily transform the underlying SVG primitive datatype to strings, number+units, and back. s/At first/As first step/ > Source/WebCore/ChangeLog:10 > + seperate it into a number and its unit. In the further steps we just animate the number. This number gets transformed back to a string s/separate/split/ > Source/WebCore/ChangeLog:16 > + This patch does not attend to the string transformations, but addresses the parsing of the string back to a number and unit in the s/attend/attempt to change/? This is invalid english :( > Source/WebCore/ChangeLog:19 > + SVG animation code. We shouldn't add an own parser to SVGAnimateElement but should use the responsible parsers of the SVG primitive datatypes. > + Also the current parser of SVGAnimateElement does not handle most unit types, nor is it possible to animate lists like SVGLengthList with the > + parsed content. An animation of values with different unit types is not possible: We shoulnd't add an own parser to??? There's no need to write a new parser in SVGAnimateElement to parse SVG primitive datatypes, we can instead reuse the existing ones. > Source/WebCore/ChangeLog:25 > + For the example above we would animate the rect width from 20px to 10px in 4 seconds and jump to the 10% of the view port at the end of the s/view port/viewport/ > Source/WebCore/ChangeLog:39 > + With this patch I add the AnimatorFactory and convert SVGLength animation to the new concept. SVGAnimatorFactory. > Source/WebCore/svg/SVGAnimatedLength.cpp:75 > + // FIXME: avoid string parsing. s/avoid/Avoid/ > Source/WebCore/svg/SVGAnimatedLength.cpp:96 > + number = percentage < 0.5f ? fromLength : toLength; s/.f// > Source/WebCore/svg/SVGAnimatedLength.cpp:106 > + float animatedLength = animated->length().value(m_contextElement); use animatedSVGLength here. > Source/WebCore/svg/SVGLength.cpp:586 > + if (s_lengthModeMap.isEmpty()) { I didn't ask before, but I guess you double-checked this is complete? > Source/WebCore/svg/SVGLength.h:26 > +#include "QualifiedName.h" Remove this included, just use a class forward here for QualifiedName. > Source/WebCore/svg/SVGLength.h:105 > + static SVGLengthMode lengthModeForAnimatedLengthAttribute(const QualifiedName& attrName); s/ attrName// doesn't add a lot of value. > LayoutTests/svg/animations/svglength-animation-values-expected.txt:13 > +PASS rect.width.animVal.value is 151.18 > +PASS rect.width.baseVal.value is 151.18 I hope results like this are stable across platforms... we'll see.

Committed r88663: <http://trac.webkit.org/changeset/88663>

Committed r88670: <http://trac.webkit.org/changeset/88670>