Created attachment 97717 [details] Patch

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

Created attachment 97718 [details] Patch

Comment on attachment 97718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97718&action=review Great work, I'm waiting with the r+ to wait on the mac build you've tried to fix. > Source/WebCore/ChangeLog:10 > + SVG primitive datatype SVGNumber we do not animate values with units but support exponents. we dont animate values with units but support exponents? "With the new animator for SVG Number we also support the scientific notation, and everything else that's supported by the SVGNumber parsing, removing the SVGAnimate* specific number parsing functionality". Better, eh? > Source/WebCore/svg/SVGAnimateElement.cpp:472 > + // FIXME: A return value of float is not enough to support paced animations on lists. Out of curiosity, what will be needed then? What's the difference here with paced anims on lists? > Source/WebCore/svg/SVGAnimateElement.h:-77 > - float m_fromNumber; > - float m_toNumber; > - float m_animatedNumber; Yay! > Source/WebCore/svg/SVGAnimatedNumber.cpp:76 > + float number; Can you move this below the next two ifs, to avoid confusion. > Source/WebCore/svg/SVGAnimatedNumber.cpp:91 > + number = percentage < 0.5f ? fromNumber : toNumber; s/.f/ > Source/WebCore/svg/SVGAnimatedNumber.cpp:95 > + // FIXME: This is not correct for values animation. I dislike these FIXMES (I know you only moved it). It would be better to say what's missing just like the the other FIXME regarding paced anims does. > Source/WebCore/svg/SVGAnimatedType.h:38 > + > + static PassOwnPtr<SVGAnimatedType> createAngle(SVGAngle*); > + static PassOwnPtr<SVGAnimatedType> createLength(SVGLength*); > + static PassOwnPtr<SVGAnimatedType> createNumber(float*); Great idea, to move all of that to its own .cpp file, it was too cluttered already. > Source/WebCore/svg/SVGParserUtilities.cpp:154 > - return genericParseNumber(ptr, end, number, skip); > + return genericParseNumber(ptr, end, number, skip) && ptr == end; That needs an explaination.

