Bug 61368 - SVGAnimation should use direct unit animation for SVGLength
Summary: SVGAnimation should use direct unit animation for SVGLength
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2011-05-24 09:11 PDT by Dirk Schulze
Modified: 2011-06-22 02:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch (37.85 KB, patch)
2011-05-24 09:13 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (112.84 KB, patch)
2011-05-28 05:02 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (117.84 KB, patch)
2011-05-28 08:29 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (119.18 KB, patch)
2011-05-28 09:45 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (123.98 KB, patch)
2011-06-12 04:57 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (124.64 KB, patch)
2011-06-12 11:52 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (125.44 KB, patch)
2011-06-12 22:35 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (125.41 KB, patch)
2011-06-12 23:49 PDT, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-05-24 09:11:04 PDT
SVGAnimation should use direct unit animation for SVGLength including parsing and calculation.
Comment 1 Dirk Schulze 2011-05-24 09:13:47 PDT
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 2 Nikolas Zimmermann 2011-05-24 11:02:14 PDT
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...
Comment 3 Dirk Schulze 2011-05-28 05:02:19 PDT
Created attachment 95258 [details]
Patch
Comment 4 WebKit Review Bot 2011-05-28 05:05:12 PDT
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.
Comment 5 Dirk Schulze 2011-05-28 05:10:00 PDT
(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 6 WebKit Review Bot 2011-05-28 05:12:40 PDT
Comment on attachment 95258 [details]
Patch

Attachment 95258 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8748263
Comment 7 Early Warning System Bot 2011-05-28 05:13:47 PDT
Comment on attachment 95258 [details]
Patch

Attachment 95258 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8747312
Comment 8 Collabora GTK+ EWS bot 2011-05-28 05:20:44 PDT
Comment on attachment 95258 [details]
Patch

Attachment 95258 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8740622
Comment 9 Gyuyoung Kim 2011-05-28 05:30:32 PDT
Comment on attachment 95258 [details]
Patch

Attachment 95258 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8743525
Comment 10 WebKit Review Bot 2011-05-28 06:14:55 PDT
Comment on attachment 95258 [details]
Patch

Attachment 95258 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8748273
Comment 11 WebKit Review Bot 2011-05-28 07:03:36 PDT
Comment on attachment 95258 [details]
Patch

Attachment 95258 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8736876
Comment 12 Dirk Schulze 2011-05-28 08:29:27 PDT
Created attachment 95261 [details]
Patch
Comment 13 Collabora GTK+ EWS bot 2011-05-28 08:54:54 PDT
Comment on attachment 95261 [details]
Patch

Attachment 95261 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8667063
Comment 14 Dirk Schulze 2011-05-28 09:35:53 PDT
(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.
Comment 15 Dirk Schulze 2011-05-28 09:45:43 PDT
Created attachment 95264 [details]
Patch
Comment 16 Dirk Schulze 2011-05-28 13:36:10 PDT
Another comment. Can you add a test with keyboard inputs? would be great.
Comment 17 Dirk Schulze 2011-05-28 13:36:38 PDT
(In reply to comment #16)
> Another comment. Can you add a test with keyboard inputs? would be great.

Wrong bug, sorry.
Comment 18 Nikolas Zimmermann 2011-05-29 06:59:34 PDT
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.
Comment 19 Dirk Schulze 2011-05-29 10:24:04 PDT
(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 20 Dirk Schulze 2011-06-11 23:50:14 PDT
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.
Comment 21 Dirk Schulze 2011-06-12 04:57:01 PDT
Created attachment 96878 [details]
Patch
Comment 22 Nikolas Zimmermann 2011-06-12 09:21:51 PDT
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 23 Dirk Schulze 2011-06-12 09:52:56 PDT
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.
Comment 24 Dirk Schulze 2011-06-12 11:52:23 PDT
Created attachment 96884 [details]
Patch
Comment 25 Nikolas Zimmermann 2011-06-12 12:54:34 PDT
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.
Comment 26 Dirk Schulze 2011-06-12 22:35:51 PDT
Created attachment 96919 [details]
Patch
Comment 27 WebKit Review Bot 2011-06-12 22:51:13 PDT
Comment on attachment 96919 [details]
Patch

Attachment 96919 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8837036
Comment 28 Collabora GTK+ EWS bot 2011-06-12 23:17:44 PDT
Comment on attachment 96919 [details]
Patch

Attachment 96919 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8837040
Comment 29 Dirk Schulze 2011-06-12 23:49:51 PDT
Created attachment 96927 [details]
Patch
Comment 30 Nikolas Zimmermann 2011-06-13 09:09:12 PDT
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.
Comment 31 Dirk Schulze 2011-06-13 11:49:11 PDT
Committed r88663: <http://trac.webkit.org/changeset/88663>
Comment 32 Dirk Schulze 2011-06-13 12:13:17 PDT
Committed r88670: <http://trac.webkit.org/changeset/88670>