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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 10 18:29:48 PDT 2008


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:

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

More information about the webkit-unassigned mailing list