(In reply to comment #4) > (From update of attachment 97718 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97718&action=review > > Great work, I'm waiting with the r+ to wait on the mac build you've tried to fix. > > > Source/WebCore/ChangeLog:10 > > + SVG primitive datatype SVGNumber we do not animate values with units but support exponents. > > we dont animate values with units but support exponents? > "With the new animator for SVG Number we also support the scientific notation, and everything else that's supported by the SVGNumber parsing, removing the SVGAnimate* specific number parsing functionality". > Better, eh? > > > Source/WebCore/svg/SVGAnimateElement.cpp:472 > > + // FIXME: A return value of float is not enough to support paced animations on lists. > > Out of curiosity, what will be needed then? What's the difference here with paced anims on lists? > > > Source/WebCore/svg/SVGAnimateElement.h:-77 > > - float m_fromNumber; > > - float m_toNumber; > > - float m_animatedNumber; > > Yay! > > > Source/WebCore/svg/SVGAnimatedNumber.cpp:76 > > + float number; > > Can you move this below the next two ifs, to avoid confusion. > > > Source/WebCore/svg/SVGAnimatedNumber.cpp:91 > > + number = percentage < 0.5f ? fromNumber : toNumber; > > s/.f/ > > > Source/WebCore/svg/SVGAnimatedNumber.cpp:95 > > + // FIXME: This is not correct for values animation. > > I dislike these FIXMES (I know you only moved it). It would be better to say what's missing just like the the other FIXME regarding paced anims does. > > > Source/WebCore/svg/SVGAnimatedType.h:38 > > + > > + static PassOwnPtr<SVGAnimatedType> createAngle(SVGAngle*); > > + static PassOwnPtr<SVGAnimatedType> createLength(SVGLength*); > > + static PassOwnPtr<SVGAnimatedType> createNumber(float*); > > Great idea, to move all of that to its own .cpp file, it was too cluttered already. > > > Source/WebCore/svg/SVGParserUtilities.cpp:154 > > - return genericParseNumber(ptr, end, number, skip); > > + return genericParseNumber(ptr, end, number, skip) && ptr == end; > > That needs an explaination. From the ChangeLog: "Check if String simply consits of a number. Return false otherwise." That should be turned into a comment IMHO. So you're returning false now, even if number parsing return true, but we're not the end of the input. sth like "5e13 " (note the trailing space, will now return false?) Or is whitespace already skipped by genericParseNumber?

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 97718 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=97718&action=review > > > > Great work, I'm waiting with the r+ to wait on the mac build you've tried to fix. The bot seems to be down. I uploaded the patch today morning. It was just an unused variable. I asserted the variable but don't used it. Thats why it build on my local machine with a debug build. But because the variable is not used, it is not necessary to assert it. > > > > > Source/WebCore/ChangeLog:10 > > > + SVG primitive datatype SVGNumber we do not animate values with units but support exponents. > > > > we dont animate values with units but support exponents? > > "With the new animator for SVG Number we also support the scientific notation, and everything else that's supported by the SVGNumber parsing, removing the SVGAnimate* specific number parsing functionality". > > Better, eh? Yes, will change it. > > > > > Source/WebCore/svg/SVGAnimateElement.cpp:472 > > > + // FIXME: A return value of float is not enough to support paced animations on lists. > > > > Out of curiosity, what will be needed then? What's the difference here with paced anims on lists? For SVGRect, SVGNumberList we need a distance for every value (4 for SVGRect). This can be fixed with a Vector<float> IMHO. > > > > > Source/WebCore/svg/SVGAnimateElement.h:-77 > > > - float m_fromNumber; > > > - float m_toNumber; > > > - float m_animatedNumber; > > > > Yay! > > > > > Source/WebCore/svg/SVGAnimatedNumber.cpp:76 > > > + float number; > > > > Can you move this below the next two ifs, to avoid confusion. Sure. > > > > > Source/WebCore/svg/SVGAnimatedNumber.cpp:91 > > > + number = percentage < 0.5f ? fromNumber : toNumber; > > > > s/.f/ You mentioned it in a earlier review. All values are floats, why should I remove the f? > > > > > Source/WebCore/svg/SVGAnimatedNumber.cpp:95 > > > + // FIXME: This is not correct for values animation. > > > > I dislike these FIXMES (I know you only moved it). It would be better to say what's missing just like the the other FIXME regarding paced anims does. I'll rephrase it. > > > > > Source/WebCore/svg/SVGAnimatedType.h:38 > > > + > > > + static PassOwnPtr<SVGAnimatedType> createAngle(SVGAngle*); > > > + static PassOwnPtr<SVGAnimatedType> createLength(SVGLength*); > > > + static PassOwnPtr<SVGAnimatedType> createNumber(float*); > > > > Great idea, to move all of that to its own .cpp file, it was too cluttered already. Thanks. > > > > > Source/WebCore/svg/SVGParserUtilities.cpp:154 > > > - return genericParseNumber(ptr, end, number, skip); > > > + return genericParseNumber(ptr, end, number, skip) && ptr == end; > > > > That needs an explaination. > From the ChangeLog: "Check if String simply consits of a number. Return false otherwise." > That should be turned into a comment IMHO. You mean in an extra comment in the ChangeLog. I'll do it. > > So you're returning false now, even if number parsing return true, but we're not the end of the input. > sth like "5e13 " (note the trailing space, will now return false?) Or is whitespace already skipped by genericParseNumber? Yes correct. genericParseNumber skips whitespaces if the flag is set. We don't use it in the animation code. I added it for consistency reasons. And it is just a extra flag with a default value. So it doesn't hurt.

Created attachment 97721 [details] Patch

Just moved the float number; declaration and modified the comments according to Nikos post.

Comment on attachment 97721 [details] Patch r=me!

Committed r89220: <http://trac.webkit.org/changeset/89220>

Committed r89222: <http://trac.webkit.org/changeset/89222>