505115 - CSS3 3D-Transforms

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Jean-Yves Perrier [:teoli]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090719 Minefield/3.6a1pre
Build Identifier: 

Extension of the CSS3 Transforms; this looks very interesting.

Apple implemented it in its iPhone2 and is in the progress of adding it to Safari ( http://webkit.org/blog/386/3d-transforms/ )

Reproducible: Always

Comment 1

[d]

This is now working in the Webkit nightlies, which means that it will "soon" be implemented in Safari and Google Chrome.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Safari has this on Mac but not on Windows. Chrome doesn't have this on any platform.

Comment 3

[d]

What is the reason that Webkit has for making it Mac only as of now? Might the same thing limit our implementation?

Comment 4

[Trevor Downs]

According to this: https://bugs.webkit.org/show_bug.cgi?id=27314 it is now implemented on Windows.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Very much WIP patch queue at http://hg.mozilla.org/users/mwoodrow_mozilla.com/3d-transforms/

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Fix OpenGL container layer to support transforming children

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Make nsDisplayTransform create a Container layer

Creates a ContainerLayer for nsDisplayTransform objects so rendering of them can be accelerated when an accelerated backend is enabled.

This gives me noticable improvements in rendering http://azarask.in/projects/tabcandy/scale/css-animation.html (within a MakeGoFaster window) - cc'ing Aza.

Will push these to try once I figure out how.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Looks like this failed a few tests on try, will debug tomorrow and update.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you need to do what I did for opacity: add a new style change hint to nsChangeHint, report that change hint when the transform changes, mark the frame active, etc. See changeset d98f8a21727e and d290d2b97416.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2 v2: Layerify nsDisplayTransform

Fixed reftests failures.
Fixed roc's suggestions to get layers timing out working correctly.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Everything looks good except

+      visibleRect.x = 0;
+      visibleRect.y = 0;
+      visibleRect.width = INT_MAX;
+      visibleRect.height = INT_MAX;

This isn't really right, the visible rect might need to extend to negative coordinates.

If nsLayoutUtils::GfxRectToIntRect fails, it's because the visible rect is nearly infinite ... i.e., the transform has scaled the content down to an incredibly small size. In fact, the content will be shrunk to be basically invisible. Is that right? In that case, why not just set the visible rect to empty?

Seems to me that would work in most cases, except for situations where you have one extreme transform with a child element with another extreme transform that cancels out the first one.

So another thing we might want to do is to teach FrameLayerBuilder to use an internal scale factor (or possibly horizontal and vertical scale factors). For example, if we have a transform that scales everything up by 2, we could make all the child layers work at twice the resolution, and draw into the ThebesLayers with a scale applied to the context. This would give better quality results too. We would have to use some heuristics so we didn't change the scale factor too often, so we don't have to rerender content too often. We'd still have this problem of extreme visibleRects for video and maybe canvas and other leaf non-Thebes layers, but we can make those invisible using the first approach with no bad consequences.

If this makes sense, I suggest we go with the empty-rect approach for now, and file a bug to implement the second approach.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2 v3: Layerify nsDisplayTransform

Use an empty visible rect for invalid transforms - Try server says yes.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix

No API changes to the class, just reworked internals in preparation

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Upgrade gfx3DMatrix

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Use gfx3DMatrix in layout

This changes larges amounts of layout to directly use gfx3DMatrix wherever possible.

This more or less concludes the preparation patches for 3d transforms.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 461956 [details] [diff] [review]
Part 2 v3: Layerify nsDisplayTransform

Need dbaron review on style system changes

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 461956 [details] [diff] [review]
Part 2 v3: Layerify nsDisplayTransform

r=dbaron on the style system changes

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in part 2: http://hg.mozilla.org/mozilla-central/rev/9a45bd27ec75

Comment 19

[José Jeria]

Could this have caused bug 584494?

Comment 20

[Matt Woodrow (:mattwoodrow)]

Yes, I'll look into that today.

Comment 21

[Ryan Jones-Ward [:sciguyryan]]

Just a little curiosity on my part here - how likely is this to make it into Firefox 4?

Comment 22

[Matt Woodrow (:mattwoodrow)]

I'm hopeful! Slowly crossing things off my list of things to implement and bugs to fix. The end is in sight!

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It might be worth starting to request code reviews on the parts that you think are done.

Comment 24

[Bobby Holley (:bholley)]

Pushed part 1 to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d91b0d9fd0a2

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 461958 [details] [diff] [review]
Part 4: Upgrade gfx3DMatrix

I would strongly suggest that we use an externally-developed and tested matrix library instead of growing our own.

If you want something minimalist that fits in one file and has code that's very easy to understand, use CImg:
    http://cimg.sourceforge.net/

If you want to know what I'd really recommend, it's Eigen (disclaimer, I'm a contributor to it):
    http://eigen.tuxfamily.org/

Comment 26

[Joe Drew (not getting mail)]

This doesn't seem required for the OpenGL reftest bug - probably a mistake?

Comment 27

[Paul Irish]

webkit's implementation also includes a media query: @media (-webkit-transform-3d)

This comes in rather handy for feature detection as checking something like
   'webkitPerspective' in document.body.style
will false positive in recent webkit versions without the graphics-side support.

I know with Firefox's multitouch it also exposes a similar -moz-touch-enabled media query. 
Are you planning to include a mediaQuery for your 3D implementation, not only for feature detection but for better style scoping for authors?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Including a media query sounds like a good idea.

Comment 29

[Ahmed Aly]

is this dead ?
could it reach firefox 4?

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v2

Fixed bitrot.

Comment 31

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Upgrade gfx3DMatrix v2

Fixed more bitrot.

Comment 32

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Use gfx3DMatrix in layout v2

More of the same.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  UntransformRect(mVisibleRect, mFrame, ToReferenceFrame(), &untransformedVisible);

What if this fails? We need at least a comment explaining what happens if we can't untransform.

+  if (disp->mTransform.IsFlat() &&

Let's call this PreservesAxisAlignedRectangles.

+  nsRect untransformedVisible;
+  UntransformRect(mVisibleRect, mFrame, ToReferenceFrame(), &untransformedVisible);

Again, probably need a comment explaining what happens for a singular matrix.

+  if (matrix.IsSingular() || !matrix.Is2D())
+    return PR_FALSE;

It's probably faster to check Is2D and get the 2D matrix, then check whether the 2D matrix is invertible. Maybe add a method Is2DAndInvertible?

+  static gfx3DMatrix GetResultingTransformMatrix(const nsIFrame* aFrame,
                                                const nsPoint& aOrigin,
                                                float aFactor,
                                                const nsRect* aBoundsOverride = nsnull);

Fix indent.

+nsStyleTransformMatrix::IsFlat() const
+{
+  if (_12 == 0.0f && _13 == 0.0f && _14 == 0.0f &&
+      _21 == 0.0f && _23 == 0.0f && _24 == 0.0f &&
+      _31 == 0.0f && _32 == 0.0f && _33 == 1.0f &&
+      _34 == 0.0f && _43 == 0.0f && _44 == 1.0f)
+    return PR_TRUE;

Should probably just call Is2D and then gfxMatrix::PreservesAxisAlignedRectangles.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 525558 [details] [diff] [review]
Part 4: Upgrade gfx3DMatrix v2

Please use #include <algorithm> instead of nsAlgorithm

Comment 35

[Matt Woodrow (:mattwoodrow)]

Progress update:

I now have a large part of the spec completed and working, with the GL layers backend.

http://hg.mozilla.org/users/mwoodrow_mozilla.com/3d-transforms-new/

The remaining ToDo list (in approximate order) is:

1) Get the patches cleaned up and ready to land (pref'd off) in a way that won't break unsupported configurations
2) Tests tests tests
3) Add -moz-perspective-origin a z component to -moz-transform-origin
4) D3D9/10 Backend support
5) Software backend support
6) -moz-transform-style
7) DOM interfaces
8) Transitions/Animations

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 525557 [details] [diff] [review]
Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v2

So I haven't dug into this in a whole lot of detail yet, but I do definitely know one major thing that's wrong, and I may as well tell you about that sooner rather than later.  (It could even be causing bugs that you're observing...)

The whole mX/mY business is to handle percentage values of translateX() and translateY(), which are percentages relative to the width and height of the element.  At the time we compute the style data, we don't know the element's width and height -- that gets computed during layout.  So the basic idea of what we've done for 2D is record three separate matrices:  the first is the part that's fixed (mMain and mDelta), the second is to be multiplied by the element's width (mX), and the third to be multiplied by the element's height (mY).

In 2-D transforms, the only way to introduce a height-relative transform is in the Y translation component, the only way to introduce a width-relative transform is in the X translation component, and, importantly, the only other matrix components that the operators in 2-D transforms allow the X translation and Y translation components to be copied to is each other (via skew or rotate).

But 3-D transforms has more operators, which means that you're pretty obviously not storing enough data.  For example, if you have rotateY(45deg) translateX(10%), you're going to rotate some of that translation into data that you simply don't have a place to store, since you don't have a place to store width-relative translation on the Z axis.



That reminds me of a second issue, which I'm less sure about what should happen:  the issue of units.  WebKit's publication of the transforms spec sort of papered over the fact that CSS doesn't have a "primary" unit type for lengths -- lengths can have different units.  In the 2-D transform matrix, the components of the transform matrix technically have the following units:

[  unitless   unitless    length   ]
[  unitless   unitless    length   ]
[  1/length   1/length   unitless  ] (but these are fixed 0 0 1)

In the 3-D transform matrix, the units really look like this:

[  unitless  unitless  length/Z   length  ]
[  unitless  unitless  length/Z   length  ]
[  Z/length  Z/length  unitless      Z    ]
[  1/length  1/length     1/Z    unitless ]

It's not clear to me that we want to continue storing those two "length" values as nscoord while storing all the rest as floats, especially given the variety of units in this matrix.  But we do certainly need to be careful about units and what conversions are expected.  And 3-D transforms do introduce the new problem (not present in 2-D transforms) that there's an assumption that CSS pixels are the canonical unit of length and that there's some unnamed canonical unit of Z distance.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #36)
> But 3-D transforms has more operators, which means that you're pretty obviously
> not storing enough data.  For example, if you have rotateY(45deg)
> translateX(10%), you're going to rotate some of that translation into data that
> you simply don't have a place to store, since you don't have a place to store
> width-relative translation on the Z axis.

And, in particular, the fact that 3-D transforms has the arbitrary 16 value matrix3d() function means that you could end up with width-relative and height-relative values in all 16 components of the matrix.  (With 2-D transforms, the two cells that were always 0 prevented them from spreading through most of the matrix.)

Comment 38

[Matt Woodrow (:mattwoodrow)]

Thanks David, I should have spotted that.

This is pushing the limit of my matrix knowledge, is there any way we can represent all the information we need, similar to what the current code is doing?

From what I can see, once the relative data has spread across the matrix there will be no easy way to store everything.

We could keep a linked list of matrix objects and only multiply them together once we can evaluate the length/width values, but this would require heap allocations and feels somewhat ugly.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The current approach is still quite doable:  you just need 48 components total:  16 for the "constant", 16 for the width-relative, and 16 for the height-relative.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, actually, no, it's not, since you can have quadratic terms now too.

So I think we'll just need to change to storing the list of transforms in computed style and only computing them into a (16-term) matrix once we know the width.  We probably then want to cache the matrix in a frame property.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, actually, we *already* store the specified transform as nsStyleDisplay::mSpecifiedTransform (since we already need it for animations), so we'd just need to stop storing nsStyleDisplay::mTransform.  Then we'd need to compute the transform later on (maybe in nsDisplayTransform?  I don't think we should need it at any stage before display lists), and then possibly cache it somewhere (if we do, we'll also need to clear that cache appropriately -- but I'm actually thinking that caching is probably unnecessary).  We largely wouldn't need nsStyleTransformMatrix anymore, except we might want to keep it around as a bunch of static methods that produce a gfx3DMatrix from the transform function data.

Or something like that -- it depends on what works out well.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need to know the transform during reflow to calculate the right overflow area in FinishAndStoreOverflow.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Of course, we have the width and height there. So a utility method to compute the final transform would be fine. I agree we don't even need to cache it on the frame, just cache it in the nsDisplayTransform.

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, since you're redoing much of the code for interpolation of transforms anyway, you should read
http://lists.w3.org/Archives/Public/www-style/2010Oct/0440.html
(a proposal on how to do it better than what's currently in the spec).

Comment 45

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v3

Huge rewrite to keep transforms as a list of transform functions until the bounds information becomes available.

I'm not sure if the test for transform changed is suitable and I'm getting a few transform reftest failures (anti-aliasing differences) that I think are caused by this being hit and forcing active (and GPU composited) layers.

We also probably want to cache the gfxMatrix inside nsDisplayTransform, how can I tell when the tranform and/or bounds have changed so that we can dump the cached value?

This passes all reftests from transforms/ and css-transitions/ (aside from the above mentioned compositing differences), but I still need to check animations tests.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #45)
> We also probably want to cache the gfxMatrix inside nsDisplayTransform, how can
> I tell when the tranform and/or bounds have changed so that we can dump the
> cached value?

Display list items are recreated every time we paint (or hit-test, or whatever) and styles and layout can't change during that time, so there's no problem.

Comment 47

[Aaron Cajes]

Is this going to land in Aurora for Firefox 5 or Firefox 6? hmmm..

Comment 48

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v4

Fixed crashes and quite a few test failures running test_transitions_per_property.html.

How do we handle compute distance with non matching transform lists? I'm a bit stuck on how to defer this calculation until the bounds information is available.

We still have 17 failures in the test, 7 are rounding issues (greater than round_error_ok allows), the other 10 are distance calculations failing (since the code for them doesn't do anything).

This is on top of the above mentioned anti-aliasing failures with reftests.

Comment 49

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v5

This now passes all tests on tryserver.

I had to disable all tests that attempted to compute a distance with non-matching transform lists (as suggested).

Comment 50

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Upgrade gfx3DMatrix v3

Fixed review comment and moved most implementations into a new gfx3DMatrix.cpp file.

Carrying forward r=jrmuizel.

Comment 51

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Use gfx3DMatrix in layout v3

Rebased and hopefully fixed review comments.

Note that we should never actually get any 3d transforms with this current code, everything should pass Is2D().

Later patches add 3d transforms, and also implement proper untransforming of these.

Comment 52

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Implement the 3d -moz-transform functions

Adds the 3d transform functions to nsStyleTransformMatrix.

Needs parts 7 and 8 to function correctly, just broken up for easier review.

Comment 53

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Layers support for 3d transforms

Add support in layers for 3d transforms.

We need to adjust the z scale of the viewport transform to 0 because all values must be in the -1 to 1 range, and we have an infinite far plane.

Comment 54

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8: Add ray tracing to untransform 2d points on a 3d plane

Let us actually untransform 3d points wherever possible.

With the pref (layout.3d-transforms-enabled) off (as it is by default), we pass all tryserver tests with this patch queue.

Parts 9 - 12 are still waiting on tryserver results, and possibly more fixes.

I plan to land this as soon as possible to try catch any regressions, and prevent them from being bitrotted again.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 537016 [details] [diff] [review]
Part 5: Use gfx3DMatrix in layout v3

Review of attachment 537016 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 537018 [details] [diff] [review]
Part 7: Layers support for 3d transforms

Review of attachment 537018 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 537019 [details] [diff] [review]
Part 8:  Add ray tracing to untransform 2d points on a 3d plane

Review of attachment 537019 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfx3DPoint.h
@@ +101,5 @@
> +        *this /= Length();
> +    }
> +};
> +
> +#endif /* GFX_3DPOINT_H */ 
\ No newline at end of file

Does it make sense to make a Base3DPoint class like BasePoint, and instantiate it here for gfx3DPoint? I think it probably does.

