Last Comment Bug 734953 - Remove skew() transformation function (leaving skewX() and skewY())
: Remove skew() transformation function (leaving skewX() and skewY())
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla14
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 747637 771180 775046 775710 775763 783045 803074
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 11:27 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-10-18 06:48 PDT (History)
4 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (20.28 KB, patch)
2012-03-18 15:28 PDT, :Aryeh Gregor (away until August 15)
dbaron: review+
Details | Diff | Review
Patch v2 (20.03 KB, patch)
2012-03-19 08:57 PDT, :Aryeh Gregor (away until August 15)
ayg: review+
Details | Diff | Review
Patch v3 (20.45 KB, patch)
2012-03-19 09:26 PDT, :Aryeh Gregor (away until August 15)
ayg: review+
Details | Diff | Review
Patch v4 (25.83 KB, patch)
2012-03-20 10:42 PDT, :Aryeh Gregor (away until August 15)
dbaron: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-03-12 11:27:35 PDT
The skew() transform is misleadingly named and it's not clear what it's useful for, if anything:

http://lists.w3.org/Archives/Public/public-fx/2012JanMar/0156.html

Would Gecko be willing to try dropping support for it?  Do we know if any pages are using it?  It's interoperably implemented in all browsers, so it might well not be worth the pain to get rid of it even if it's useless.
Comment 1 :Aryeh Gregor (away until August 15) 2012-03-16 10:04:31 PDT
The spec was updated to remove skew(): https://www.w3.org/Bugs/Public/show_bug.cgi?id=16300

Do we want to remove support entirely, or just when we unprefix?
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-16 11:47:13 PDT
I think we should remove support ASAP.
Comment 3 :Aryeh Gregor (away until August 15) 2012-03-18 15:28:08 PDT
Created attachment 607015 [details] [diff] [review]
Patch v1

I looked for all mentions of "skew" in layout/style/.  Should I look anywhere else?  I'm not sure I got all the tests, but the try server will tell me that . . .
Comment 4 Mozilla RelEng Bot 2012-03-18 15:44:37 PDT
Autoland Patchset:
	Patches: 607015
	Branch: mozilla-central => try
Patch 607015 could not be applied to mozilla-central.
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patch 607015 could not be applied to mozilla-central.
patching file layout/reftests/transform/compound-1-fail.html
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-fail.html.rej
patching file layout/reftests/transform/compound-1-ref.html
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-ref.html.rej
patching file layout/reftests/transform/compound-1a.html
Hunk #1 FAILED at 14
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1a.html.rej
patching file layout/reftests/transform/reftest.list
Hunk #1 FAILED at 76
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/reftest.list.rej
unable to find 'layout/reftests/transform/skew-1-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1-ref.html.rej
unable to find 'layout/reftests/transform/skew-1a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1a.html.rej
unable to find 'layout/reftests/transform/skew-1b.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1b.html.rej
unable to find 'layout/reftests/transform/skew-2-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2-ref.html.rej
unable to find 'layout/reftests/transform/skew-2a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2a.html.rej
patching file layout/style/nsCSSKeywordList.h
Hunk #1 FAILED at 413
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSKeywordList.h.rej
patching file layout/style/nsCSSParser.cpp
Hunk #1 FAILED at 7929
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSParser.cpp.rej
patching file layout/style/nsStyleAnimation.cpp
Hunk #1 FAILED at 1005
Hunk #2 FAILED at 1108
Hunk #3 FAILED at 1523
Hunk #4 FAILED at 1564
Hunk #5 FAILED at 2024
5 out of 5 hunks FAILED -- saving rejects to file layout/style/nsStyleAnimation.cpp.rej
patching file layout/style/nsStyleTransformMatrix.cpp
Hunk #1 FAILED at 416
Hunk #2 FAILED at 590
2 out of 2 hunks FAILED -- saving rejects to file layout/style/nsStyleTransformMatrix.cpp.rej
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patching file layout/style/test/test_bug721136.html
Hunk #1 FAILED at 18
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_bug721136.html.rej
patching file layout/style/test/test_transitions_per_property.html
Hunk #1 FAILED at 1410
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_transitions_per_property.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-18 15:58:51 PDT
Comment on attachment 607015 [details] [diff] [review]
Patch v1

>-      // It would probably be nicer to animate skew in tangent space
>-      // rather than angle space.  However, it's easy to specify
>-      // skews with infinite tangents, and behavior changes pretty
>-      // drastically when crossing such skews (since the direction of
>-      // animation flips), so interop is probably more important here.

This comment (in nsStyleAnimation.cpp) still applies to skewx and skewy.  Leave it.

