Bug 66080 – Unmatrix algorithm implementation is wrong

• Component: CSS
• Status: RESOLVED FIXED

[Mihnea Ovidenie]

The implementation of unmatrix algorithm in TransformationMatrix.cpp, function decompose is wrong. The spec contains the corrected version of the algorithm:
http://dev.w3.org/csswg/css3-2d-transforms/Overview.html#unmatrix

[Mihnea Ovidenie]

Created attachment 103649 [details]
Patch

[Mihnea Ovidenie]

The implementation is wrong at the step where the matrix and the scale factors must be negated. The current implementation only negates scaleX while leaving scaleY and scaleZ untouched.

[Simon Fraser (smfr)]

Comment on attachment 103649 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103649&action=review

> LayoutTests/animations/animation-matrix-negative-scale.html:74
> +                setTimeout("snapshot(0)", 500);
> +                setTimeout("snapshot(1)", 1000);
> +                setTimeout("snapshot(2)", 1500);
> +                setTimeout("snapshot(3)", 1900);

A test that takes almost 2 seconds is not acceptable. Can't you test this change with WebKitCSSMatix somehow?

[WebKit Review Bot]

Attachment 103649 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihnea Ovidenie]

(In reply to comment #3)
> (From update of attachment 103649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103649&action=review
> 
> > LayoutTests/animations/animation-matrix-negative-scale.html:74
> > +                setTimeout("snapshot(0)", 500);
> > +                setTimeout("snapshot(1)", 1000);
> > +                setTimeout("snapshot(2)", 1500);
> > +                setTimeout("snapshot(3)", 1900);
> 
> A test that takes almost 2 seconds is not acceptable. Can't you test this change with WebKitCSSMatix somehow?

I will change the test, thx. for the review.

[Mihnea Ovidenie]

Created attachment 103875 [details]
Patch 2

[Dean Jackson]

Comment on attachment 103875 [details]
Patch 2

I like the new test. My only comment would be that the test file name could related to the fix - something like animation-matrix-unmatrix-scale

[Simon Fraser (smfr)]

Comment on attachment 103875 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=103875&action=review

> LayoutTests/animations/animation-matrix-negative-scale.html:54
> +            function stopAnimation()
> +            {
> +                document.getElementById("box").style.webkitAnimationName = "";
> +            }

Why is this necessary? snapshot() does notifyDone().

[Dean Jackson]

Created attachment 103945 [details]
Updated test case

There are a couple of issues with the test case. I've uploaded a new test.

- no need for quotes around the keyframe
- I needed to add a to rule for the animation to fail on TOT as it stands
- no need for stopAnimation

[Mihnea Ovidenie]

Created attachment 104015 [details]
Patch 3

Reworked patch, sorry for the delay, i was in vacation. I also renamed the test to animation-matrix-negative-scale-unmatrix.html

[WebKit Review Bot]

Comment on attachment 104015 [details]
Patch 3

Rejecting attachment 104015 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Dean Jackson]

Comment on attachment 104015 [details]
Patch 3

I wasn't listed in the reviewers section. I've updated that but the queue might not pick it up for a while. I'll check again tomorrow if this doesn't stick.

[WebKit Review Bot]

Comment on attachment 104015 [details]
Patch 3

Rejecting attachment 104015 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 104015 [details]
Patch 3

Clearing flags on attachment: 104015

Committed r93150: <http://trac.webkit.org/changeset/93150>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.