Bug 66080 - Unmatrix algorithm implementation is wrong
Summary: Unmatrix algorithm implementation is wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-11 11:20 PDT by Mihnea Ovidenie
Modified: 2011-08-16 13:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.83 KB, patch)
2011-08-11 11:28 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (5.21 KB, patch)
2011-08-14 00:13 PDT, Mihnea Ovidenie
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Updated test case (2.09 KB, text/html)
2011-08-15 12:41 PDT, Dean Jackson
no flags Details
Patch 3 (5.08 KB, patch)
2011-08-16 01:26 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.