>@@ -2025,18 +1944,17 @@ nsStyleAnimation::AddWeighted(nsCSSPrope
>       return true;
>     }
>     case eUnit_Transform: {
>       const nsCSSValueList *list1 = aValue1.GetCSSValueListValue();
>       const nsCSSValueList *list2 = aValue2.GetCSSValueListValue();
> 
>       // We want to avoid the matrix decomposition when we can, since
>       // avoiding it can produce better results both for compound
>-      // transforms and for skew and skewY (see below).  We can do this
>-      // in two cases:
>+      // transforms and for skewY (see below).  We can do this in two cases:

Wrap "cases:" to the next line to match the existing wrapping (which is likely tw=72).

r=dbaron with those fixed
Comment 6 :Aryeh Gregor (away until August 15) 2012-03-19 08:57:05 PDT
Created attachment 607182 [details] [diff] [review]
Patch v2

(In reply to David Baron [:dbaron] from comment #5)
> Wrap "cases:" to the next line to match the existing wrapping (which is
> likely tw=72).

Is tw=72 standard at Mozilla?  I generally use tw=79.  All I found is <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Line_length>, which is somewhat nonspecific.
Comment 7 Mozilla RelEng Bot 2012-03-19 09:13:44 PDT
Autoland Patchset:
	Patches: 607182
	Branch: mozilla-central => try
Patch 607182 could not be applied to mozilla-central.
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patch 607182 could not be applied to mozilla-central.
patching file layout/reftests/transform/compound-1-fail.html
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-fail.html.rej
patching file layout/reftests/transform/compound-1-ref.html
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-ref.html.rej
patching file layout/reftests/transform/compound-1a.html
Hunk #1 FAILED at 14
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1a.html.rej
patching file layout/reftests/transform/reftest.list
Hunk #1 FAILED at 76
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/reftest.list.rej
unable to find 'layout/reftests/transform/skew-1-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1-ref.html.rej
unable to find 'layout/reftests/transform/skew-1a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1a.html.rej
unable to find 'layout/reftests/transform/skew-1b.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1b.html.rej
unable to find 'layout/reftests/transform/skew-2-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2-ref.html.rej
unable to find 'layout/reftests/transform/skew-2a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2a.html.rej
patching file layout/style/nsCSSKeywordList.h
Hunk #1 FAILED at 413
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSKeywordList.h.rej
patching file layout/style/nsCSSParser.cpp
Hunk #1 FAILED at 7929
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSParser.cpp.rej
patching file layout/style/nsStyleAnimation.cpp
Hunk #1 FAILED at 1005
Hunk #2 FAILED at 1108
Hunk #3 FAILED at 1523
Hunk #4 FAILED at 1569
Hunk #5 FAILED at 2024
5 out of 5 hunks FAILED -- saving rejects to file layout/style/nsStyleAnimation.cpp.rej
patching file layout/style/nsStyleTransformMatrix.cpp
Hunk #1 FAILED at 416
Hunk #2 FAILED at 590
2 out of 2 hunks FAILED -- saving rejects to file layout/style/nsStyleTransformMatrix.cpp.rej
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patching file layout/style/test/test_bug721136.html
Hunk #1 FAILED at 18
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_bug721136.html.rej
patching file layout/style/test/test_transitions_per_property.html
Hunk #1 FAILED at 1410
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_transitions_per_property.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Comment 8 :Aryeh Gregor (away until August 15) 2012-03-19 09:26:21 PDT
Created attachment 607192 [details] [diff] [review]
Patch v3

Oops -- it was conflicting with the bug 719054 patch I have applied locally.  I should make sure any patches I submit are bottommost before I post them.
Comment 9 Mozilla RelEng Bot 2012-03-19 09:32:30 PDT
Autoland Patchset:
	Patches: 607192
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=04c24b87c645
Try run started, revision 04c24b87c645. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=04c24b87c645
Comment 10 Mozilla RelEng Bot 2012-03-19 14:46:55 PDT
Try run for 04c24b87c645 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=04c24b87c645
Results (out of 217 total builds):
    exception: 2
    success: 159
    warnings: 40
    failure: 14
    other: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-04c24b87c645
Comment 11 :Aryeh Gregor (away until August 15) 2012-03-20 10:42:01 PDT
Created attachment 607617 [details] [diff] [review]
Patch v4

Fixed test failures in test_bug435293-skew.html.  Otherwise the same as the last version.  Will re-request review when I get a green try run.
Comment 12 Mozilla RelEng Bot 2012-03-20 12:11:56 PDT
Autoland Patchset:
	Patches: 607617
	Branch: mozilla-central => try
An error occurred while cloning https://hg.mozilla.org/mozilla-central.
Comment 13 Mozilla RelEng Bot 2012-03-20 12:54:09 PDT
Autoland Patchset:
	Patches: 607617
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=6f350f60aa0f
Try run started, revision 6f350f60aa0f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=6f350f60aa0f
Comment 14 Mozilla RelEng Bot 2012-03-20 17:17:39 PDT
Try run for ce075b94be45 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ce075b94be45
Results (out of 217 total builds):
    success: 175
    warnings: 28
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-ce075b94be45
Comment 15 Mozilla RelEng Bot 2012-03-21 06:53:49 PDT
Try run for 6f350f60aa0f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6f350f60aa0f
Results (out of 219 total builds):
    exception: 1
    success: 178
    warnings: 26
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-6f350f60aa0f
 Timed out after 12 hours without completing.
Comment 16 :Aryeh Gregor (away until August 15) 2012-03-21 08:38:57 PDT
Comment on attachment 607617 [details] [diff] [review]
Patch v4

(In reply to Aryeh Gregor from comment #11)
> Created attachment 607617 [details] [diff] [review]
> Patch v4
> 
> Fixed test failures in test_bug435293-skew.html.  Otherwise the same as the
> last version.  Will re-request review when I get a green try run.

Okay, re-requesting review for the test changes.
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-21 14:40:41 PDT
Comment on attachment 607617 [details] [diff] [review]
Patch v4

r=dbaron
Comment 18 :Aryeh Gregor (away until August 15) 2012-03-21 15:32:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b48e42774f6
Comment 19 Marco Bonardo [::mak] 2012-03-22 06:38:28 PDT
https://hg.mozilla.org/mozilla-central/rev/8b48e42774f6
Comment 20 Anthony Ricaud (:rik) 2012-04-04 05:07:13 PDT
I've noticed a broken page due to this change.

http://sudweb.fr/2012/ Below the "Inscrivez-vous" box, skew() is used to create an arrow. The source code of this website is available so I can remove it. Just wondering if this is a sign that this change is affecting real world websites.
Comment 22 :Aryeh Gregor (away until August 15) 2012-07-31 03:52:42 PDT
(FTR, the patch for bug 747637 accidentally landed with this bug's number: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1dadae3427 )

Note You need to log in before you can comment on or make changes to this bug.