[Webkit-unassigned] [Bug 18375] Make SVG animation work

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=18375


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #20459|review?                     |review+
               Flag|                            |




------- Comment #9 from eric at webkit.org  2008-04-10 18:29 PDT -------
(From update of attachment 20459)
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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.