[webkit-reviews] review granted: [Bug 18375] Make SVG animation work : [Attachment 20459] another updated patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 18375: Make SVG animation work
http://bugs.webkit.org/show_bug.cgi?id=18375

Attachment 20459: another updated patch
http://bugs.webkit.org/attachment.cgi?id=20459&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
So I'm not sure it even makes sense to do any kind of real hard review of this
patch.	Obviously this work is incomplete.

I did a cursory review anyway:


parseNumberValueAndUnit
the .endsWith stuff is about as inefficient as possible. :)  Normally we use
hash tables when you have more than 3-4 values.  In this case, we're re-walking
the string each time. :)

  if (isAdditive()) {
 97		valueToApply = ColorDistance::addColorsAndClamp(base,
m_animatedColor).name();
 98	    } else

no { } needed.

I'm in general not a big fan of shadowing:
AnimationMode animationMode = this->animationMode();


m_cachedMax = -1.;

-1 should have some name here.	The comparisons with 0 should also use a
variable of this same name.  

if (m_cachedMax.isValid())
    return m_cachedMax;


You might want to add
using SVGNames; at the top of SMILTimingElement.cpp


I don't like your use of copying parsers.  We've spent a lot of time in the
last 3 years removing copying parsers and replacing them with scaning parsers. 
I agree with you that the rest of this work is much more important than that
small issue however.


All in all, I'm very very very very glad to see this patch land.  I really hope
that you plan to push this through to completion.  This is the 3rd animation
system we've seen, and they've all been partial. :)  Hopefully this is the last
one we'll see.

We talked a lot over IRC.  You said you'd land the UnitBezier stuff separately.
 I don't need to see the updated patch.

Thanks again.