Bug 66080

Summary: Unmatrix algorithm implementation is wrong
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dino, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch 2
simon.fraser: review+, simon.fraser: commit-queue-
Updated test case
none
Patch 3 none

Description Mihnea Ovidenie 2011-08-11 11:20:45 PDT
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
Comment 1 Mihnea Ovidenie 2011-08-11 11:28:18 PDT
Created attachment 103649 [details]
Patch
Comment 2 Mihnea Ovidenie 2011-08-11 11:40:19 PDT
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 3 Simon Fraser (smfr) 2011-08-11 12:17:12 PDT
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?
Comment 4 WebKit Review Bot 2011-08-11 12:31:31 PDT
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.
Comment 5 Mihnea Ovidenie 2011-08-12 23:26:54 PDT
(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.
Comment 6 Mihnea Ovidenie 2011-08-14 00:13:36 PDT
Created attachment 103875 [details]
Patch 2
Comment 7 Dean Jackson 2011-08-14 05:16:06 PDT
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 8 Simon Fraser (smfr) 2011-08-15 12:25:02 PDT
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().
Comment 9 Dean Jackson 2011-08-15 12:41:36 PDT
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
Comment 10 Mihnea Ovidenie 2011-08-16 01:26:54 PDT
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 11 WebKit Review Bot 2011-08-16 04:17:58 PDT
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 12 Dean Jackson 2011-08-16 04:36:47 PDT
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 13 WebKit Review Bot 2011-08-16 04:37:00 PDT
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 14 WebKit Review Bot 2011-08-16 13:45:05 PDT
Comment on attachment 104015 [details]
Patch 3

Clearing flags on attachment: 104015

Committed r93150: <http://trac.webkit.org/changeset/93150>
Comment 15 WebKit Review Bot 2011-08-16 13:45:11 PDT
All reviewed patches have been landed.  Closing bug.