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
Created attachment 103649 [details] Patch
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.
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?
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.
(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.
Created attachment 103875 [details] Patch 2
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
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().
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
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
Comment on attachment 104015 [details] Patch 3 Rejecting attachment 104015 [details] from review queue. dino@apple.com 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.
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.
Comment on attachment 104015 [details] Patch 3 Clearing flags on attachment: 104015 Committed r93150: <http://trac.webkit.org/changeset/93150>
All reviewed patches have been landed. Closing bug.