[webkit-reviews] review denied: [Bug 26544] Transition animation fails to render correctly at http://www.ferretarmy.com/ (singular matrices) : [Attachment 31547] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 26544: Transition animation fails to render correctly at
http://www.ferretarmy.com/ (singular matrices)
https://bugs.webkit.org/show_bug.cgi?id=26544

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> ===================================================================
> --- WebCore/platform/graphics/mac/GraphicsLayerCA.mm	(revision 44828)
> +++ WebCore/platform/graphics/mac/GraphicsLayerCA.mm	(working copy)
> @@ -981,6 +981,15 @@ bool GraphicsLayerCA::animateTransform(c
>		   if (isMatrixAnimation) {
>		       TransformationMatrix t;
>		       curValue.value()->apply(size, t);
> +		       
> +		       // If any matrix is singular, CA won't animate it
correctly. So fall back to software animation
> +		       if (!t.isInvertible()) {
> +			   [timesArray release];
> +			   [valArray release];
> +			   [tfArray release];
> +			   return false;
> +		       }

Maybe use OwnPtrs for those arrays so we don't have to worry about releasing
them?

Also, you can't return here without END_BLOCK_OBJC_EXCEPTIONS. I'd prefer a
'break' out of the loop, to avoid additional returns from this method.

>		       CATransform3D cat;
>		       copyTransform(cat, t);
>		       [valArray addObject:[NSValue
valueWithCATransform3D:cat]];
> @@ -1004,6 +1013,12 @@ bool GraphicsLayerCA::animateTransform(c
>		   TransformationMatrix fromt, tot;
>		   valueList.at(0).value()->apply(size, fromt);
>		   valueList.at(1).value()->apply(size, tot);
> +
> +		   // If any matrix is singular, CA won't animate it correctly.
So fall back to software animation
> +		   if (!fromt.isInvertible())
> +		       return false;
> +		   if (!tot.isInvertible())
> +		       return false;

Same issue about returning without END_BLOCK_OBJC_EXCEPTIONS.