[webkit-reviews] review denied: [Bug 21586] SVG CSS properties don't work with CSS transitions : [Attachment 104814] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dean Jackson
<dino at apple.com>'s request for review:
Bug 21586: SVG CSS properties don't work with CSS transitions
https://bugs.webkit.org/show_bug.cgi?id=21586

Attachment 104814: Patch
https://bugs.webkit.org/attachment.cgi?id=104814&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104814&action=review


Looks sane, still some comments from my side:

> Source/WebCore/svg/SVGLength.cpp:223
> +	   break;

All these breaks can be removed.

> Source/WebCore/svg/SVGLength.cpp:255
> -    ASSERT_NOT_REACHED();
> +    ec = NOT_SUPPORTED_ERR;

This can stay as-is, it's not possible to reach this state.

> Source/WebCore/svg/SVGLength.cpp:271
>	   break;

These breaks are superfluous now. you can remove them.

> Source/WebCore/svg/SVGLength.cpp:298
>      }
> +    ec = NOT_SUPPORTED_ERR;
> +    return 0;

Same as above, after removing all breaks; you can also reintroduce
ASSERT_NOT_REACHED()  here, as it's not possible to have any other types than
those handled in the switch. The compiler would warn about any missing, as
there's no default: case.

> Source/WebCore/svg/SVGLength.h:142
> +	       length.newValueSpecifiedUnits(LengthTypePercentage, fromPercent
+ (toPercent - fromPercent) * progress, ec);
> +	   } else {

Early return here, instead of adding an else branch.
if (ec)
    return SVGLength();
return length;
}
should be sufficient.

> Source/WebCore/svg/SVGLength.h:150
> +		       length.newValueSpecifiedUnits(toType, fromValue +
(toValue - fromValue) * progress, ec);
> +	       } else {

Same here, early-exit instead of branching.

> Source/WebCore/svg/SVGLength.h:159
> +		   if (!ec) {
> +		       float fromValue =
convertValueFromUserUnits(fromValueInUserUnits, toType, 0, ec);
> +		       if (!ec) {
> +			   float toValue = valueInSpecifiedUnits();
> +			   length.newValueSpecifiedUnits(toType, fromValue +
(toValue - fromValue) * progress, ec);
> +		       }

turn each of these branches in early returns if (ec) return SVGLength()...