Comment 58

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8: Add ray tracing to untransform 2d points on a 3d plane v2

Renamed gfx3DPoint to gfx3DVector (I think it makes more sense given the operations we use it for).

Added Base3DVector.

Comment 59

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 9 - Implement the perspective() transform function and style property.

Comment 60

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 10 - Implement -moz-backface-visible

Comment 61

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 11 - Make -moz-transform-origin also support a z component.

Comment 62

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 12 - Implement -moz-perspective-origin.

All green on tryserver up to this point (with the pref disabled).

Took a guess at appropriate reviewers for each piece, happy for anyone to change them.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 538995 [details] [diff] [review]
Part 8: Add ray tracing to untransform 2d points on a 3d plane v2

Review of attachment 538995 [details] [diff] [review]:
-----------------------------------------------------------------

Let's break this patch up into the 3DVector stuff, the matrix changes, and the layout changes.

For Base3DVector, "3D" should be last in the name for consistency with Azure and because we can't use 3DVector as a class name.

Also, per discussion on IRC this should be BasePoint3D. We'll have to review it carefully to keep the API as consistent with BasePoint as makes sense.

::: gfx/src/Base3DVector.h
@@ +124,5 @@
> +      return sqrt(x*x + y*y + z*z);
> +  }
> +
> +  void Normalize() {
> +      *this /= Length();

What if Length() is zero? At least document what the behavior is.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 538998 [details] [diff] [review]
Part 11 - Make -moz-transform-origin also support a z component.

Review of attachment 538998 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on everything outside of layout/style, needs dbaron review for the style changes

::: layout/style/nsCSSParser.cpp
@@ +7524,5 @@
> +  if (!ParseBoxPositionValues(position, PR_TRUE))
> +    return PR_FALSE;
> +
> +  PRBool allow3D =
> +      mozilla::Preferences::GetBool("layout.3d-transforms.enabled", PR_FALSE);

I think we should cache this somewhere. Use Preferences::AddBoolVarCache.

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe you can break out the style system changes in parts 11 and 12 into separate patches from the code that actually uses the new properties?

Comment 66

[Joe Drew (not getting mail)]

<joe> fwiw i'm not really in favour of having vector3 and point3 be the same class
<joe> what happens when you say matrix4 * vector3?
<joe> if you don't have a difference between vector and point types, you don't know whether to use the translation components of the matrix or not

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Benoit, what do you say to that?

Comment 68

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #67)
> Benoit, what do you say to that?

We had a conversation in Real Life (tm) with Joe and we're now in agreement on the set of point/vector classes that we want. Only have one class for points/vectors with 3 coords (x,y,z). Call that e.g. Point3D. Then only have one class for points/vectors with 4 homogeneous coords (x,y,z,w). Call that e.g. Point4D.

It doesn't matter to me whether they're called 'point' or 'vector', what matters to me is that we have only one non-homogeneous class and one homogeneous class, and we're now in agreement on that.

Comment 69

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #63)
> ::: gfx/src/Base3DVector.h
> @@ +124,5 @@
> > +      return sqrt(x*x + y*y + z*z);
> > +  }
> > +
> > +  void Normalize() {
> > +      *this /= Length();
> 
> What if Length() is zero? At least document what the behavior is.

FWIW I consider that to be purely a documentation issue i.e. I don't think that the code should perform any check. I've found that it's just simpler to design vector/matrix classes in such a way that they behave as similarly as possible to plain floats. Surely the compiler doesn't generate any code to protect against divisions by zero, and neither should we.

Comment 70

[Joe Drew (not getting mail)]

(In reply to comment #68)
> It doesn't matter to me whether they're called 'point' or 'vector', what
> matters to me is that we have only one non-homogeneous class and one
> homogeneous class, and we're now in agreement on that.

They should be called Point and Vector; optional 3D and 4D. ;)

Comment 71

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #70)
> (In reply to comment #68)
> > It doesn't matter to me whether they're called 'point' or 'vector', what
> > matters to me is that we have only one non-homogeneous class and one
> > homogeneous class, and we're now in agreement on that.
> 
> They should be called Point and Vector; optional 3D and 4D. ;)

Ah, so we still disagree: I don't agree with having Points with non-homogeneous coordinates vs Vectors with homogeneous coordinates.

In homogeneous coordinates there is no notion of 'length of a vector', since if you multiply all homogeneous coords by 2 it's still the same point. So in a proposal where the only vectors would have homogeneous coords, we'd be able to represent only _unit_ vectors. I am not comfortable with such a limitation.

=== My proposal ===

Vector3D class has 3 coords (x,y,z). You can also simply call it 'Vector'
Vector4D class has 4 homogeneous coords (x,y,z,w). You can also call it 'HVector', where H stands for Homogeneous.

No distinction between 'points' and 'vectors'.

Comment 72

[Benoit Jacob [:bjacob] (mostly away)]

Forgot to reply to that:

(In reply to comment #66)
> <joe> fwiw i'm not really in favour of having vector3 and point3 be the same
> class
> <joe> what happens when you say matrix4 * vector3?
> <joe> if you don't have a difference between vector and point types, you
> don't know whether to use the translation components of the matrix or not

As I see it, Vector3D would be for ordinary 3D points, not for directions at infinity, so if one had implicit casting to homogeneous coords, (x,y,z) would unambiguously become (x,y,z,1). So there is no ambiguity, once this is agreed on.

However, regardless of that, matrix4 * vector3 would have to return a vector4 since the matrix4 can have anything on the fourth row.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #71)
> Vector3D class has 3 coords (x,y,z). You can also simply call it 'Vector'
> Vector4D class has 4 homogeneous coords (x,y,z,w). You can also call it
> 'HVector', where H stands for Homogeneous.

The name should be Point3D, for consistency with 2D points.

Point3D and Point4D sound like the best approaches for now.

Comment 74

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #73)
> (In reply to comment #71)
> > Vector3D class has 3 coords (x,y,z). You can also simply call it 'Vector'
> > Vector4D class has 4 homogeneous coords (x,y,z,w). You can also call it
> > 'HVector', where H stands for Homogeneous.
> 
> The name should be Point3D, for consistency with 2D points.
> 
> Point3D and Point4D sound like the best approaches for now.

I don't know what's the plan with 2D points, but I feel that it's best to pick names that reflect the difference between affine (x,y,z) and homogeneous (x,y,z,w) coordinates. Homogeneous means that we're doing projective geometry i.e. (x,y,z,w) is the same point as (a*x,a*y,a*z,a*w) for any nonzero a. So maybe my suggestion of Vector4D vs Vector3D was not good, as it wrongly suggests that the only difference is the number of coords.

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well then, Point3D vs HPoint3D?

Comment 76

[Benoit Jacob [:bjacob] (mostly away)]

Why not. Or maybe PointH3D. Instead of H for homogeneous, the difference can also be represented by a P for Projective.

(note: i've been assuming that we wanted to have a class for points with (x,y,z,w) projective coordinates, I don't know exactly what the needs here are, I'm not implying we should).

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

HPoint3D, PPoint3D, PointH3D, PointP3D ... all work for me. Someone pick one.

Comment 78

[Benoit Jacob [:bjacob] (mostly away)]

PointH3D it is then.

Comment 79

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8a: Add BasePoint3D and gfxPoint3D

Comment 80

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8b: Add 3D Point support, and ray tracing to gfx3DMatrix

Comment 81

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8c: Use ray tracing to untransform 2d points on a 3d plane.

Comment 82

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 10 - Implement -moz-backface-visible

s/gfx3DVector/gfxPoint3D

Comment 83

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 11a: Add nsCSSValueTriplet and optionally read a z component to -moz-transform-origin

Comment 84

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 11b: Layout changes to use a z component for -moz-transform-origin

Comment 85

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 12a: Implement -moz-perspective-origin style property.

Comment 86

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 12b: Layout changes to use -moz-perspective-origin

Comment 87

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 540957 [details] [diff] [review]
Part 11b: Layout changes to use a z component for -moz-transform-origin

Carrying forward r=roc

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540952 [details] [diff] [review]
Part 8a: Add BasePoint3D and gfxPoint3D

Review of attachment 540952 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/BasePoint3D.h
@@ +62,5 @@
> +  bool operator==(const Sub& aPoint) const {
> +    return x == aPoint.x && y == aPoint.y && z == aPoint.z;
> +  }
> +  bool operator!=(const Sub& aPoint) const {
> +    return x != aPoint.x || y != aPoint.y || z != aPoint.z;

For future reference, I usually find it easier and safer to write !(*this == aOther), but you needn't bother changing it.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540953 [details] [diff] [review]
Part 8b:  Add 3D Point support, and ray tracing to gfx3DMatrix

Review of attachment 540953 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfx3DMatrix.h
@@ +117,5 @@
> +   */
> +  gfxPoint3D Transform3D(const gfxPoint3D& point) const;
> +
> +  gfxPoint ProjectPoint(const gfxPoint& aPoint) const;
> +  gfxRect ProjectRect(const gfxRect& aRect) const;

Call this ProjectRectBounds since it takes the bounding rect.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540954 [details] [diff] [review]
Part 8c: Use ray tracing to untransform 2d points on a 3d plane.

Review of attachment 540954 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +512,5 @@
> +  
> +
> +  static nsRect InvertRectToAncestor(nsIFrame* aFrame,
> +                                     const nsRect& aRect,
> +                                     nsIFrame* aStopAtAncestor);

InvertRectToAncestorBounds? InvertRectBoundsToAncestor?

Why not just TransformRectToBoundsInAncestor?

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540959 [details] [diff] [review]
Part 12b: Layout changes to use -moz-perspective-origin

Review of attachment 540959 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +2389,5 @@
> +  }
> +  
> +  /* Default position of perspective-origin is in the centre of the element */
> +  result.x -= NSAppUnitsToFloatPixels(boundingRect.width, aFactor)/2;
> +  result.y -= NSAppUnitsToFloatPixels(boundingRect.height, aFactor)/2;

This seems wrong. The initial value of the property is 50% 50%, and this should be what's in "display" by default, so why do we need to calculate it again?

@@ +2394,5 @@
> +
> +  result.x = -result.x;
> +  result.y = -result.y;
> +
> +  return result;

return -result;

Comment 92

[Matt Woodrow (:mattwoodrow)]

> > +  
> > +  /* Default position of perspective-origin is in the centre of the element */
> > +  result.x -= NSAppUnitsToFloatPixels(boundingRect.width, aFactor)/2;
> > +  result.y -= NSAppUnitsToFloatPixels(boundingRect.height, aFactor)/2;
> 
> This seems wrong. The initial value of the property is 50% 50%, and this
> should be what's in "display" by default, so why do we need to calculate it
> again?
> 

A value of (0,0) has the effect of applying perspective with the vanishing point in the centre of the object. So we need to adjust the point so that (50%, 50%) returns (0,0).

Excluding possible typos, this is what WebKit does and it appears to look correct.

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540959 [details] [diff] [review]
Part 12b: Layout changes to use -moz-perspective-origin

Review of attachment 540959 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 94

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 537017 [details] [diff] [review]
Part 6: Implement the 3d -moz-transform functions

So this treats translateZ() the Z component of translate3d() differently from what the spec says:  the spec says it's a <length> rather than a <number>.  What does WebKit do?

ParseSingleTransforms argument should be aIs3D rather than is3d; I spent a while trying to figure out what is3D was used for before realizing it was an out-param.

That said, rather than having a separate switch statement inside ParseSingleTransform, I think you should just pass aIs3D into GetFunctionParseInformation and set it there for each case.

In ParseMozTransform:
 * you really really really should cache this preference somewhere rather than getting it every time
 * your introduction of |prev| doesn't appear to be used for anything; remove it

I'm going to wait to look at the nsStyleTransformMatrix stuff in this patch until going through the other one again...

Comment 95

[Matt Woodrow (:mattwoodrow)]

Thanks dbaron, I'll fix those and upload a new patch soon.

I'm currently stuck on calcuating overflow areas/visible regions etc for layers with -moz-preserve-3d set.

Consider the (relatively) simple case of

Parent Frame - preserve3d - rotateY(90deg)
  - Child Frame 1 - No transform
  - Child Frame 2 - RotateY(90deg).

The current GetBounds implementation calls GetBounds on each child, Union()'s the result and then transforms it into local coordinate space. This returns a 0 sized area since Child Frame 2 (CF2) is essentially singular in 2d space, as is Parent Frame (PF).

My solution for finding the bounds of PF, is to instead apply the transformation to each child's GetBounds() result and create a Union of these areas. In the case where the preserve-3d flag is set, and the specific child is also an nsDisplayTransform, we skip the transformation step. When calculating the transformation to use in an nsDisplayTransform, we check for parent frames with preserve-3d and multiply the transforms together.

In short CF2::GetBounds() is returning an area in PF's coordinate space instead of it's own.

The piece I'm stuck on is what to return for CF2::GetBounds() when not being used as a component of PF::GetBounds(). Can I just always return an area in the parents coordinate system? The 'actual' bounds of CF2 is 0 sized, and results in the object not being drawn.

I believe HitTest, ComputeVisibility and FinishAndStoreOverflow are essentially the same problem.

For FinishAndStoreOverflow, will storing the area in parents coordinate space cause the frame size for the parent to be changed? We don't want to transform it again.

Ideas appreciated :)

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

preserve-3d always confuses me.

Can we pretend the parent frame simply has no transform at all, and propagate its transform down to its children every time we compute the used transform value?

Comment 97

[Matt Woodrow (:mattwoodrow)]

I might be able to do this for some cases.

Where does the size value come from for nsIFrame::FinishAndStoreOverflow?

The problem is that children that *don't* have a transform need to be transfomed, yet children that do can probably include the transform with theirs and transform themselves.

If the area given to FinishAndStoreOverflow is the sum of the child areas, then sections of it would need to be transformed, while others not. I'm not sure how to achieve this.

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

aNewSize is computed by Reflow(), it's the border-box size which is about to be returned for the frame by Reflow(). It has not yet been set on the frame. It does *not* include child areas.

aOverflowAreas contains the visual and scrollable overflow areas for this frame, which includes those areas for the child frames. But if you need to, you can recompute the child frames' contribution by iterating over all child frames and asking for their overflow areas, then transforming them, then you can add that back in to the passed-in aOverflowAreas.

Comment 99

[Matt Woodrow (:mattwoodrow)]

I'm still struggling with the handling of preserve-3d. Slow progress is slow.

I had an idea for another possible approach to solving this:

Given a frame structure like:

parent frame - preserves3d - rotatey(90deg)
   - child frame - rotatey(90deg)
   - child frame - not transformed


The current code will generate a display item tree that looks like:

nsDisplayTransform - rotatey(90deg) - preserve3d
    - nsDisplayTransform - rotatey(90deg)
    - nsDisplayText - whatever this content was

Is it instead possible to generate:

nsDisplayList
    - nsDisplayTransform - rotatey(180deg)
    - nsDisplayTransform - rotatey(90deg)
        - nsDisplayText - contentcontentcontent

This looks like it will show the correct rendering for this limited example (not sure if nsDisplayList is the correct 'container') and won't require any changes in visibility calculation, hit testing etc.

Are there any problems with this approach that I'm missing?

Comment 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds good.

So if you have
parent frame - preserves3d - rotatey(90deg)
   - child frame - opacity:0.5
       - child frame - not transformed
       - child frame - rotatey(90deg)
   - child frame - not transformed

You'd build
nsDisplayList
    - nsDisplayOpacity - opacity:0.5
        - nsDisplayTransform - rotatey(90deg)
            - nsDisplayText - contentcontentcontent
        - nsDisplayTransform - rotatey(180deg)
    - nsDisplayTransform - rotatey(90deg)
        - nsDisplayText - contentcontentcontent
?

Comment 101

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think that approach should work, and I think it shouldn't be too hard to implement. In BuildDisplayListForStackingContext, when the frame has preserves-3d, you can walk the child display list and do whatever surgery you want to do.

Comment 102

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess you would walk the display items in the child list:
-- for any nsDisplayTransforms, just fold in the extra transform from the parent
-- for any nsDisplayOpacity or nsDisplayWrapList, recursively walk over the children (since opacity commutes with transforms)
-- for any nsDisplayClip ... I dunno! Introduce a new nsDisplayClipPath which generalizes nsDisplayClipRoundedRect and can clip to an arbitrary path, and transform the rectangle to a quad, replace the nsDisplayClip with an nsDisplayClipPath for that quad, and recursively walk over the children? FrameLayerBuilder would have to know about nsDisplayClipPath. This could probably be done as a followup bug, if we do it at all, since preserving 3D through clipping is reasonably esoteric
-- for any other display item, wrap it in an nsDisplayTransform with the parent's transform

Comment 103

[Matt Woodrow (:mattwoodrow)]

I have good news and bad news.

This approach seems to work fairly well, I've got it working and rendering the safari 'poster circle' demo correctly.

The problem (found by looking at some of the other safari demos) is that animations/transitions of preserve-3d needs to take the child transformation into account.

Example:

Parent - -moz-transition: -moz-transform 2s; - preserve3d - transformed
  Child - transformed.

If we change the transformation on the child, it should transition over 2 seconds.

I'm not sure how to implement this without moving the preserve-3d handling further down again, onto the actual frames.

Will think about this further.

Comment 104

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #103)
> The problem (found by looking at some of the other safari demos) is that
> animations/transitions of preserve-3d needs to take the child transformation
> into account.
> 
> Example:
> 
> Parent - -moz-transition: -moz-transform 2s; - preserve3d - transformed
>   Child - transformed.
> 
> If we change the transformation on the child, it should transition over 2
> seconds.

Surely this is a Webkit bug? That behavior makes no sense to me.

Comment 105

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what's the plan here with the preference and how we enable this?  Patch 6 introduces a preference for 3D transforms, defaulting to off.

Is there going to be a situation where, on some machines, we have the ability to use graphics capabilities that make doing 3D transforms reasonable, and on others we don't?  When we enable 3D transforms, do you expect we'll be able to enable it reasonably across all machines, or will we need to enable it only on some of them?

Comment 106

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It depends on the use-case. The software implementation is presumably a lot slower, but it might be OK for some kinds of usage.

Comment 107

[Matt Woodrow (:mattwoodrow)]

The rapid release guidlines (as I understand them) encourage the use of the preferences to disable new features if needed. I am working on a software rendering implementation, performance of this is yet to be tested.

I want to land code with the preference disabled so we can get some user testing, with lower risk of regressions.

Do you have any ETA on being able to review the first few of these patches? Parts 3-5 seem the most likely to cause regressions (and can't be trivially turned off/backed out), so I'd like to get these baking on m-c as soon as possible (and before they bitrot).

Comment 108

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 537013 [details] [diff] [review]
Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v5

Why does gfx3DMatrix use |float| when gfxMatrix uses |double|?  It seems
like these ought to be consistent with each other, at least.  I suspect
this is responsible for the various rounding errors this patch
introduces (especially since the code in nsComputedDOMStyle is using
gfxMatrix, and is therefore using nsString::AppendFloat(double) rather
than nsString::AppendFloat(float).

>+    PRBool IsIdentity() const {
>+       return xx == 1.0f && yx == 0.0f &&
>+              xy == 0.0f && yy == 1.0f &&
>+              x0 == 0.0f && y0 == 0.0f;
>+    }

Drop the 'f' suffixes, since xx, yx, etc., are double rather than float.

>-    (newOrigin + toMozOrigin, disp->mTransform.GetThebesMatrix(bounds, aFactor));
>+    (newOrigin + toMozOrigin, nsStyleTransformMatrix::ReadTransforms(disp->mSpecifiedTransform,
>+                                                                     aFrame->GetStyleContext(),
>+                                                                     aFrame->PresContext(),
>+                                                                     dummy, bounds, aFactor));

Find a way to wrap before the 80th column.

>+  const gfxMatrix& newTransformMatrix = GetTransform(mFrame->PresContext()->AppUnitsPerDevPixel());

Break after the = to fit within 80 columns.

It would be really nice if nsDisplayTransform::BuildLayer didn't need to
use AppUnitsPerDevPixel() (unlike all the other callers) and thus
require the mCachedFactor business.  Might be nice to clean that up in a
followup.

nsCSSValueList::CloneExisting() is a confusing name for what you've
done.  Instad, call it CloneInto, since it's the *argument* that
receives the clone.  Also add a comment to nsCSSValue.h saying what it
does.  Finally, reuse Clone() so that the contents of the method are
just:
  aList->mValue = mValue;
  aList->mNext = mNext ? mNext->Clone() : nsnull;
which also fixes the bug in your implementation that it doesn't
overwrite a non-null next pointer with a null one.  Finally, make it
assert at the start that aList->mNext is null (otherwise it would leak).


nsComputedDOMStyle.cpp:

As I said above, I think it's bogus to serialize these numbers as
doubles when they're stored as float accuracy.

Patch 5 doesn't fix this, either.

Furthermore, something in this patch series needs to make the 3-D parts
of the matrix show up in computed style by serializing to matrix3d
rather than just matrix, at least in 3D cases.  I'd have expected that
in patch 5 or 6 (probably 6?), but it doesn't seem to be there.

>+  /* Now, we need to convert the matrix into a string.

Not useful; remove the comment.

nsStyleAnimation.cpp:

Rather than inserting "squareDistance += 42", I propose that instead you
replace the entire eUnit_Transform case within
nsStyleAnimation::ComputeDistance with "return PR_FALSE;", and
simultaneously remove the check_distance call in
test_transform_transition() in
layout/style/test/test_transitions_per_property.html .


Please document AddTransformMatrix as taking gfxMatrix parameters in CSS
pixels (in .h and .cpp), since it produces eCSSUnit_Pixel output based
on the translation coordinates in the gfx matrix.  I'm still trying to
figure out if nsStyleTransformMatrix::ProcessInterpolateMatrix has a
scaling error here (when dev pixels != css pixels).
 ... WRITE MORE HERE ...

In AddDifferentTransformLists:

>+  nsCSSValueList* list = arr->Item(2).SetListValue();
>+  aList1->CloneExisting(list);

I think this is clearer on one line (especially if CloneExisting is
renamed to CloneInto):
  aList1->CloneInto(arr->Item(2).SetListValue());

>+        ///XXX: If the matrix contains only numbers then we could decompose here.
>+        // If it contains % values, I think we can too, less sure about
>+        // calc values though.

A bunch of things here:
 - first line goes past column 80
 - use "// FIXME:" rather than "///XXX:".
 - % and calc() are pretty much equivalent; it's the same for both.
 - However, I'd add that dealing with % and calc() is not possible with
   matrix3d(), and having consistency between matrix() and matrix3d() is
   valuable

>         while ((*resultTail)->mNext) {
>-          resultTail = &(*resultTail)->mNext;
>+            resultTail = &(*resultTail)->mNext;
>         }

Leave this as it was.

nsStyleAnimation.h:

Please forward-declare gfxMatrix rather than including the header.

nsStyleStruct.cpp:

-    if (mTransform != aOther.mTransform)
+    if (mSpecifiedTransform != aOther.mSpecifiedTransform)

This should be
  !mSpecifiedTransform != !aOther.mSpecifiedTransform ||
  (mSpecifiedTransform && *mSpecifiedTransform != *aOther.mSpecifiedTransform)
since you want deep comparison of the lists rather than pointers.

nsStyleTransformMatrix.cpp:

Could you rename the aFactor param to pretty much everything to be
called aAppUnitsPerMatrixUnit or something like that?  Otherwise I think
it's not clear that it can be used to create a matrix in either device
or CSS pixels.  Furthermore, you need to fix the comments above
SetToTransformFunction in the .h file -- that currently says it requires
device pixels, but most of the time you actually give it CSS pixels.
Really, it picks which units the matrix is in terms of.  (That said, as
I said above, I think it would be good to clean up that inconsistency.)

Please wrap the last line of ProcessTranslatePart.

In nsStyleTransformMatrix::ProcessMatrix could you call the matrix
result rather than temp?

>+  nsAutoPtr<nsCSSValueList> result;
>+  nsCSSValueList **resultTail = getter_Transfers(result);
>+
>+  *resultTail = nsStyleAnimation::AddTransformMatrix(matrix1, coeff1,
>+                                                     matrix2, coeff2);

Instead, write:

  nsAutoPtr<nsCSSValueList> result(nsStyleAnimation::AddTransformMatrix(
                                     matrix1, coeff1, matrix2, coeff2));

That said, this is rather silly -- AddTransformMatrix now painstakingly
builds up a list from its decomposed data only to have its only caller
immediately decompose that list.  AddTransformMatrix should probably
just return another matrix.  Maybe you do that in a later patch...?  If
not, could you do it as another patch on top of this queue, if not just
in this one?

I hope all the gfx3DMatrix return values from
nsStyleTransformMatrix::Process* don't lead to too much struct copying.
Have you checked?

>-  ProcessSkewHelper(xSkew, ySkew, aMain);
>+  return ProcessSkewHelper(xSkew, ySkew);;

No double ;, please.

Could you rename SetToTransformFunction to MatrixForTransformFunction?

r=dbaron with all of that fixed

Comment 109

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, even after patch 5, I think there are still some pieces that are awkwardly 2-D only (like animation).  That'll need to be fixed before this is enabled, but it's ok for now.

Comment 110

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #108)
> Why does gfx3DMatrix use |float| when gfxMatrix uses |double|?  It seems
> like these ought to be consistent with each other, at least.  I suspect
> this is responsible for the various rounding errors this patch
> introduces (especially since the code in nsComputedDOMStyle is using
> gfxMatrix, and is therefore using nsString::AppendFloat(double) rather
> than nsString::AppendFloat(float).

gfxMatrix is double because that's what cairo uses.

I think gfx3DMatrix uses floats because they're far better supported on GPUs than doubles.

Comment 111

[Matt Woodrow (:mattwoodrow)]

Yep, we use floats matrices for both OpenGL and D3D*. I think the loss of precision is probably better than creating a gfxDouble3DMatrix and converting back to floats at draw time, but I'm open to suggestions.

Animations are definitely awkward with this patch, but I have a followup patch in my queue that implements these properly.

Was the " ... WRITE MORE HERE ..." comment meant to be removed or is still an intentional placeholder?

Thanks for the review dbaron!

Comment 112

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #111)
> Was the " ... WRITE MORE HERE ..." comment meant to be removed or is still
> an intentional placeholder?

Er, yeah, one of us should figure out if there's actually a bug there, though it sort of looks like it to me.

Comment 113

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 537017 [details] [diff] [review]
Part 6: Implement the 3d -moz-transform functions


...continued from comment 94.

We should add support for number values on translatex(), translatey(),
translate(), translate3d(), and the last 2 parts of matrix() -- and
treat them as pixels.  You should do that as part of this bug --
otherwise this patch leaves things in an oddly inconsistent state.

>+  /* There are several cases to consider.
>+   * Next, the values might be lengths, or they might be percents.  If they're
>+   * percents, store them in the dX and dY components.  Otherwise, store them in
>+   * the main matrix.
>+   */

You copied this from somewhere, but it no longer belongs.  If the
original is still there, delete that too.

>+  temp._11 = 1 + (1 - cosTheta) * (x * x - 1);
>+  temp._12 = -z * sinTheta + (1 - cosTheta) * x * y;
>+  temp._13 = y * sinTheta + (1 - cosTheta) * x * z;
>+  temp._14 = 0.0f;
>+  temp._21 = z * sinTheta + (1 - cosTheta) * x * y;
>+  temp._22 = 1 + (1 - cosTheta) * (y * y - 1);
>+  temp._23 = -x * sinTheta + (1 - cosTheta) * y * z;
>+  temp._24 = 0.0f;
>+  temp._31 = -y * sinTheta + (1 - cosTheta) * x * z;
>+  temp._32 = x * sinTheta + (1 - cosTheta) * y * z;
>+  temp._33 = 1 + (1 - cosTheta) * (z * z - 1);
>+  temp._34 = 0.0f;
>+  temp._41 = 0.0f;
>+  temp._42 = 0.0f;
>+  temp._43 = 0.0f;
>+  temp._44 = 0.0f;

According to the spec _44 should be 1.0f, not 0.0f.

>+  case eCSSKeyword_scale3d:
>+    return ProcessScale3D(aData);
>   case eCSSKeyword_scale:
>     return ProcessScale(aData);

Could you stick scale3d() after scale() for consistency with the rest?


r=dbaron with that fixed, though I probably want to look at the pref caching
code once you address that

Comment 114

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Note that the number values thing is probably easiest as a separate patch -- if you merge it with an existing one I'd want to review it.)

Comment 115

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 538996 [details] [diff] [review]
Part 9 - Implement the perspective() transform function and style property.

>Bug 505115 - Part 9 - Implement the perspective() transform function and style property. try: -b d -p linux -u mochitest-4 -t none

You don't want try syntax in your commit message.

I'd also write it as:

Implement the perspective() transform function and the perspective CSS property.

As per patch 6, I think you should cache the pref (which also has value in
encapsulating the "are 3d transforms enabled" decision to one place, which
could be valuable if it's more than just a pref check in future).

>+CSS_PROP_DISPLAY(
>+    -moz-perspective,
>+    _moz_perspective,
>+    CSS_PROP_DOMPROP_PREFIXED(Perspective),
>+    CSS_PROPERTY_PARSE_VALUE,
>+    VARIANT_NONE|VARIANT_INHERIT|VARIANT_NUMBER,
>+    kDisplayKTable,
>+    offsetof(nsStyleDisplay, mChildPerspective),
>+    eStyleAnimType_float)

_moz_perspective -> perspective (don't prefix internals)
kDisplayKTable -> nsnull
put spaces around the |s


The mPerspective and mChildPerspective stuff is really broken -- it's 
going to produce incorrect results in a lot of cases due to your code
not disabling sharing in cases where you'd need to disable sharing.
Rather than explain the whole thing, I'm just going to say you should 
rip it out.  Make the style struct contain the computed value of the 
property for that element (what's now mChildPerspective).  When you want
the parent's perspective, look at 
parentFrame->GetStyleContext->GetParent()->GetStyleDisplay()->mPerspective
(except you'll need to null-check the GetParent() call).


I didn't really look too closely at rest of the code (though the whole 
thing with the perspective transform function seems pretty weird to me
-- I haven't dug into that), since this is pretty clearly wrong and 
needs substantial reworking because of the mPerspective/mChildPerspective
thing being broken.

Comment 116

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #115)
> When you want
> the parent's perspective, look at 
> parentFrame->GetStyleContext->GetParent()->GetStyleDisplay()->mPerspective
> (except you'll need to null-check the GetParent() call).

er, I meant
frame->GetStyleContext()->GetParent() (not parentFrame -- otherwise you'd be walking up twice).

Comment 117

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 540955 [details] [diff] [review]
Part 10 - Implement -moz-backface-visible

>Bug 505115 - Part 9 - Implement the perspective() transform function and style property.

How about calling it Part 10, and using an appropriate commit message
for part 10.

>+CSS_PROP_DISPLAY(
>+    -moz-backface-visibility,
>+    _moz_backface_visibility,
>+    CSS_PROP_DOMPROP_PREFIXED(BackfaceVisibility),
>+    CSS_PROPERTY_PARSE_VALUE,
>+    VARIANT_HK,
>+    kBackfaceVisibilityKTable,
>+    offsetof(nsStyleDisplay, mBackfaceVisible),
>+    eStyleAnimType_EnumU8)

_moz_backface_visibility -> backface_visibility (don't prefix internals)


Please call the member of nsStyleDisplay mBackfaceVisibility to
match the property name rather than calling it mBackfaceVisible.

>+#define NS_STYLE_BACKFACE_VISIBLE                   1
>+#define NS_STYLE_BACKFACE_HIDDEN                    0

The convention here suggests you should insert VISIBILITY_ into these
names (since they're property-value).

>+    COMPUTED_STYLE_MAP_ENTRY_LAYOUT(_moz_backface_visibility,       MozBackfaceVisibility),

This isn't layout-dependent, drop the "_LAYOUT".

I'm puzzled by the nsStyleAnimation changes -- I don't see anything in
the transitions or 3d transform specs saying the property is
transitionable.  If it is, though, I'd have expected it to work like
'visibility'... which seems to be what you test for, despite doing the
code entirely differently.  What does WebKit do?  If the goal is to work
like visibility, why not do it the same way in nsStyleAnimation?

In nsStyleDisplay::CalcDifference, why is reflow required?  I'd have
expected repaint and maybe layer stuff.

In property_database.js, please test that "collapse" is not an accepted
value (put it in invalid_values).  (That's the value for 'visibility'
that's not valid here.)

>+    // Convert to two vectors on the surface of the plane.
>+    gfxPoint3D ab = a - b;
>+    gfxPoint3D ac = a - c;
>+
>+    return ab.CrossProduct(ac);

I find this a little confusing.  ab is the *negative* of the
transformation of the unit Y axis vector, and ac is the *negative* of
the transformation of the unit X axis vector.  It seems clearer to use
b-a and c-a.

Also, it seems really weird that your definition of "normal vector" is
something that, with no transformation, points along the negative Z
axis.  Is this some standard terminology, or would it make more sense to
take ac.CrossProduct(ab)?

I presume you've tested that this matches WebKit in the case of
scaleZ(-1).  The spec seems totally unclear, and it ought to be
clarified (i.e., that it's the results of transforming the X and Y axes
that matter, rather than the result of transforming the Z axis).  One of
us should email www-style about that.

>+  gfxPoint3D normal = newTransformMatrix.GetNormalVector();
>+  if (newTransformMatrix.IsSingular() ||
>+      (mFrame->GetStyleDisplay()->mBackfaceVisible  == NS_STYLE_BACKFACE_HIDDEN &&
>+       normal.DotProduct(gfxPoint3D(0,0,1)) >= 0.0)) {
>+      return nsnull;
>+  }

Seems better to skip the variable |normal| and just put the whole
expression inside the if().

Also, don't use two spaces before the ==.

Also, just check if normal.z >= 0 and skip the DotProduct().

And if you change GetNormalVector() to work the way that makes sense to
me, make it <= 0 here :-)

And please add a comment in gfx3DMatrix.h saying what GetNormalVector does.


Marking as review- because I'd like to look at this again for the
animation issues; otherwise I'd be fine with you making the changes
without my looking again.

Comment 118

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 540956 [details] [diff] [review]
Part 11a: Add nsCSSValueTriplet and optionally read a z component to -moz-transform-origin

>+  if (CheckEndProperty() || !ParseVariant(depth, VARIANT_AHL, nsnull) || !allow3D) {

VARIANT_AHL is wrong; as far as I can tell it should just be
VARIANT_LENGTH | VARIANT_CALC.

Additionally, you should explicitly not look for a third value if the
first value has unit inherit or initial.

Also, wrap before the 80th column.

You need (again) to cache the preference.

I'd probably have preferred use of nsCSSValue::Array over creating
nsCSSValueTriplet, but I suppose I can live with this approach.

In nsCSSValue.h, could you make Triplet 51, and bump the current 51-56,
to be consistent with the order you use in the code?

You need to add a case to nsCSSValue::operator==, which you missed.

I'm concerned that you serialize (in the specified value serialization;
the computed value serialization is fine, and also has fewer constraints
about needing to be faithful to what was specified) a transform-origin
without a third value by adding that value, since we've generally tried
to serialize to the most supported (i.e., oldest) syntax.  One way to do
that is to use the eCSSUnit_Null on the Z value to distinguish these
cases; I think that would require adjusting a few assertions
(nsCSSValue::SetTripletValue's assertion about null)...  though
assertions already inconsistent with what's handled in
nsCSSValueTriplet::AppendToString.  (The animation code can still
maintain the invariant that the third value is non-null... though when
the resulting value is zero it should *set* is as null.)  There are
probably other options for handling this as well, but that's the way
that seems obvious to me.

In nsCSSValue.h:
>+    nsCSSValueTriplet(const nsCSSValue& aXValue, const nsCSSValue& aYValue, const nsCSSValue& aZValue)
>+        : mXValue(aXValue), mYValue(aYValue), mZValue(aZValue)

Please wrap before column 80.

In nsRuleNode.cpp, don't give SetTripletCoords a general name; it's
specific to transform origin since it has special treatment of the Z
component.  I tend to think it's probably best to inline it into the
caller.

nsStyleAnimation.cpp:

I'd like to look at this more closely after you address the
serialization stuff with the third value.  A little more sharing of code
that you're copying from the pair case would be nice, though.

nsStyleStruct.h:

>-  nsStyleCoord mTransformOrigin[2]; // [reset] percent, coord, calc
>+  nsStyleCoord mTransformOrigin[3]; // [reset] percent, coord, calc

comment should explain that the third value can't be a percent


I'd like to look at this again once you address the third-value serialization issue, so marking review-.

Comment 119

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 540958 [details] [diff] [review]
Part 12a: Implement -moz-perspective-origin style property.

>diff --git a/dom/interfaces/css/nsIDOMCSS2Properties.idl b/dom/interfaces/css/nsIDOMCSS2Properties.idl

You need to bump the IID here.

(And I forgot to mention that for 2 other patches, but it applies there too!)

>+PRBool CSSParserImpl::ParseMozPerspectiveOrigin()

The indentation of this function is weird.  Also, I think you should
share ParseMozTransformOrigin and add a property argument (and use it
for whether to do the third value), rather than have two separate
almost-identical functions.

> CSS_PROP_DISPLAY(
>+    -moz-perspective-origin,
>+    _moz_perspective_origin,

No _moz_ here, again.

>+    CSS_PROP_DOMPROP_PREFIXED(PerspectiveOrigin),
>+    CSS_PROPERTY_PARSE_FUNCTION |
>+    CSS_PROPERTY_STORES_CALC,

Please indent the line after the | following local style.

>+    COMPUTED_STYLE_MAP_ENTRY_LAYOUT(_moz_perspective_origin, MozPerspectiveOrigin),

The use of _LAYOUT here is correct (unlike the last time, when I pointed
out it was wrong).  Just pointing out that you shouldn't change this one
too.

nsRuleNode.cpp:

Please match the local 2-space indent.

property_database.js:

Indentation here is wacky.

r=dbaron with that fixed

Comment 120

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #108)
> 
> nsComputedDOMStyle.cpp:
> 
> As I said above, I think it's bogus to serialize these numbers as
> doubles when they're stored as float accuracy.
> 
> Patch 5 doesn't fix this, either.

I've added this to patch 5 now.

> 
> Furthermore, something in this patch series needs to make the 3-D parts
> of the matrix show up in computed style by serializing to matrix3d
> rather than just matrix, at least in 3D cases.  I'd have expected that
> in patch 5 or 6 (probably 6?), but it doesn't seem to be there.

This is currently in a much later part of the patch queue, I can split this out and bring it forward if needed.

> 
> >+  /* Now, we need to convert the matrix into a string.
> 
> Not useful; remove the comment.
> 
> nsStyleAnimation.cpp:
> 
> Rather than inserting "squareDistance += 42", I propose that instead you
> replace the entire eUnit_Transform case within
> nsStyleAnimation::ComputeDistance with "return PR_FALSE;", and
> simultaneously remove the check_distance call in
> test_transform_transition() in
> layout/style/test/test_transitions_per_property.html .

Alright, done!


> 
> That said, this is rather silly -- AddTransformMatrix now painstakingly
> builds up a list from its decomposed data only to have its only caller
> immediately decompose that list.  AddTransformMatrix should probably
> just return another matrix.  Maybe you do that in a later patch...?  If
> not, could you do it as another patch on top of this queue, if not just
> in this one?

Good point, made this this change too.

> 
> I hope all the gfx3DMatrix return values from
> nsStyleTransformMatrix::Process* don't lead to too much struct copying.
> Have you checked?

Looking at the assembly for my debug mac build, it appears that it is constructing the temporary gfx3DMatrix objects directly into a pointer passed by the caller, and not copying the result at all.

Comment 121

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Convert nsStyleTransformMatrix to be backed by a 4x4 matrix v6

Fixed review comments, and rebased against tip.

Comment 122

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Use gfx3DMatrix in layout v4

Fixed serialization of gfx3DMatrix objects to stay as floats. Rebased against tip/Part 3 changes.

Carrying forward r=roc

Comment 123

[Matt Woodrow (:mattwoodrow)]

Landed parts 3-5 on inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/7e16ec834b15
http://hg.mozilla.org/integration/mozilla-inbound/rev/92bd75756f43
http://hg.mozilla.org/integration/mozilla-inbound/rev/89f90f9fac80

Comment 124

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Implement the 3d -moz-transform functions v2

Fixed review comments, including caching the preference.

Carrying forward r=dbaron

Comment 125

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Layers support for 3d transforms v2

Rebased against tip/previous changes.

Carrying forward r=roc

Comment 126

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 9 - Implement the perspective() transform function and style property. v2

Fixed review comments and changed handling of perspective() function to match the spec properly.

Comment 127

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 10 - Implement -moz-backface-visible v2

Fixed review comments, removed animations code entirely.

Comment 128

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 11a: Add nsCSSValueTriplet and optionally read a z component to -moz-transform-origin v2

Fixed review comments

Comment 129

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 12a: Implement -moz-perspective-origin style property. v2

Fixed review comments, carrying forward r=dbaron

Comment 130

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 13: Add basic reftests for 3d transforms and expose 3d transform status in GfxInfo

Initial set of basic tests for 3d transforms, that really don't test enough yet. Still thinking of more ideas to test perspective etc.

Comment 131

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 14a: Add -moz-transform-style CSS property

Comment 132

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 14b: Layout changes for preserve-3d

Comment 133

[Matt Woodrow (:mattwoodrow)]

The preserve-3d patches raises a few interesting question, which also probably need to go through www-style.

The depth sorting in the spec is very vague, although this is intentional I think it needs to be clarified. Currently I've implemented sorting based on the z translate component of the matrix (same as Chromium does/did).

One interesting case is elements that contain the preserve-3d flag, but no actual transform. Webkit is treating these as still 'transformed' and is including any parent perspective into child transforms. This could also be considered as having children of a preserve-3d frame being brought up into the frame space (if that makes sense).

The other problem case is where there is multiple levels of preserve-3d (such as the WebKit poster circle demo). This has 12 elements transformed into a ring, 3 rings transformed into a cylinder, and the whole cylinder rotates. My depth sorting code arranges elements within a single ring correctly, and each ring correctly, but elements can still weirdly overlap between rings. WebKit appears to be sorting all 36 elements by depth, which seems to go against the fact that a transform creates a stacking context/containing block I'm not sure what the correct behaviour should be, but I this needs to be clarified in the spec.

Comment 134

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 15 - Add 4D Vectors, Quaternions and gfx3DMatrix functions

Comment 135

[Marco Bonardo [:mak]]

part 3 http://hg.mozilla.org/mozilla-central/rev/7e16ec834b15
part 4 http://hg.mozilla.org/mozilla-central/rev/92bd75756f43
part 5 http://hg.mozilla.org/mozilla-central/rev/89f90f9fac80

Comment 136

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 540953 [details] [diff] [review]
Part 8b:  Add 3D Point support, and ray tracing to gfx3DMatrix

>+gfxRect gfx3DMatrix::ProjectRect(const gfxRect& aRect) const

It would be nice to add a comment explaining what is the rect returned by this function, since it's not just returning the transformed rect (that wouldn't be a gfxRect). If I understand the code correctly, it returns the smallest gfxRect containting the image of aRect under the transformation.

Comment 137

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 15 - Add 4D Vectors, Quaternions and gfx3DMatrix functions v2

Fixed a few bugs

Comment 138

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 16 - Implement transitions/animations for 3d transforms.

Comment 139

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 17 - Add style tests for the new transform functions, and transitions

Initial testing for 3d transitions/animations. I really need to add more tests here, working on it.

Comment 140

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 548076 [details] [diff] [review]
Part 9 - Implement the perspective() transform function and style property. v2

>diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp

>+#include "mozilla/Preferences.h"

No need for this include anymore.

>+      parentDisp->mChildPerspective != 0.0) {

Per spec, you should test > 0.0, since negative values should also lead to no perspective.

>+  const nsStyleDisplay* parentDisp = aFrame->GetParent()->GetStyleDisplay();

You should null-check the result of aFrame->GetParent() before calling GetStyleDisplay() on it.

>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
>+nsIDOMCSSValue*
>+nsComputedDOMStyle::DoGetMozPerspective()
>+{
>+    nsROCSSPrimitiveValue* val = GetROCSSPrimitiveValue();
>+    val->SetNumber(GetStyleDisplay()->mChildPerspective);
>+    return val;

Given that 'none' is the initial value, I think you should probably report '0' as 'none'.

>+    COMPUTED_STYLE_MAP_ENTRY_LAYOUT(perspective,            MozPerspective),

Remove the "_LAYOUT".

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>--- a/layout/style/nsRuleNode.cpp
>+++ b/layout/style/nsRuleNode.cpp
>@@ -4479,16 +4479,22 @@ nsRuleNode::ComputeDisplayData(void* aSt
>                     parentDisplay->mTransformOrigin[0],
>                     parentDisplay->mTransformOrigin[1],
>                     SETCOORD_LPH | SETCOORD_INITIAL_HALF |
>                     SETCOORD_BOX_POSITION | SETCOORD_STORE_CALC,
>                     aContext, mPresContext, canStoreInRuleTree);
>     NS_ASSERTION(result, "Malformed -moz-transform-origin parse!");
>   }
> 
>+  SetFactor(*aRuleData->ValueForPerspective(),
>+            display->mChildPerspective, canStoreInRuleTree,
>+            parentDisplay->mChildPerspective, 0.0f,
>+            SETFCT_NONE);
>+
>+

No need for the double linebreak here.

>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
>+    if (!(mChildPerspective == aOther.mChildPerspective))

Just use != rather than ! ( == ).

>diff --git a/layout/style/nsStyleTransformMatrix.cpp b/layout/style/nsStyleTransformMatrix.cpp
>+  if (depth > 0.0f) {
>+    temp._34 = -1.0/depth;
>+  }

The spec says non-positive values should be rejected by the parser, so by this point you should be able to assert that depth > 0.0f -- though I'd hope you haven't done any double-to-float conversions since it was parsed!

That said -- you need to add code to the parser to reject nonpositive values, since you're missing that code.

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js
>+	"-moz-perspective": {
>+		domProp: "MozPerspective",
>+		inherited: false,
>+		type: CSS_TYPE_LONGHAND,
>+		initial_values: [ "0" ],

you should add "none" here

>+		other_values: [ "1000", "500.2" ],

and add some negative values here

>+		invalid_values: [ "pants" ]
>+	},


I'd also note that the spec says the perspective() function and the 'perspective' property take <length> rather than <number>.  Which does WebKit do?  Does the spec need to be fixed, or should you be changing this to accept lengths rather than numbers?  Number does seem to make more sense to me.  If the spec needs fixing, please email www-style.


r=dbaron with that fixed

Comment 141

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #140)

Thanks for the comments, fixed and running through tryserver now.

> 
> I'd also note that the spec says the perspective() function and the
> 'perspective' property take <length> rather than <number>.  Which does
> WebKit do?  Does the spec need to be fixed, or should you be changing this
> to accept lengths rather than numbers?  Number does seem to make more sense
> to me.  If the spec needs fixing, please email www-style.

Where does it say this?

The document I've been referring to says <number>.

http://www.w3.org/TR/css3-3d-transforms/#perspective

Comment 142

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://dev.w3.org/csswg/css3-3d-transforms/ is newer than that

Comment 143

[Matt Woodrow (:mattwoodrow)]

Oh right, guess I've been reading the wrong thing :)

Thanks for that, I'll switch to parsing lengths then!

WebKit appears to be parsing lengths too.

Comment 144

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 9 - Implement the perspective() transform function and style property. v3

Fixed review comments.

Requesting review again because of the number -> length changes.

Comment 145

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 9 - Implement the perspective() transform function and style property. v4

Fixed test failures.

Comment 146

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 548080 [details] [diff] [review]
Part 13: Add basic reftests for 3d transforms and expose 3d transform status in GfxInfo

Review of attachment 548080 [details] [diff] [review]:
-----------------------------------------------------------------

Instead of exposing whether 3D transforms are enabled, how about just making the tests conditional on GPU acceleration enabled (for now)?

::: layout/reftests/transform-3d/reftest.list
@@ +23,5 @@
> +== rotate3d-2a.html rotatey-1-ref.html
> +!= backface-visibility-1a.html about:blank
> +fails-if(!transforms3d) == backface-visibility-1b.html about:blank
> +fails-if(!transforms3d) != perspective-origin-1a.html rotatex-perspective-1a.html
> +== perspective-origin-1b.html perspective-origin-1a.html

Can you put these in alphabetical order?

Comment 147

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 548084 [details] [diff] [review]
Part 14b: Layout changes for preserve-3d

Review of attachment 548084 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +2441,4 @@
>  {
>    NS_PRECONDITION(aFrame, "Cannot get transform matrix for a null frame!");
> +  //NS_PRECONDITION(aFrame->GetStyleDisplay()->HasTransform(),
> +  //                "Cannot get transform matrix if frame isn't transformed!");

Just remove this

@@ +2446,5 @@
> +  if (aOutAncestor) {
> +      *aOutAncestor = aFrame->GetParent();
> +  }
> +
> +  if (!aFrame->GetStyleDisplay()->HasTransform()) {

Add comment that this is the preserve-3d case

@@ +2476,5 @@
> +                                                    aFrame->PresContext(),
> +                                                    dummy, bounds, aFactor);
> +  } else {
> +     NS_ASSERTION(aFrame->Preserves3DChildren(), 
> +                  "If we don't have a transform, then we must be at least preserving transforms of our children");

Shouldn't this be asserting aFrame->Preserves3D?

@@ +2776,5 @@
>                                           const nsRect* aBoundsOverride)
>  {
>    NS_PRECONDITION(aFrame, "Can't take the transform based on a null frame!");
> +  //NS_PRECONDITION(aFrame->GetStyleDisplay()->HasTransform(),
> +  //                "Cannot transform a rectangle if there's no transformation!");

Just remove this, and the ones below

::: layout/generic/nsFrame.cpp
@@ +1431,5 @@
> +  nsresult rv = NS_OK;
> +  nsDisplayList newList;
> +  while (nsDisplayItem *item = aList->RemoveBottom()) {
> +    if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM && item->GetUnderlyingFrame()->GetParent()->Preserves3DChildren()) {
> +      NS_ASSERTION(item->GetUnderlyingFrame()->Preserves3D(), "");

Need a message!

But can't this assertion fire if you have a preserves-3D parent with a child that has a transform but also a clip?

@@ +1481,5 @@
>     */
>    if ((mState & NS_FRAME_MAY_BE_TRANSFORMED) &&
>        disp->HasTransform()) {
>      /* If we have a complex transform, just grab the entire overflow rect instead. */
> +    if (Preserves3DChildren() || GetParent()->Preserves3DChildren() || !nsDisplayTransform::UntransformRect(dirtyRect, this, nsPoint(0, 0), &dirtyRect)) {

Just call Preserves3D() here?

@@ +1560,5 @@
>    resultList.AppendToTop(set.BlockBorderBackgrounds());
>    // 5: floats
>    resultList.AppendToTop(set.Floats());
>    // 7: general content
> +  set.Content()->SortByZPosition(aBuilder, GetContent());

Hmm, this is a little bit scary. Maybe make this if (Preserves3DChildren())? And maybe instead of calling SortByZPosition and having SortByZPosition account for 3D, use a separate SortByZPosition3D path which you use here?

@@ +4534,5 @@
>     * See bug #452496 for more details.
>     */
> +
> +  // Check the transformed flags and remove it
> +  PRBool isTransformed = (aFlags & INVALIDATE_ALREADY_TRANSFORMED);

"rectIsTransformed" to be clearly different from IsTransformed()?

@@ +4601,5 @@
>  
>    /* If we're transformed, the matrix will be relative to our
>     * cross-doc parent frame.
>     */
> +  //*aOutAncestor = nsLayoutUtils::GetCrossDocParentFrame(this);

Remove

@@ +6670,5 @@
> +      }
> +    } else {
> +      // When we are preserving 3d we need to iterate over all children separately.
> +      // If the child also preserves 3d then their overflow will already been in our
> +      // coordinate space, otherwise we need to transform.

Move this code to a helper function? FinishAndStoreOverflow is already a bit long :-)

::: layout/generic/nsIFrame.h
@@ +1192,5 @@
>    /**
> +   * Returns whether this frame will attempt to preserve the 3d transforms of its
> +   * children. This is a direct indicator of -moz-transform-style: preserve-3d.
> +   */
> +  virtual PRBool Preserves3DChildren() const;

Doesn't need to be virtual?

@@ +1195,5 @@
> +   */
> +  virtual PRBool Preserves3DChildren() const;
> +
> +  /**
> +   * Returns whether this child frame will preserve 3d transforms.

Needs a better comment to distinguish it from the previous method.

@@ +1197,5 @@
> +
> +  /**
> +   * Returns whether this child frame will preserve 3d transforms.
> +   */
> +  virtual PRBool Preserves3D() const;

Doesn't need to be virtual?

Comment 148

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 548077 [details] [diff] [review]
Part 10 - Implement -moz-backface-visible v2

r=dbaron

Comment 149

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 548078 [details] [diff] [review]
Part 11a: Add nsCSSValueTriplet and optionally read a z component to -moz-transform-origin v2

>+    if (CheckEndProperty() ||
>+        !ParseVariant(depth, VARIANT_LENGTH | VARIANT_CALC, nsnull) || 
>+        !nsLayoutUtils::Are3DTransformsEnabled()) {

I think you can drop the CheckEndProperty || here.

>+    NS_ABORT_IF_FALSE(xValue.GetUnit() != eCSSUnit_Null &&
>+                      yValue.GetUnit() != eCSSUnit_Null &&
>+                      xValue.GetUnit() != eCSSUnit_Inherit &&
>+                      yValue.GetUnit() != eCSSUnit_Inherit &&
>+                      zValue.GetUnit() != eCSSUnit_Inherit &&
>+                      xValue.GetUnit() != eCSSUnit_Initial &&
>+                      yValue.GetUnit() != eCSSUnit_Initial &&
>+                      zValue.GetUnit() != eCSSUnit_Initial,
>+                      "inappropriate pair value");

"pair" -> "triplet"

Also, please add a comment noting that a null Z value *is* allowed,
since it isn't obvious when skimming.

(both comments repeated for both SetTripletValue variants)

nsRuleNode.cpp:

You still have the SetTripletCoords function, but you're not calling it.
Remove it.  (It has bugs, too, but I won't bother describing them!)

It would probably be useful to use mozilla::DebugOnly<PRBool> in
your new code in ComputeDisplayData.

>+     if (valZ.GetUnit() == eCSSUnit_Null) {
>+       display->mTransformOrigin[2].SetCoordValue(0);

You should add a comment here that we've already separately checked
transformOriginValue against eCSSUnit_Null, so this is safe (i.e.,
clearly means 0 rather than unspecified).

r=dbaron with those things fixed

Comment 150

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 549699 [details] [diff] [review]
Part 9 - Implement the perspective() transform function and style property. v4

Carrying forward r=dbaron

Comment 151

[Matt Woodrow (:mattwoodrow)]

Landed parts 6 through 12b on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/ba5eb1cd42f3
http://hg.mozilla.org/integration/mozilla-inbound/rev/e7dc1c09ae24
http://hg.mozilla.org/integration/mozilla-inbound/rev/a637d482b540
http://hg.mozilla.org/integration/mozilla-inbound/rev/74eac3a09043
http://hg.mozilla.org/integration/mozilla-inbound/rev/08b756f93436
http://hg.mozilla.org/integration/mozilla-inbound/rev/96941ed69aeb
http://hg.mozilla.org/integration/mozilla-inbound/rev/85cc7836c552
http://hg.mozilla.org/integration/mozilla-inbound/rev/ea882f18d8ac
http://hg.mozilla.org/integration/mozilla-inbound/rev/ad334152010a
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d3b6382054d
http://hg.mozilla.org/integration/mozilla-inbound/rev/166f4f247772

Comment 152

[Brendan Eich [:brendan]]

Excitement!

/be

Comment 153

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ba5eb1cd42f3
http://hg.mozilla.org/mozilla-central/rev/e7dc1c09ae24
http://hg.mozilla.org/mozilla-central/rev/a637d482b540
http://hg.mozilla.org/mozilla-central/rev/74eac3a09043
http://hg.mozilla.org/mozilla-central/rev/08b756f93436
http://hg.mozilla.org/mozilla-central/rev/96941ed69aeb
http://hg.mozilla.org/mozilla-central/rev/85cc7836c552
http://hg.mozilla.org/mozilla-central/rev/ea882f18d8ac
http://hg.mozilla.org/mozilla-central/rev/ad334152010a
http://hg.mozilla.org/mozilla-central/rev/2d3b6382054d
http://hg.mozilla.org/mozilla-central/rev/166f4f247772

Comment 154

[Dean Jackson]

Cool!

Comment 155

[Matt Woodrow (:mattwoodrow)]

> 
> @@ +1560,5 @@
> >    resultList.AppendToTop(set.BlockBorderBackgrounds());
> >    // 5: floats
> >    resultList.AppendToTop(set.Floats());
> >    // 7: general content
> > +  set.Content()->SortByZPosition(aBuilder, GetContent());
> 
> Hmm, this is a little bit scary. Maybe make this if (Preserves3DChildren())?
> And maybe instead of calling SortByZPosition and having SortByZPosition
> account for 3D, use a separate SortByZPosition3D path which you use here?
> 

Can you explain this a bit more please.

I've actually removed this line in my queue because it was causing tests to fail. What exactly is in the Content() group? I was under the impression that everything would have a z-order of 0 (positive/negative z-order content is separate), and no z-position (exisiting tests won't have 3d transforms), so this would just be a content-order sort and a no-op.

Comment 156

[Matt Woodrow (:mattwoodrow)]

Relatedly, all the 3d content that I've tested ends up in the positive z-order group, so this line doesn't actually appear to be necessary at all.

Comment 157

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #155)
> > 
> > @@ +1560,5 @@
> > >    resultList.AppendToTop(set.BlockBorderBackgrounds());
> > >    // 5: floats
> > >    resultList.AppendToTop(set.Floats());
> > >    // 7: general content
> > > +  set.Content()->SortByZPosition(aBuilder, GetContent());
> > 
> > Hmm, this is a little bit scary. Maybe make this if (Preserves3DChildren())?
> > And maybe instead of calling SortByZPosition and having SortByZPosition
> > account for 3D, use a separate SortByZPosition3D path which you use here?
> > 
> 
> Can you explain this a bit more please.

The scariness is that you're re-sorting everything by CSS z-index and content order, and if any code has deliberately violated that order then you'll be undoing that. I don't remember off the top of my head what does that, but it sounds like that's what's causing your tests failing. There's also an efficiency issue, you're adding an O(N log N) pass over the content list.

> I've actually removed this line in my queue because it was causing tests to
> fail. What exactly is in the Content() group?

Content() contains everything "normal" that doesn't belong in one of the other lists. See nsDisplayList.h.

> Relatedly, all the 3d content that I've tested ends up in the positive
> z-order group, so this line doesn't actually appear to be necessary at all.

Right, nsDisplayTransform items should always be in the PositionedDescendants list since they are pseudo-stacking-contexts.

Comment 158

[Louis-Rémi BABE]

Is it planned to have a media query available to test for 3d transform support?

Regards,
Lr

Comment 159

[Paul Rouget [:paul]]

(In reply to Louis-Rémi BABE from comment #158)
> Is it planned to have a media query available to test for 3d transform
> support?

This feature can't be detected afaik. So a mediaQuery sounds appropriate. Webkit does that, right?

Comment 160

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Paul Rouget [:paul] from comment #159)
> (In reply to Louis-Rémi BABE from comment #158)
> > Is it planned to have a media query available to test for 3d transform
> > support?
> 
> This feature can't be detected afaik. So a mediaQuery sounds appropriate.
> Webkit does that, right?

There's also dbaron's @supports[1], but I guess that wouldn't be ready in time.

[1] http://dev.w3.org/csswg/css3-conditional/#at-supports

Comment 161

[flying sheep]

hi guys. i really love how much progress and effort goes into supporting new standards and bringing the web forward. i’m following this bug report for quite some time now.

but every time i see how some parts are advancing and getting care from so many, it feels funny how other small deetails are neglected despite having hideous bugs for years.

specifically, i’m talking about some elements not being stylable, partly or at all (bug 418833, bug 52500, bug 402625 and bug 79107), buttons still have this invisible-yet-padded “::-moz-focus-inner” (bug 140562), and cursor themes are not completely supported (bug 609889) (firefox uses it’s own 90ies-cursors sometimes)

i hope this gets one or two of you to look at these, and my whinig didn’t distract you too much, but i think new standards are not everything, and little old inconsitencies matter, too. i just tried to direct some of the positive energy here elwhere, where it is needed imho, too.

Comment 162

[Dean Jackson]

You could technically feature detect by setting a 3d transform on something and examining the computed style, but that's annoying and why we implemented the media query in webkit.

Comment 163

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Currently the plan is to support 3d-transforms everywhere (see bug 675837). The performance won't be great without GPU acceleration, so maybe we need a query for whether transforms are GPU-accelerated.

Comment 164

[Dean Jackson]

At least the Apple port of WebKit for Safari only does 3d in accelerated content. We don't have a software fallback. I'm not sure if other ports do. In other words, for Safari our media query for 3d was basically enough to guarantee performance.

We were quite hesitant to introduce a media query that is used as feature detection. As roc says, this is really testing performance, not the media. Oh well :(

Comment 165

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 14b: Layout changes for preserve-3d v2

Fixed review comments

Comment 166

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 551300 [details] [diff] [review]
Part 14b: Layout changes for preserve-3d v2

Review of attachment 551300 [details] [diff] [review]:
-----------------------------------------------------------------

Somewhere you need a comment or two to explain the general setup: how preserve-3d transforms are applied to the display list (in BuildDisplayListForStackingContext, I guess), and how invalidation is handled (in Invalidate somewhere).

::: layout/base/nsDisplayList.cpp
@@ +772,5 @@
> +  nsIFrame* ancestor;
> +  gfx3DMatrix matrix1 = aItem1->GetUnderlyingFrame()->GetTransformMatrix(&ancestor);
> +  gfx3DMatrix matrix2 = aItem2->GetUnderlyingFrame()->GetTransformMatrix(&ancestor);
> +
> +  return matrix1._43 < matrix2._43;

If these are equal, we should check IsContentLEQ, right?

::: layout/generic/nsFrame.cpp
@@ +1444,5 @@
> +{
> +  nsresult rv = NS_OK;
> +  nsDisplayList newList;
> +  while (nsDisplayItem *item = aList->RemoveBottom()) {
> +    if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM && item->GetUnderlyingFrame()->GetParent()->Preserves3DChildren()) {

How about check item->GetUnderlyingFrame() not null, then check item->GetUnderlyingFrame()->GetParent()->Preserves3DChildren(), then switch on item->GetType()?

@@ +1445,5 @@
> +  nsresult rv = NS_OK;
> +  nsDisplayList newList;
> +  while (nsDisplayItem *item = aList->RemoveBottom()) {
> +    if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM && item->GetUnderlyingFrame()->GetParent()->Preserves3DChildren()) {
> +      NS_ASSERTION(item->GetUnderlyingFrame()->Preserves3D(), "");

Need a message.

Comment 167

[Boris Zbarsky [:bzbarsky]]

It looks like this has landed in a partially-enabled state, which causes site compat issues.  See bug 677173.

Comment 168

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm right. We can't really turn off IDL attributes based on a pref, of course :-(. If we can back out just the CSSDeclaration part of the patch, we should do that.

Comment 169

[Boris Zbarsky [:bzbarsky]]

> We can't really turn off IDL attributes based on a pref, of course

You sort of can, if it lives on a separate interface; we have some code in nsDOMClassInfo.cpp right now that does that sort of thing.

Comment 170

[Christopher Blizzard (:blizzard)]

Should probably track for Firefox 8 given the site compat issues.

Comment 171

[Matt Woodrow (:mattwoodrow)]

Is it worth creating a new interface just for the 3d transform properties, with the plan of having them unconditionally defined in the near future?

It might be easier to just remove the properties, since this should only be needed temporarily.

Comment 172

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #171)
> It might be easier to just remove the properties, since this should only be
> needed temporarily.

Let's do that.

Comment 173

[Matt Woodrow (:mattwoodrow)]

How would I do this exactly?

Just removing the .idl entries fails to compile because the macros in nsCSSPropList.h reference them.

Comment 174

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 14b: Layout changes for preserve-3d v3

Comment 175

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Where do you get errors from removing the IDL entries?  It's probably fixable, with hacks.

In addition to that, though, we'd need to make a bunch of other APIs act as though the property is not implemented.  I think hacking nsCSSProps::LookupProperty to return eCSSProperty_UNKNOWN for them would produce exactly the right results, but this would need an audit of all the callers.

Comment 176

[Matt Woodrow (:mattwoodrow)]

In file included from /Users/mattwoodrow/mozilla-incoming/layout/style/nsDOMCSSDeclaration.cpp:365:
/Users/mattwoodrow/mozilla-incoming/layout/style/nsCSSPropList.h:2261: error: no ‘nsresult nsDOMCSSDeclaration::GetMozPerspectiveOrigin(nsAString_internal&)’ member function declared in class ‘nsDOMCSSDeclaration’

Comment 177

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] from comment #175)
> In addition to that, though, we'd need to make a bunch of other APIs act as
> though the property is not implemented.  I think hacking
> nsCSSProps::LookupProperty to return eCSSProperty_UNKNOWN for them would
> produce exactly the right results, but this would need an audit of all the
> callers.

Yes, I just did that code audit and I believe it does the right thing.  But the change needs to apply to both versions of LookupProperty.

(In reply to Matt Woodrow (:mattwoodrow) from comment #176)
> In file included from
> /Users/mattwoodrow/mozilla-incoming/layout/style/nsDOMCSSDeclaration.cpp:365:
> /Users/mattwoodrow/mozilla-incoming/layout/style/nsCSSPropList.h:2261:
> error: no ‘nsresult
> nsDOMCSSDeclaration::GetMozPerspectiveOrigin(nsAString_internal&)’ member
> function declared in class ‘nsDOMCSSDeclaration’

Just add the declarations to nsDOMCSSDeclaration.h manually, right below NS_DECL_NSIDOMCSS2PROPERTIES.

Comment 178

[Timothy B. Terriberry (:derf)]

Comment on attachment 548634 [details] [diff] [review]
Part 15 - Add 4D Vectors, Quaternions and gfx3DMatrix functions v2

Review of attachment 548634 [details] [diff] [review]:
-----------------------------------------------------------------

So, the operator[] issues, the order of translation in Inverse(), the left-over debugging code, and the MAX macro in a header must be fixed. The rest are suggestions you can take or leave as you like.

::: gfx/2d/BasePoint3D.h
@@ +68,5 @@
> +          return y;
> +      } else {
> +          return z;
> +      }
> +  }

This function made me throw up in my mouth a little. Just saying.

::: gfx/2d/BasePoint4D.h
@@ +72,5 @@
> +  Sub operator+(const Sub& aPoint) const {
> +    return Sub(x + aPoint.x, y + aPoint.y, z + aPoint.z, w + aPoint.w);
> +  }
> +  Sub operator-(const Sub& aPoint) const {
> +    return Sub(x - aPoint.x, y - aPoint.y, z - aPoint.z, w + aPoint.w);

Should be w - aPoint.w, not +.

::: gfx/2d/Makefile.in
@@ +51,5 @@
>  EXPORTS_mozilla/gfx	= \
>          2D.h \
>          BasePoint.h \
>  	BasePoint3D.h \
> +	BasePoint4D.h \

You're using tabs here when the rest of the list (except the $(NULL) line) isn't.

::: gfx/thebes/gfx3DMatrix.cpp
@@ +114,5 @@
>    return *this;
>  }
>  
> +gfxPointH3D&
> +gfx3DMatrix::operator[](const int aIndex)

Is there any reason to declare aIndex const?

@@ +118,5 @@
> +gfx3DMatrix::operator[](const int aIndex)
> +{
> +    NS_ABORT_IF_FALSE(aIndex >= 0 && aIndex <= 3, "Invalid matrix array index");
> +    if (aIndex == 0) {
> +        return *reinterpret_cast<gfxPointH3D*>(&_11);

So, this kind of thing is technically invalid, though I don't know a compiler/architecture combination where it will actually break.

The only way to guarantee, at a language level, that a series of memory cells are adjacent in memory, with no padding for alignment or whatever else between them, is to put them in an array.

However, if you're going to rely on _11 through _14 being adjacent in memory, why not rely on _11 through _44 being adjacent? Then you can get rid of the if/else chain and just use

  return *reinterpret_cast<gfxPointH3D*>((&_11)+4*aIndex);

directly, and the whole thing can go in the header so it can be inlined, as it becomes basically one lea instruction on x86. A similar approach will eliminate the if/else chain in BasePoint[34]D and even in TransposedVector()/SetTransposedVector(). That at least has the advantage of generating reasonable code, while the current construction will create surprisingly slow code for some future caller that naively expects operator[] on a matrix class to be implemented efficiently.

In other words, if you're going to make the assumption these things are tightly packed, then you should actually use that assumption, rather than doing things halfway. Otherwise you're relying on loop unrolling to get reasonable code out of things like Normalize() and Transposed() below (looking at the asm, not only does gcc not unroll the loops, it doesn't inline the functions that are available to be inlined, either, likely because all the conditionals make them huge).

@@ +129,5 @@
> +    }
> +}
> +
> +const gfxPointH3D&
> +gfx3DMatrix::operator[](const int aIndex) const

Is there any reason to declare aIndex const?

@@ +249,5 @@
> +gfx3DMatrix
> +gfx3DMatrix::Inverse3x3() const
> +{
> +    gfxFloat det = Determinant3x3();
> +    if (det == 0.0) {

So, gfxMatrix::Invert uses cairo_matrix_invert(), which handles singular matrices slightly differently.

1) It also fails if !ISFINITE(det).
2) Because of the way it is called, the inverse of a singular matrix is (usually) the original singular matrix, rather than the identity. I say "usually" because in the simple scaling/translation case, it inverts the X row before checking to see if the Y row is invertible, but I'd call that behavior a bug in Cairo (the documentation only says the matrix is modified if the inversion process is successful, and in general it's bad form to partially modify things and then return an error). It also doesn't check ISFINITE in that case, which is probably another bug.

I don't know how important preserving either of these semantics are. Given the aforementioned Cairo bugs, probably not very.

@@ +264,5 @@
> +    temp._23 = _13 * _21 - _11 * _23;
> +    temp._31 = _21 * _32 - _22 * _31;
> +    temp._32 = _31 * _12 - _11 * _32;
> +    temp._33 = _11 * _22 - _12 * _21;
> +    temp /= det;

I recommend temp *= (1/det).

@@ +293,5 @@
> +     */
> +    gfx3DMatrix translation;
> +    translation[3] = gfxPointH3D(-_41, -_42, -_43, 1);
> +
> +    matrix3 = Inverse3x3() * translation;

Is this really correct? I get Inverse[{{a,b,c,0},{d,e,f,0},{g,e,h,0},{x,y,z,1}}] == {{1,0,0,0},{0,1,0,0},{0,0,1,0},{-x,-y,-z,1}}.Inverse[{{a,b,c,0},{d,e,f,0},{g,e,h,0},{0,0,0,1}}]. I.e.,

  matrix3 = translation * Inverse3x3();

Also, the vast majority of these multiplications are by 0 or 1, but because things get stored to memory and then read back, the optimizer is unlikely to eliminate them all. Perhaps it's worth expanding them out, e.g.:

  matrix3 = Inverse3x3();
  matrix3[3] = gfxPointH3D(-_41*matrix3._11 - _42*matrix3._22 - _43*matrix3._33,
   ...

That reduces the number of multiplications from 64 to 9. If you opt not to do that, you should still at least use the gfx3DMatrix::Translation() factory, rather than the translation[3] = ... construction you use above.

@@ +294,5 @@
> +    gfx3DMatrix translation;
> +    translation[3] = gfxPointH3D(-_41, -_42, -_43, 1);
> +
> +    matrix3 = Inverse3x3() * translation;
> +    test = PR_TRUE;

So, this looks like left-over debugging code. In particular, you aren't actually returning the inverse you just computed, but are continuing on to compute the full 4x4 inverse.

@@ +301,3 @@
>    gfxFloat det = Determinant();
>    if (det == 0.0) {
> +    return gfx3DMatrix();

Ah, I see you're actually explicitly changing the behavior away from the Cairo behavior (i.e., returning the identity instead of the original, singular matrix). Is there a reason for this? Perhaps it should be documented?

@@ +356,4 @@
>  
> +  temp /= det;
> +
> +  //NS_ABORT_IF_FALSE(!test || matrix3 == temp, "AAAAH");

Either #ifdef this to actually work in debug mode, or remove it entirely. Don't just leave it here commented out.

@@ +407,5 @@
> +gfx3DMatrix::Normalize()
> +{
> +    for (int i = 0; i < 4; i++) {
> +        for (int j = 0; j < 4; j++) {
> +            this->operator [](i)[j] /= this->operator [](3)[3];

Perhaps (*this)[i][j] would be cleaner?

@@ +465,5 @@
> +    return gfxPointH3D(x, y, z, w);
> +}
> +
> +gfxPointH3D
> +gfx3DMatrix::Transform4DLeft(const gfxPointH3D& aPoint) const

"Left" messes with my head. It's correct for OpenGL, where points are column vectors multiplied on the right and the matrix is stored in column-major order, but it's backwards for D3D, where points are row vectors multiplied on the left to begin with (and in particular, this is how multiplication is documented to work for the 2D gfxMatrix class).

Perhaps TransposeTransform4D?

::: gfx/thebes/gfxQuaternion.h
@@ +40,5 @@
> +
> +#include "mozilla/gfx/BasePoint4D.h"
> +#include "gfx3DMatrix.h"
> +
> +#define MAX(a,b) ((a)>(b)?(a):(b))

It's probably not a great idea to put a macro named MAX in a header file like this. I also recommend documenting that it has to return b when a==b, and why. Otherwise someone is likely to replace it with some other MAX implementation and break things without realizing it.

@@ +67,5 @@
> +        gfxFloat dot = DotProduct(aOther);
> +        if (dot == 1.0) {
> +            return *this;
> +        }
> +        gfxFloat theta = acos(dot);

Rounding errors in the quaternion normalization or dot product could potentially give you a value slightly outside the range [-1,1], which will make acos() return NaN. I recommend clamping dot to that range.

@@ +70,5 @@
> +        }
> +        gfxFloat theta = acos(dot);
> +
> +        gfxQuaternion left = *this;
> +        left *= sin((1 - aCoeff) * theta) / sin(theta);

Instead of dividing by sin(theta), it may be better to use

  gfxFloat rsintheta = 1/sqrt(1 - dot*dot);

and then multiply by rsintheta. Square roots, and particularly reciprocal square roots, are much easier to compute.

@@ +73,5 @@
> +        gfxQuaternion left = *this;
> +        left *= sin((1 - aCoeff) * theta) / sin(theta);
> +
> +        gfxQuaternion right = aOther;
> +        right *= sin(aCoeff * theta) / sin(theta);

You can also replace the remaining two calls to sin() with a call to sin() and cos() on the same angle (which is faster on many platforms). E.g., using the identity
  sin((1 - aCoeff) * theta)/sin(theta)
  == sin(theta - aCoeff*theta)/sin(theta)
  == (sin(theta)*cos(aCoeff*theta) - cos(theta)*sin(aCoeff*theta))/sin(theta)
  == sin(theta)*cos(aCoeff*theta)/sin(theta) - cos(theta)*sin(aCoeff*theta)/sin(theta)
  == cos(aCoeff*theta) - dot*sin(aCoeff*theta)/sin(theta)
 leads to the code:

gfxFloat w = sin(aCoeff*theta)*rsintheta;

left *= cos(aCoeff*theta) - dot*w;
right *= w;

@@ +89,5 @@
> +        temp[1][1] = 1 - (2 * x * x) - (2 * z * z);
> +        temp[1][2] = (2 * y * z) + (2 * w * x);
> +        temp[2][0] = (2 * x * z) + (2 * w * y);
> +        temp[2][1] = (2 * y * z) - (2 * w * x);
> +        temp[2][2] = 1 - (2 * x * x) - (2 * y * y);

Recommend factoring out the 2's in each term, to save 9 multiplies, e.g.,

  temp[0][0] = 1 - 2*(y * y - z * z);

The compiler is generally conservative about doing such optimizations for you unless you compile with -ffast-math or an equivalent.

Comment 179

[Timothy B. Terriberry (:derf)]

Comment on attachment 548635 [details] [diff] [review]
Part 16 - Implement transitions/animations for 3d transforms.

Review of attachment 548635 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with some suggestions, but I did not review the style stuff carefully, as I'm relying on the fact that dbaron knows that code much better than I do.

::: gfx/2d/BasePoint4D.h
@@ +72,5 @@
>    Sub operator+(const Sub& aPoint) const {
>      return Sub(x + aPoint.x, y + aPoint.y, z + aPoint.z, w + aPoint.w);
>    }
>    Sub operator-(const Sub& aPoint) const {
> +    return Sub(x - aPoint.x, y - aPoint.y, z - aPoint.z, w - aPoint.w);

You should just fix this in Part 15, not here.

::: layout/style/nsStyleAnimation.cpp
@@ +1115,5 @@
>  
>    float scaleY = sqrt(C * C + D * D);
>    C /= scaleY;
>    D /= scaleY;
> +

Whitespace-only change.

@@ +1120,4 @@
>    XYshear /= scaleY;
>  
> +  // A*D - B*C should now be 1 or -1
> +  NS_ASSERTION(0.99 < PR_ABS(A*D - B*C) && PR_ABS(A*D - B*C) < 1.01,

Why the change from NS_ABS to PR_ABS? There's plenty of other uses of NS_ABS that you didn't change.

@@ +1147,5 @@
> +static PRBool
> +Decompose3DMatrix(const gfx3DMatrix &aMatrix, gfxPoint3D &aScale,
> +                  float aShear[3], gfx3DMatrix &aRotate,
> +                  gfxPoint3D &aTranslate, gfxPointH3D &aPerspective)
> +{

This is a pretty straightforward translation of the code from http://dev.w3.org/csswg/css3-2d-transforms/#unmatrix to our C++ API, except you dropped all the comments from the original code. It would be nice if you could preserve those. Referencing that link would be a good idea, too.

@@ +1194,5 @@
> +        row[i].z = local[i].z;
> +    }
> +
> +    aScale.x = row[0].Length();
> +    row[0].Normalize();

You just computed Length(), but Normalize() will compute it again internally. Recommend just dividing by the length you already computed. Ditto for the other two occurrences below.

@@ +1197,5 @@
> +    aScale.x = row[0].Length();
> +    row[0].Normalize();
> +    
> +    aShear[XYSHEAR] = row[0].DotProduct(row[1]);
> +    row[1] += row[0] * -aShear[XYSHEAR];

row[1] -= row[0] * aShear[XYSHEAR] perhaps? Ditto for the other two lines below.

@@ +1224,5 @@
> +
> +    for (int i =0; i < 3; i++) {
> +        aRotate[i] = gfxPointH3D(row[i].x, row[i].y, row[i].z, 0);
> +    }
> +    aRotate[3] = gfxPointH3D(0, 0, 0, 1);

Is it really worth breaking out local into row[]? Operating on the rows of local directly would add 15 FMA's (assuming I can count), but the copies add 18 loads/stores, not counting loop overhead, and a bunch of code.

@@ +1310,4 @@
>  
> +  gfxPointH3D perspective = 
> +    InterpolateNumerically(perspective1, perspective2, aProgress);
> +  if (perspective != gfxPointH3D(0, 0, 0, 1)) {

What's the point of this check? Can't we just write perspective into result unconditionally? It would seem cheaper to always do that than to even do this test. I'm dubious about the utility of some of the other checks below, as well.

@@ +1317,5 @@
> +  gfxPoint3D translate = 
> +    InterpolateNumerically(translate1, translate2, aProgress);
> +  if (translate != gfxPoint3D(0, 0, 0)) {
> +      gfx3DMatrix temp = gfx3DMatrix::Translation(translate);
> +      result = temp * result;

As in the last patch, you can reduce this from 64 multiplies to 9. Perhaps you should just add a function for it to gfx3DMatrix?

@@ +1322,5 @@
> +  }
> +
> +  gfxQuaternion q1(rotate1);
> +  gfxQuaternion q2(rotate2);
> +  gfxQuaternion q3 = q1.Slerp(q2, aProgress);

Perhaps it would be better to have Decompose3DMatrix return a gfxQuaternion directly. Especially combined with the suggestion to just keep things in "local", it would avoid copying everything from local to aRotate at the end.

@@ +1335,5 @@
> +    InterpolateNumerically(shear1[YZSHEAR], shear2[YZSHEAR], aProgress);
> +  if (yzshear != 0.0) {
> +    gfx3DMatrix temp;
> +    temp._32 = yzshear;
> +    result = temp * result;

This can also be reduced to result[2] += yzshear*result[1], going from 64 multiplies to 4 and 3 lines of code to 1.

@@ +1358,5 @@
> +  gfxPoint3D scale = 
> +    InterpolateNumerically(scale1, scale2, aProgress);
> +  if (scale != gfxPoint3D(1.0, 1.0, 1.0)) {
> +    gfx3DMatrix temp = gfx3DMatrix::Scale(scale.x, scale.y, scale.z);
> +    result = temp * result;

This can also be reduced from 64 multiplies to 12, though it requires 3 lines of code instead of 2.

@@ +1576,4 @@
>  
>          // FIXME: If the matrix contains only numbers then we could decompose
>          // here. We can't do this for matrix3d though, so it's probably
>          // best to stay consistent.

I'm not sure that the second sentence here is true any longer. Certainly doing the decomposition once instead of for every step of the animation would save more computation than basically all of the other optimizations I pointed out combined.

Comment 180

[Benoit Jacob [:bjacob] (mostly away)]

I concur with Timothy, a Vector or Matrix class should store all its coefficients as a single one-dimensional array. Then, if you want to address vector coefficients as "x" and "y", implement that as inline methods. It is then completely safe to assume that they are inlined (provided optimization is enabled) and incur zero overhead.

Comment 181

[Matt Woodrow (:mattwoodrow)]

> I'm not sure that the second sentence here is true any longer. Certainly
> doing the decomposition once instead of for every step of the animation
> would save more computation than basically all of the other optimizations I
> pointed out combined.

I believe this would only prevent us from decomposing multiple times for each stage of the animation. Ie for visible region calculation, and for drawing.

I think we'd need a separate optimization to let us decompose the list once, and then start using those lists for future steps of the animation.

Comment 182

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just use NS_MAX instead of MAX.

Comment 183

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Timothy B. Terriberry (:derf) from comment #179)
> Comment on attachment 548635 [details] [diff] [review]
> Part 16 - Implement transitions/animations for 3d transforms.
> @@ +1120,4 @@
> >    XYshear /= scaleY;
> >  
> > +  // A*D - B*C should now be 1 or -1
> > +  NS_ASSERTION(0.99 < PR_ABS(A*D - B*C) && PR_ABS(A*D - B*C) < 1.01,
> 
> Why the change from NS_ABS to PR_ABS? There's plenty of other uses of NS_ABS
> that you didn't change.

FwIW, people have spent quite some time eradicating PR_ABS. Please be so kind not to revert their work.

Comment 184

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 15 - Add 4D Vectors, Quaternions and gfx3DMatrix functions v3

Fixed most of (if not all) review comments

Comment 185

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 16 - Implement transitions/animations for 3d transforms. v2

Fixed Tim's review comments

Carrying forward r=derf

Comment 186

[Timothy B. Terriberry (:derf)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #182)
> Just use NS_MAX instead of MAX.

There's potential for a subtle bug here. The first argument to MAX can be -0 instead of 0, and the second one is always 0. They'll compare equal with the standard relational operators, so it depends on the exact implementation which one gets returned. If you pass -0 instead of 0 to sqrt(), you'll get NaN back. Although the current implementation of NS_MAX looks like it does what we want, and it's unlikely to change, I don't see this as a documented part of its interface, and think it's safer to have a custom version where the desired behavior _is_ documented.

Comment 187

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You could document it for NS_MAX, and write a test?

Comment 188

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, let's just document it for NS_MAX. Very good catch though, I wouldn't have thought of it.

Comment 189

[Brandon Sterne (:bsterne)]

Jesse is going to add this to his DOM fuzzers.

Comment 190

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 548083 [details] [diff] [review]
Part 14a: Add -moz-transform-style CSS property

We need to do the same things done in bug677173 to hide these properties
for now.  (I'd like to come up with a better solution so that the pref
can fully enable them, though; the main obstacle there is the
nsIDOMCSS2Properties interface.  If we had no interface at all, things
would be simpler...)

In property_database.js, you should:

 - use tabs to match the rest of the file
 - use invalid_values rather than invalid_value

r=dbaron with that

Comment 191

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 548636 [details] [diff] [review]
Part 17 - Add style tests for the new transform functions, and transitions

In property_database.js, you need tabs rather than spaces.

Why do the perspective tests all take <number> arguments when the
spec says perspective() takes lengths?  I guess that's a change
in the editor's draft since the working draft.  I suppose numbers
do make sense -- except I'd expect perspective() to be consistent
with translatez().  I think we should change one of them.

test_transitions_per_property.html:

  Please leave the existing tests using rotate() rather than converting
  them all to rotatez(), but it's not a bad idea to test them with
  rotatez in *addition*.  (Also, if the pref is working right, those
  should require the pref... which makes me think that either something
  is wrong or you didn't test this with the pref off.)

  Some of the horrible expected: fields with matrix3d() values would
  have been easier to write with c()... but now that you've written them
  they're probably worth leaving.

I'm guessing you didn't test with the pref off, or maybe that there's
another patch in the series to turn the pref on.  You probably need a
pref check in property_database.js to make the addition of some of the
values to those arrays condition on the pref.  Should be easy enough
with the structure
  [ values, for, always ].concat(check_pref() ? [values, for, 3d] : [])

r=dbaron with those things fixed

Comment 192

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 553087 [details] [diff] [review]
Part 16 - Implement transitions/animations for 3d transforms. v2

comments on the nsComputedDOMStyle.cpp part only (haven't gotten through nsStyleAnimation.cpp yet):

  So I'm not sure why these changes are in this patch.  But that's ok
  for now, though I think they should probably be separate.

  More importantly, though, this needs to output a result that we can
  parse.  matrix3d() takes 16 <number>s, as it should, so this is
  producing matrix3d() expressions that aren't parseable.

  Given appropriate tests in property_database.js (and their being
  enabled), this should cause test failures.  If it doesn't, there's a
  problem.


  I think the best way to fix this is:
   + change matrix() to accept <number> or <length> for the
   transformation parts, so that matrix() and matrix3d() both
   accept the syntax WebKit accepts, but that ours also accepts px for
   matrix() as we have in the past

   + change nsComputedDOMStyle::DoGetMozTransform to never output
   "px"

  review- on this part of the patch because I'd like to look at that
  again once you've done it

Comment 193

[Asa Dotzler [:asa]]

release drivers are not going to track this for 8 because we believe it is turned off for the 8 release (on aurora today.) If that is not correct, please re-nominate.

Comment 194

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 16 - Implement transitions/animations for 3d transforms. v3

That bug did indeed break tests.

I've gone for the simple solution of just printing 3d matrices without 'px'. This <length>/<number> problem exists for other functions too, so I'd like to handle it all in one single patch.

Comment 195

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 17 - Add style tests for the new transform functions, and transitions v2

Updated so these actually pass again with the current patch queue.

Indeed, I've only tested these with 3d transforms enabled. Not setting review flag for now until I implement handling these without transforms enabled.

Comment 196

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 17 - Add style tests for the new transform functions, and transitions v3

Updated test_transitions_per_property.html

Comment 197

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 18 - Make the perspective() transform function actually fail on numbers <= 0

Comment 198

[Timothy B. Terriberry (:derf)]

Comment on attachment 553086 [details] [diff] [review]
Part 15 - Add 4D Vectors, Quaternions and gfx3DMatrix functions v3

Review of attachment 553086 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from me, but you should probably address roc's comment 188.

::: gfx/2d/BasePoint3D.h
@@ +61,5 @@
>    // compiler generated default assignment operator
>  
> +  T& operator[](int aIndex) {
> +    NS_ABORT_IF_FALSE(aIndex >= 0 && aIndex <= 2, "Invalid array index");
> +    return *reinterpret_cast<T*>((&x)+aIndex);

Is the reinterpret_cast as written here necessary? &x should already be of type T*. One subtlety here is that automated tools like coverity may detect references past the end of the object x as being out-of-bounds. It might be possible that a reinterpret_cast to an array of size 3 instead would avoid triggering bogus reports in coverity, but I have no way to test this.

The same comment applies to all the other places you do this.

::: gfx/thebes/gfx3DMatrix.cpp
@@ +260,5 @@
> +    }
> +
> +    gfx3DMatrix temp;
> +
> +    temp._11 = (_22 * _33 - _23 * _32) / det;

Making the "/ det" local is good (the optimizer doesn't have to deal with things going out to memory and back and trying to figure out if intermediate things have side-effects). But my suggestion was to compute the reciprocal "1 / det" and then multiply by that.

It's just a suggestion, and if you want to ignore me, feel free. It can be a ULP or two less accurate than doing all the divisions, but it should be significantly faster. Not only do divisions take many cycles, but on x86 they are also not pipelined, so all your other floating-point operations will stall, waiting for them.

Comment 199

[Timothy B. Terriberry (:derf)]

Comment on attachment 555944 [details] [diff] [review]
Part 16 - Implement transitions/animations for 3d transforms. v3

Review of attachment 555944 [details] [diff] [review]:
-----------------------------------------------------------------

I know I already r+'d this, but I had a few more quick comments anyway.

::: layout/style/nsStyleAnimation.cpp
@@ +1163,5 @@
> +    /* Normalize the matrix */
> +    local.Normalize();
> +
> +    /** 
> +     * perspective is used to solve for perspective, but it also provides

This sounds pretty funny. Perhaps "This" in place of the first "perspective".

@@ +1205,5 @@
> +    /* Compute X scale factor and normalize first row. */
> +    aScale.x = local[0].Length();
> +    local[0] /= aScale.x;
> +    
> +    /* Compute XY shear factor and make 2nd local orthogonal to 1st. */

This looks like an erroneous search and replace? I think "2nd row" still makes more sense here than "2nd local" (there's only one variable named "local"). Same for all the places below.

@@ +1228,5 @@
> +    aShear[XZSHEAR] /= aScale.z;
> +    aShear[YZSHEAR] /= aScale.z;
> +
> +    /**
> +     * At this point, the matrix (in locals) is orthonormal.

Unless you actually want to rename the variable to "locals"... but I think that's pretty silly.

Comment 200

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 15 - Add 4D Vectors, Quaternions and gfx3DMatrix functions v4

Fixed review comments, carrying forward r=derf

Comment 201

[Matt Woodrow (:mattwoodrow)]

Landed parts 14a, 14b and 15 on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/14eb41309965
http://hg.mozilla.org/integration/mozilla-inbound/rev/364548e43748
http://hg.mozilla.org/integration/mozilla-inbound/rev/be7cefceceac

Comment 202

[Dean Jackson]

BTW - if you're up to implementing matrix decomposition, be aware that there was a bug in the spec which was fixed recently.

Use the editor's copy at http://dev.w3.org/csswg/css3-2d-transforms/
(decomposition is in the 2d transforms spec, not 3d)

Comment 203

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/14eb41309965
http://hg.mozilla.org/mozilla-central/rev/364548e43748
http://hg.mozilla.org/mozilla-central/rev/be7cefceceac

Comment 204

[Matt Woodrow (:mattwoodrow)]

Thanks Dean!

Do you know what changed exactly? The only difference I can see is the scale[0] *= -1; to scale[i] *= -1; change, which I have fixed already.

Comment 205

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 16 - Implement transitions/animations for 3d transforms. v4

Fixed Tim's comments

Comment 206

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 17 - Add style tests for the new transform functions, and transitions v4

Fixed review comments, and made them all pass with and without the pref.

Carrying forward r=dbaron

Comment 207

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 19: Make all translate functions handle lengths and percents

Comment 208

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 13: Add basic reftests for 3d transforms v3

Lets check the tests in for now, but not add the folder to the main reftest.list.

Then they can be run easily when people need to, and we can trivially enable then when 3d transforms are enabled by default.

Comment 209

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #208)
> Created attachment 556453 [details] [diff] [review]
> Part 13: Add basic reftests for 3d transforms v3
> 
> Lets check the tests in for now, but not add the folder to the main
> reftest.list.

I recommend putting the addition to the main reftest.list in, but commented out, so people know there's a directory that's not being run.

Comment 210

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron [:dbaron] from comment #209) 
> I recommend putting the addition to the main reftest.list in, but commented
> out, so people know there's a directory that's not being run.

Sounds reasonable.

Comment 211

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 13: Add basic reftests for 3d transforms v4

Added disabled reftest.list entry.

Carrying forward r=roc

Comment 212

[Matt Woodrow (:mattwoodrow)]

Landed parts 13 and 17 on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/819f2a8e1318
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f201033b0e6

Comment 213

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/819f2a8e1318
http://hg.mozilla.org/mozilla-central/rev/9f201033b0e6

Comment 214

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 20 - Add more gfx3DMatrix transformation function and use these in nsStyleTransformMatrix

Comment 215

[Timothy B. Terriberry (:derf)]

Comment on attachment 557395 [details] [diff] [review]
Part 20 - Add more gfx3DMatrix transformation function and use these in nsStyleTransformMatrix

Review of attachment 557395 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with changes.

::: gfx/thebes/gfx3DMatrix.cpp
@@ +349,5 @@
> +  _24 = -sinTheta * temp + cosTheta * _24;
> +}
> +
> +void 
> +gfx3DMatrix::Multiply(const gfx3DMatrix& aOther)

This should be called something like PreMultiply (like in gfxMatrix) to distinguish it from the normal operator*=(). It would also be a lot simpler to implement it the same way:
  *this = aOther * (*this);

@@ +374,5 @@
> +  *this = temp;
> +}
> +
> +void
> +gfx3DMatrix::Multiply(const gfxMatrix& aOther)

Same comment here with respect to the name.

::: layout/style/nsStyleTransformMatrix.cpp
@@ +118,5 @@
>              NSAppUnitsToFloatPixels(offset, aAppUnitsPerMatrixUnit);
>  }
>  
> +static void
> +ProcessTranslatePart(double& aResult,

Can't you just make the ProcessTranslatePart() function above return a float instead of taking an aResult outparam? Then you wouldn't need this extra version.

@@ +258,5 @@
>     * | dx  0  0  1 |
>     * So E = value
>     * 
>     * Otherwise, we might have a percentage, so we want to set the dX component
>     * to the percent.

We're no longer constructing a matrix here, so this comment should be updated. A similar comment showing the equivalent matrix at the declaration of gfx3DMatrix::Translate() (and its friends) might be useful, however. Same for all the other, similar comments below.

@@ +603,5 @@
>    float depth;
>    ProcessTranslatePart(depth, aData->Item(1), aContext,
>                         aPresContext, aCanStoreInRuleTree,
>                         0, aAppUnitsPerMatrixUnit);
>    NS_ASSERTION(depth > 0.0, "Perspective must be positive!");

You've got this same assertion in gfx3DMatrix::Perspective(). You probably only need it in one place, and I think there is better.

::: layout/style/nsStyleTransformMatrix.h
@@ +79,5 @@
>    /**
>     * Given an nsCSSValue::Array* containing a -moz-transform function,
>     * returns a matrix containing the value of that function.
>     *
>     * @param aData The nsCSSValue::Array* containing the transform function.

The documentation here needs to be updated to include the aMatrix parameter. You may also want to put the full documentation on the public ReadTransforms() instead of this private method.

@@ +102,2 @@
>  
> +  static void ProcessMatrix(gfx3DMatrix& aMatrix, const nsCSSValue::Array *aData,

All of these functions look like they are only ever used as implementation details of the public nsStyleTransformMatrix functions, and are never referenced from anywhere else. That class has no data members and only static methods (its entire existence looks like an odd stand-in for a namespace). I realize it's good C++ form to expose lots of unnecessary details of your private implementation in the public headers, to spread the joy of recompiling when things change and to tempt people to expose things they can see right there, just under that pesky "private" keyword, but perhaps you should just make these into non-member, static (in the original C sense of the keyword) functions (so they only appear in the .cpp file).

Comment 216

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 20 - Add more gfx3DMatrix transformation function and use these in nsStyleTransformMatrix v2

Fixed review comments, and test failures. Carrying forward r=derf

Comment 217

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 556450 [details] [diff] [review]
Part 16 - Implement transitions/animations for 3d transforms. v4

See also comment 192 for comments on the nsComputedDOMStyle part.

Somewhat large changes needed:

 - check both 2d together, not separately
   - i.e., Decompose3DMatrix shouldn't call Decompose2DMatrix; caller
     should figure out which is right
   - and need to be careful to be consistent about what gets set
     or left untouched (e.g., Decompose2DMatrix has the odd behavior
     that it touches only one of the three parts of aShear.  It should
     probably set them to 0 appropriately -- and other parts to 0 or 1
     as appropriate.  And then you don't need to initialize).  Though
     maybe fixing that should wait until after you've fixed the handling
     for matrices that we can't decompose.

 - rotate3d() and perspective() should use the decomposition algorithm
   (though rotate3d() *could* use quaternions directly, but
   interpolating components is probably wrong).  I think this requires
   changing both the main switch() in AddTransformLists and the test
   around the AppendTransformFunction call at the top.

More detailed comments:

In AppendTransformFunction, if you put the:
  default:
    NS_ERROR("...");
*above* the longest list of cases (so that it falls through into the
nargs=1), you'll end up with more efficient code for opt builds, I
think, since the long list of cases will just translate to default:

Also, AppendTransformFunction should use 2-space indent.   But the best
way to fix this is by indenting the lines starting with "case" and
"default" by 2 additional spaces and leaving the code inside them where
it is.

Decompose3DMatrix should be 2 space indent rather than 4 space.

The comment at the top of Decompose3DMatrix should have a comment
saying that it's derived from the code in Graphics Gems, so that it
complies with the license on the graphics gems.

At the end of Decompose3DMatrix, could you change this:
  aRotate = local;
to:
  aRotate = gfxQuaternion(local);
to make it clearer that there's a matrix -> quaternion assignment there.

At some point we probably want to handle decomposition failure better,
so we claim to be unable to interpolate, rather than interpolating
between the identity matrix and something else.

>+  // TODO: Would it be better to interpolate these as angles? How do we convert back to angles?

probably better to just say why it's better to interpolate as shears
and that we're not interpolating as angles.

In AddTransformLists, at least add a comment suggesting that maybe
this isn't the best way to interpolate perspective.

Could you fix the indentation in the second lines of the function
calls in the translate3d and scale3d cases?

The rotate3d case shouldn't have separate code; it should use the matrix
decomposition.

>+              // rotatez and rotate are identical, is there an easier way to check this?

If you want this comment, put it in the TransformFunctionsMatch
function.

(I also think it might not be that hard to do the general Add here
with 2 coefficients, though maybe I'm missing something.)

Also, the special case for aList1 == aList2 doesn't make much sense to
me, given that that makes
nsStyleTransformMatrix::ProcessInterpolateMatrix treat list1 as the
identity.  It's not clear to me why that special case is there; maybe
just remove it, and along with it the special handling of non-list
interpolatematrix items in nsStyleTransformMatrix?

r=dbaron with those changes


Comments on other code:

gfx3DMatrix::Skew* methods seem poorly named, since Skew is usually
described as an angle but shear as what these take.

gfxQuaternion::Slerp should have a comment expanding the acronym,
perhaps?

Comment 218

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 555951 [details] [diff] [review]
Part 18 - Make the perspective() transform function actually fail on numbers <= 0

I'd say just remove the line you're modifying instead of making the modification; you're inside checks that you're either eCSSToken_Dimension, or that you're eCSSToken_Number and 0.  Both token types have a valid mNumber, so you're fine just checking tk->mNumber without a tk->mType check.

r=dbaron with that

Comment 219

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 556452 [details] [diff] [review]
Part 19: Make all translate functions handle lengths and percents

I don't think we want all of the translation places to take numbers; translate{X,Y,Z,,3D}() should continue to take only <length>, <percent>, or <calc>.  We just want matrix() and matrix3d() to allow number in the translation parts.  (Did I say otherwise somewhere, or was there discussion otherwise?)

I'd also prefer not to have VARIANT_* names that don't say what's in them.  In other words, if you're adding VARIANT_NUMBER to something, it should also get an N inserted in its name.  (And while you're there, I don't see the point of having TRANSFORM in the name -- I'd go with VARIANT_LP_CALC, VARIANT_LPN_CALC, VARIANT_LN_CALC, or similar.)

Also, it's helpful if you include the commit message in patches that you post for review.

Comment 220

[Matt Woodrow (:mattwoodrow)]

 
> Also, the special case for aList1 == aList2 doesn't make much sense to
> me, given that that makes
> nsStyleTransformMatrix::ProcessInterpolateMatrix treat list1 as the
> identity.  It's not clear to me why that special case is there; maybe
> just remove it, and along with it the special handling of non-list
> interpolatematrix items in nsStyleTransformMatrix?
> 


The problem I'm trying to solve here is that the inputs to AddTransformLists when interpolating from 'none' to a list, is two identical lists and coefficients that don't add to 1 (eg, 0.0, 0.5).

Since coeeficients that add to more than 1 don't make sense for transforms, it's preferable to have a null list object, and a single coefficient that just represents the progress through the animation.

AddDifferentTransformLists is detecting this special case, and inserting a null list, instead of the same list twice. nsStyleTransformMatrix::ProcessInterpolateMatrix detects this empty list, and using an identity matrix for decomposition.

The aList1 == aList2 check in AddTransformLists is there because we construct temporary list objects to pass into AddDifferentTransformLists, and this breaks the detection of identical list pointer.

Comment 221

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron [:dbaron] from comment #219)
> Comment on attachment 556452 [details] [diff] [review]
> Part 19: Make all translate functions handle lengths and percents
> 
> I don't think we want all of the translation places to take numbers;
> translate{X,Y,Z,,3D}() should continue to take only <length>, <percent>, or
> <calc>.  We just want matrix() and matrix3d() to allow number in the
> translation parts.  (Did I say otherwise somewhere, or was there discussion
> otherwise?)

You suggested taking lengths for all of the translate functions in https://bugzilla.mozilla.org/show_bug.cgi?id=505115#c113

Comment 222

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #221)
> (In reply to David Baron [:dbaron] from comment #219)
> > Comment on attachment 556452 [details] [diff] [review]
> > Part 19: Make all translate functions handle lengths and percents
> > 
> > I don't think we want all of the translation places to take numbers;
> > translate{X,Y,Z,,3D}() should continue to take only <length>, <percent>, or
> > <calc>.  We just want matrix() and matrix3d() to allow number in the
> > translation parts.  (Did I say otherwise somewhere, or was there discussion
> > otherwise?)
> 
> You suggested taking lengths for all of the translate functions in
> https://bugzilla.mozilla.org/show_bug.cgi?id=505115#c113

Ah, interesting.  :-)  If WebKit accepts raw lengths there, then we probably should too, but if it doesn't, we should only accept raw lengths where it does (and I know it does in the matrix* functions.)

Comment 223

[Matt Woodrow (:mattwoodrow)]

WebKit apparently doesn't accept raw numbers for translate*, so I'll update the patch to only support this for the matrix* variants.

Comment 224

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 16 - Implement transitions/animations for 3d transforms. v5

Fixed review comments, carrying forward r=dbaron,derf

Comment 225

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 18 - Make the perspective() transform function actually fail on numbers <= 0 v2

Fixed review comment, carrying forward r=dbaron

Comment 226

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 19: Make matrix* functions handle lengths and percents

Removed the changes to the translate* functions, and fixed the TRANSFORM macros as suggested

Comment 227

[Matt Woodrow (:mattwoodrow)]

 
> Also, it's helpful if you include the commit message in patches that you
> post for review.

Sorry, I usually only add these when I go to land patches. I've added them for all the new patches I just uploaded.

Comment 228

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 561962 [details] [diff] [review]
Part 19: Make matrix* functions handle lengths and percents

You should probably have a test in property_database.js that the following are invalid:
 * matrix3d() with a % value in the translatez part
 * matrix3d() with a length value in the _44 part

r=dbaron with that

Comment 229

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97983a84f0c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/29b915205135
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1af522b8b47
https://hg.mozilla.org/integration/mozilla-inbound/rev/24fa4e8bed1a

Comment 230

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 21: Enable 3D transforms!

This is just a backout of the patch from bug 677173, plus flipping the pref to enabled.

I want to land this just after the aurora merge to get as much testing coverage as possible before it hits a release.

Comment 231

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do 3D transforms actually work when we don't have an accelerated layers backend?

Comment 232

[Matt Woodrow (:mattwoodrow)]

Yes, we support these in BasicLayers via pixman.

Comment 233

[Geoff Lankow (:darktrojan)]

Matt, I noticed this line in your last patch (reftest.list) which needs fixing:
> # 3d transforms - disabled currently

Comment 234

[Ed Morley [:emorley]]

Part 16: https://hg.mozilla.org/mozilla-central/rev/97983a84f0c3
Part 18: https://hg.mozilla.org/mozilla-central/rev/29b915205135
Part 19: https://hg.mozilla.org/mozilla-central/rev/f1af522b8b47
Part 20: https://hg.mozilla.org/mozilla-central/rev/24fa4e8bed1a

Not sure what else is left to checkin, please close if everything done here (and mark mozilla9) - thanks :-)

Comment 235

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 561953 [details] [diff] [review]
Part 16 - Implement transitions/animations for 3d transforms. v5

># HG changeset patch
># Parent 50b36274e6896899e9b3e1de6b4e0ad75ed60a37
>--- a/layout/style/nsComputedDOMStyle.cpp
>+++ b/layout/style/nsComputedDOMStyle.cpp
>@@ -1053,37 +1053,63 @@ nsComputedDOMStyle::DoGetMozTransform()
>+  resultString.Append(NS_LITERAL_STRING("("));
>+  resultString.Append(NS_LITERAL_STRING(")"));

For future reference, I think these would be ever so slightly more efficient as resultString.Append('('); and resultString.Append(')');.

Comment 236

[Matt Woodrow (:mattwoodrow)]

Landed Part 21 on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/730a82c21c53

That should conclude this bug, unless it gets backed out :)

Comment 237

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/730a82c21c53

\o/

Comment 238

[Ioana (away)]

Verified as fixed on Aurora (Firefox 10):
layout/reftests/transform-3d passes on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7341560&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7342134&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7344904&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7344827&full=1&branch=mozilla-aurora

/tests/layout/style/test/test_transitions_per_property.html and layout/base/tests/test_preserve3d_sorting_hit_testing.html are passing on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7342105&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7342137&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7344446&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7344634&full=1&branch=mozilla-aurora

Comment 239

[Ioana (away)]

Verified as fixed on Central (Firefox 11):
layout/reftests/transform-3d passes on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7382780&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7383302&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7383253&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7383649&full=1&branch=services-central

/tests/layout/style/test/test_transitions_per_property.html and layout/base/tests/test_preserve3d_sorting_hit_testing.html are passing on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7383643&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7382748&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7383073&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7383082&full=1&branch=services-central

This bug will only be closed when the CSS3 3D Transforms feature will be verified on a RC.

Comment 240

[Daniel Holbert [:dholbert]]

(In reply to Ioana Budnar [QA] from comment #239)
> This bug will only be closed when the CSS3 3D Transforms feature will be
> verified on a RC.

This bug is already closed. (RESOLVED|FIXED)

Perhaps by "closed" you meant "officially marked as verified"?  IIRC, we don't have that requirement in general -- once a bug is verified as fixed on trunk (or whatever branch it lands directly on), that's sufficient to mark the bug as "verified".

(Thank you for the comprehensive verification, btw!)

Comment 241

[Ioana (away)]

Daniel,

I meant closed from the QA point of view, which means setting it as VERIFIED FIXED ("officially marked as verified" as you put it). I am not working on this bug by the general requirements because it is a feature tracking bug and I need to make sure the feature enters betas and RC without any (major) issues. Please let me know if you have any concerns about this.

Comment 242

[Daniel Holbert [:dholbert]]

(Ah, ok -- I hadn't realized that we wait for feature-tracking bugs to reach an RC before marking them as verified -- but it may be that I just never noticed that, and it sounds like you know what you're doing, so I'll be quiet. :))

Comment 243

[Ioana (away)]

This issue has also been verified as fixed on Firefox 10 beta 1:
layout/reftests/transform-3d passes on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8104308&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=8105180&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=8105483&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=8105382&full=1&branch=mozilla-beta

/tests/layout/style/test/test_transitions_per_property.html and layout/base/tests/test_preserve3d_sorting_hit_testing.html are passing on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8105147&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=8105282&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=8105277&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=8104017&full=1&branch=mozilla-beta

The manual tests here http://bit.ly/tbXKxE also pass with known issues.

Comment 244

[Ioana (away)]

This feature has been shipped (all the major issues were fixed).