We'd like to implement the proposed CSS3 Transform property, which WebKit recently added to their development trunk. ('-webkit-transform') WebKit's proposal spec is: http://webkit.org/specs/CSSVisualEffects/CSSTransforms.html and a simple demo page is: http://tekkie.flashbit.net/tmp/css3_transform_sample.html

To try out WebKit's behavior on transforms, you can get WebKit nightlies at: http://nightly.webkit.org/ or in Ubuntu Linux: sudo apt-get install midori (midori uses a fairly recent webkit trunk build)

Marking wanted1.9.1? to get this on the triage queue.

Bumpin' to P1 as I think this would be great...

Keith Schwarz is taking this on as his internship project. Go Keith! :)

Basic (and somewhat broken) functionality... can parse the -moz-transform property and render it correctly, but has some issues with invalidation. Click detection is not yet implemented.

Correction... previous patch is invalid. This is the current WIP patch.

Comment on attachment 328393 [details] [diff] [review] WIP Patch #2 >+PRBool nsDisplayTransform::OptimizeVisibility(nsDisplayListBuilder *aBuilder, nsRegion *aVisibleRegion) >+{ >+ return PR_TRUE; >+} >+ >+#include <time.h> >+ >+/* HitTest does some fun stuff with matrix transforms to obtain the answer. */ >+nsIFrame *nsDisplayTransform::HitTest(nsDisplayListBuilder *aBuilder, nsPoint aPt, HitTestState *aState) You probably want this "#include <time.h>" at the top of the file with the other #includes, right?

This is a test case that showcases Javascript interacting with the transform property. It should cause the Google logo to grow, shrink, and rotate. As of the second WIP patch, this test case renders correctly, but may have some issues with highlighting.

This test case showcases the transform property applied to a form. As of WIP patch 2, the form renders correctly but does not correctly support click detection. Also, selecting the form text causes unusual results.

This looks great! One thing I noticed on testcase 2: the form controls aren't always repainting correctly when I navigate through them with the keyboard. Experiment A: - Load testcase 2 - hit tab 3 times, to focus the "Click when finished" button Result: * Radio button focus outline isn't entirely erased. * focus-outline is missing on upper-left & bottom-right corners of button Experiment B: - Load testcase 2 - Hit tab 2 times, to focus a radio button - Hit uparrow, to switch radio buttons Result: * Both radio buttons are only partly repainted with their new state.

(In reply to comment #10) > One thing I noticed on testcase 2: the form controls aren't always repainting > correctly when I navigate through them with the keyboard. One more related experiment: Experiment C: - Load testcase 2 - Hit tab 1 time, to focus the combobox - Press downarrow. Result: - Combobox doesn't update to show new selected choice ("Last digit is 1"), until I click the mouse somewhere inside the window (which apparently forces a repaint) Also, one nitpick: The description at the top of the page says "Current Test: Skew matrix: skewy(20deg) with form elements.", but the page source shows "rotate(-10deg);" is the actual transform you're using.

I looked at the patch briefly -- didn't get very far in -- but I noticed in the very first change, you're changing the IID of the wrong interface (you should change nsIDOMNSCSS2Properties, not nsIDOMCSS2Properties).

WIP Patch #3 fixes several bugs from WIP Patch #2. Fixes: 1. The patch now correctly changes the IID of the nsIDOMNSCSS2Properties interface. 2. The -moz-transform-origin property now works correctly. 3. The -moz-transform property now correctly works on top-level elements, not just elements contained in a block-level element. Known Issues: 1. When changing the -moz-transform property for an element using JavaScript, only the old overflow rectangle is invalidated. 2. When part of the overflow rectangle for the transform region is invalidated, the transformed area is not invalidated. 3. Clicking-and-dragging the transformed part of a transformed element doesn't trigger the "floating drag" feature. 4. Click detection for transformed elements doesn't work. 5. Changing the -moz-transform-origin property in JavaScript corrupts the -moz-transform property. 6. The computed style value for the -moz-transform property contains extra whitespace. 7. Nested elements using the -moz-transform property don't render correctly.

WIP Patch #4 Fixes: 1. Changing the transform property of an element now correctly invalidates the new overflow area. 2. Click detection now works for transformed objects. Known Issues: 1. The transformed area isn't invalidated correctly (e.g. scrolling or selecting items in the transform area doesn't issue a redraw correctly). 2. Cannot drag items except from their original bounding rectangles (for the drag-and-drop feature) 3. Changing the -moz-transform-origin property of a non-block level element corrupts the -moz-transform property. 4. The serialized version of the -moz-transform property has extra whitespace not present in the original 5. Webkit's proposed specs allow the translate family of properties to accept percentages, but the current implementation doesn't allow this.

(In reply to comment #14) > Known Issues: > 1. The transformed area isn't invalidated correctly (e.g. scrolling or > selecting > items in the transform area doesn't issue a redraw correctly). You probably want to look at nsFrame::InvalidateInternal and (for invalidation from style changes) at the ApplyRenderingChangeToTree function in nsCSSFrameConstructor.cpp. > 3. Changing the -moz-transform-origin property of a non-block level element > corrupts the -moz-transform property. I suspect this is due to bugs in nsRuleNode::ComputeDisplayData. The nsRuleNode::Compute*Data functions must not touch the style data if there's nothing specified, since they're sometimes computing a set of partial data on top of a copy of the computed data for what they're building on (aStartStruct). I'd also note that there are many places where the patch isn't following local whitespace / indentation conventions, which it should do. (This includes the use of tabs in files that don't use tabs.)

Comment on attachment 328922 [details] [diff] [review] WIP Patch #4 Correction 2 >diff -r bf20f88e6916 layout/reftests/bugs/433700-ref.html >--- a/layout/reftests/bugs/433700-ref.html Thu Jul 10 09:36:24 2008 -0400 >+++ b/layout/reftests/bugs/433700-ref.html Thu Jul 10 09:56:55 2008 -0700 >@@ -34,7 +34,6 @@ fieldset div { padding-left:60px; } > .legend { display:block; } >- Also -- looks like you accidentally deleted a newline from this unrelated reftest.

+/* The box is opaque iff the transform is perfectly rectangular. This happens only when we have either + * the identity transform or a complete rotation. For simplicity, though, we'll just assume neither + * is true, since it's unlikely that either case will occur. A pure scale seems like it might be common, and that can be opaque, but don't worry about it for now. +/* Helper function that, given a rectangle and a matrix, returns the smallest rectangle containing the + * image of the source rectangle. Can't you use gfxMatrix::TransformBounds? const nsIFrame *const aFrame I don't find the second 'const' here valuable, and we don't generally do this, so you should probably remove it. + nsIFrame *ReferenceFrame() + { + return const_cast<nsIFrame *>(static_cast<const nsDisplayListBuilder *>(this)->ReferenceFrame()); + } I don't understand why this is necessary, can't you just return mReferenceFrame? The reason for TryMerge is not so much to increase performance as to ensure correctness in certain situations. For example if you have a transformed inline element that breaks across line boundaries and it has opacity:0.5, we need to render the whole element as a single unit before applying opacity. If each frame is wrapped in an nsDisplayTransform wrapper we won't be able to do that. You should revert your changes to nsBlockFrame.cpp The spec says it applies to inline and block elements but your comments make it sound like it only applies to block elements. TransformedItem should be IsTransformed or something like that. You'll need to have some code to make the transformed element an abs-pos and fixed-pos containing block. The fixed-pos containing block is going to be a little bit of work since currently it's hardwired to the viewport. I would use nsTransformFunction instead of nsXfrmFunction. The days of 'creat' are over. I have some patches in the bling branch that clean up invalidation so everything goes through InvalidateInternal, which you need to hook. I'll pick those patches out and submit them, you'll need them.

WIP Patch #5 Fixes from last patch: 1. The coordinate system for rotations is now corrected to match WebKit's implementation. 2. Changing the -moz-transform-origin property no longer corrupts the -moz-transform property. 3. The transformed area is no longer clipped based on the untransformed position of the element. 4. Transformed frames now correctly invalidate themselves. 5. Click-and-drag to highlight text now correctly works for transformed elements. 6. Frames split by multiple continuations now have the transform applied to all continuation frames as a batch, not to each frame individually. 7. Various style fixes from the previous patch. Known Issues: 1. Drop-down menus in form controls still drop down relative to the original position. 2. Drop-down menus in form controls don't transform their drop-down list. 3. Rotated scrollbars don't respond to mouse events correctly (e.g. always assume left-right and up-down axes.) 4. Transformed scrollbars cause graphical glitches (waiting for roc's patches to land) 5. Nested transforms result in a larger-than-necessary overflow rectangle. 6. Absolutely-positioned elements aren't factored into the transform overflow rectangle (possibly not a bug... looking into WebKit's implementation) 7. WebKit supports a "skew" transform that isn't handled by the -moz-transform property. 8. Elements with continuations sometimes don't display when the page is loaded. 9. Can't drag a transformed object for the "drag-and-drop" feature except when dragging from the object's original rectangle. 10. The serialized version of the transform property has extra whitespace. 11. The translate family of transforms don't accept percentages, though the proposed spec allows this.

This is a test case that showcases a known issue with continuation frames. Currently, the -moz-transform property is designed to consider all continuation frames as the same frame for the purposes of the transform origin. For example, if there were a multi-line 'span' element with a transform applied, even if the span spanned multiple lines, all of the text of the span would be transformed relative to the same point. The problem is that the -moz-transform-origin property can accept percentage values that indicate the fraction of the frame rectangle to move over when placing the origin. When the page is initially being reflowed, text elements that will have multiple continuations have each continuation compute its overflow rectangle, accounting for the transform. The problem is that as more and more continuations are added, the newer continuations will use different bounding rectangles than the original elements, since the union of all of the continuations' bounding rectangles changes as more continuations are added. However, the older continuations don't have their overflow rectangles adjusted. The net result is that earlier continuations sometimes don't display during initial page load, though in later reflows they tend to be correct. This test case has a transformed span and a script that appends text children. If you run this test case with the current WIP patch and then click "click to add text" several times, you can see that although the transform changes, the old overflow rectangles do not and there will be some extra text artifacts. roc: Do you have any suggestions about how to solve this problem? Is there an elegant and efficient way to have old continuations recompute their overflow rects as new elements are applied?

Actually now that you mention it, I ran into this in my SVG effects work. There isn't really a good solution except to make drastic changes to the way overflow areas work, which we don't want to do for 1.9.1. In my case, the problem is hardly noticeable since the overflow area I compute for each continuation frame of an element with 'filter' tries to cover the whole filter result, so normally includes the overflow areas of all continuations we know about "so far"; in particular the last continuation's overflow area covers all the right stuff, so when things change we usually end up repainting enough. Your case is different however. I'll think about whether we can hack something in, but for now don't worry about it. You could perhaps disable support for percentage -moz-transform-origin on inlines, or just disable percentages altogether, to hide the problem if it bothers you.

Can this also happen with blocks in multi-column, or are there no cases where we'll skip reflow of the first column?

It could happen with blocks in multi-column, but that's a lot more of an edge case.

WIP Patch #6 Fixes from WIP Patch #5: 1. The 'skew' transform is now defined and works like WebKit's skew. 2. TryMerge is now defined for nsDisplayTransform. 3. Various style cleanups - _should_ have cleaned up tabs, hopefully this is fixed here. Also renamed functions based on roc's suggestions and eliminated stray #includes. Of the bugs mentioned in the previous patch, based on advice from roc and dbaron, the following bugs cannot easily be resolved in this patch and will be left as unfixed unless stated otherwise: 1. Nested transforms result in a larger-than-necessary overflow rectangle. This has to do with some general issues with the overflow system that will be addressed in a larger patch. 2. Elements with continuations have their overflow rects computed incorrectly. This has to do with how continuation frames compute overflow rectangles and cannot be resolved easily. 3. System-level widgets aren't affected by transforms. It may be possible to fix this, but from what I've gathered this may not be easily resolved. Known Issues: 1. Rotated scrollbars don't respond to mouse events correctly (e.g. assume vertical and horizontal motion necessary even when transformed). This may be a symptom of a larger issue in which events are always expressed relative to the default coordinate system and not the transformed system. 2. Transformed scrollbars cause graphical glitches (scrolling causes overdraw of scrollbars and doesn't fully invalidate the frames) 3. Elements with the -moz-transform property should be abs-pos and fixed-pos containing blocks. 4. Cannot use the click-and-drag feature on transformed elements except when clicking in the union of the transformed area and the original area. This may be related to (1). 5. The serialized version of the -moz-transform property has extra whitespace relative to the original. 6. The 'translate' family of transforms don't accept percentages, though the proposed spec allows this.

(In reply to comment #25) > 3. System-level widgets aren't affected by transforms. It may be possible to > fix this, but from what I've gathered this may not be easily resolved. What do you mean by "system-level widgets"? > 1. Rotated scrollbars don't respond to mouse events correctly (e.g. assume > vertical and horizontal motion necessary even when transformed). This may be a > symptom of a larger issue in which events are always expressed relative to the > default coordinate system and not the transformed system. Not sure what you mean here? Can you give a concrete example? > 2. Transformed scrollbars cause graphical glitches (scrolling causes overdraw > of scrollbars and doesn't fully invalidate the frames) The issue here is that we still try to do bitblit scrolling and even if we decided not to do that, we invalidate through the view system which is wrong. We shouldn't even try to do view-based scrolling in this case, we should just invalidate the frame. We can do this by adding a new view bit which says "I have a transform or other effect on me, use frame invalidation for any scroll operation on my descendants".

A few minor notes, while they're in my head: * nsTransformFunction.cpp should probably begin with the same short comment as nsTransformFunction.h, since the single-line comments that show up before any #includes are there because they show up as the description in http://mxr.mozilla.org/mozilla-central/source/layout/style/ * nsTransformFunction.h 's include guard is messed up following the rename

WIP Patch #7 More progress than before, but still a good ways to go. The main issues fixed here: 1. The event system now sends event coordinates to frames in the correct coordinate space, so transformed items will always receive events in the proper Cartesian plane. As a side-effect, click-and-drag detection is now operational. 2. Overflow rectangles are now computed correctly for unsized elements (elements without fixed sizes through CSS) 3. Transformed elements with scrollbars now display correctly. New known issues: 1. Drop-down menus for combo boxes respond to input in the transformed coordinate space, but draw their menus in untransformed coordinate space. 2. Autocomplete menus respond to input and appear in the untransformed coordinate space. 3. Absolute-positioned and fixed-positioned elements aren't factored into the overflow rectangle. 4. Click-and-dragging transformed elements draws the graphics incorrectly or in the wrong place. 5. Elements with overflow scrollbars sometimes don't redraw correctly when the main window is scrolled. 6. Rounding errors in matrix calculations lead to graphical aberrations. 7. Changing properties unrelated to -moz-transform forces a recalculation of the property and, consequently, a frame reconstruction.

WIP Patch #8 Numerous fixes from the earlier version of the patch, including: 1. The translate family of transforms now accepts percentage values in addition to absolute lengths. 2. Absolutely-positioned elements are now factored into the overflow rectangle for transformed elements. 3. Transformed elements are now abs-pos containing blocks. 4. Zooming no longer breaks transforms. 5. Transformed elements with scrollbars no longer have graphical glitches. New Known Issues: 1. Transformed elements aren't fixed-pos containing blocks. 2. Zooming during "Print Preview" causes graphical glitches with text in IFRAMEs. 3. IFRAMEs scaled up beyond a factor of 2 stop responding to user input. I've traced this to nsDisplayTransform::HitTest, but don't know exactly what's causing it. I think it has to do with a change of app unit/CSS pixel ratios for elements scaled beyond a certain point, but I'm not sure. 4. nsComptuedStyleValue for -moz-transform doesn't factor in the effects of percentage translations. I am still working on this one, so it should be resolved soon. 5. Some of the reftests with percentage translations versus absolute translations fail, even though they are ostensibly equal. There's probably a rounding error in here somewhere... I am interested in making system-level widgets subject to transformations the way that all other elements are, but I don't know how to do this. Any suggestions would be appreciated. Also, if you have any ideas why an IFRAME scaled by a factor of 2.00 will respond to user input while an IFRAME scaled by a factor of 2.01 will not, please let me know.

(In reply to comment #29) > 2. Zooming during "Print Preview" causes graphical glitches with text in > IFRAMEs. This sounds like bug 444448.

I still want to know what exactly you mean by "system-level widgets". If you're talking about popup menus and combobox dropdowns, I think we shouldn't bother with them except possibly to transform the anchor point so at least that's correct. + if(aScrolledView->IsInvalidateFrameOnScroll()) + { + nsIFrame *const frame = static_cast<nsIFrame *>(aScrolledView->GetClientData()); + frame->InvalidateOverflowRect(); + return; + } We really don't want to depend on nsIFrame here. Probably best to do this indirectly through nsIViewObserver/nsPresShell. Also, you should call AdjustChildWidgets here.

(In reply to comment #31) > I still want to know what exactly you mean by "system-level widgets". If you're > talking about popup menus and combobox dropdowns, I think we shouldn't bother > with them except possibly to transform the anchor point so at least that's > correct. > > + if(aScrolledView->IsInvalidateFrameOnScroll()) > + { > + nsIFrame *const frame = static_cast<nsIFrame > *>(aScrolledView->GetClientData()); > + frame->InvalidateOverflowRect(); > + return; > + } > > We really don't want to depend on nsIFrame here. Probably best to do this > indirectly through nsIViewObserver/nsPresShell. Also, you should call > AdjustChildWidgets here. > Thanks for the tip, I'll rework it a bit. May I ask if there's any reason that we want to distance the view and frame modules? As for "system-level widgets," I was referring to popups and dropdowns as you mentioned. I can try to transform the origin point if that's a good idea. Is there a spot in the code that's not platform-specific that computes the origin points for widgets that I could modify?

(In reply to comment #32) > Thanks for the tip, I'll rework it a bit. May I ask if there's any reason that > we want to distance the view and frame modules? It reduces complexity. I think. I'm not 100% sure but we're going to get rid of views anyway and I don't want to change the architecture now. > As for "system-level widgets," I was referring to popups and dropdowns as you > mentioned. I can try to transform the origin point if that's a good idea. Is > there a spot in the code that's not platform-specific that computes the origin > points for widgets that I could modify? There is, but I strongly urge you to leave this alone for now and work on it as a follow-up issue.

WIP Patch #9 This patch is getting close to being finished. The following are known issues that I will try to have ready for the final version of the patch: 1. Transformed frames that can't handle abs-pos child lists (e.g. tables) that have abs-pos or fixed-pos children cause those children not to display. 2. Transformed elements with scrollbars that are scaled up by a factor greater than two stop responding to user input. 3. Some of the attached reftests should pass but don't because of small pixel differences. 4. Transformed elements with continuations don't render correctly on page load or have their overflow rectangles computed correctly. 5. The serialized version of the -moz-transform property has unnecessary whitespace. The following are known issues that will not be addressed in this patch, either because they represent known architecture issues or because they should be addressed in a supplemental patch: 1. Widgets aren't transformed. 2. Nesting transforms cause the overflow rectangle of the topmost transform to be too large... this is because the overflow rectangles accumulate errors since they are always in the canonical basis instead of the transformed basis. 3. Transformed elements with abs-pos children have those abs-pos children overdraw the continuations of the transformed element. This is related to Bug #444448. All comments are welcome and appreciated.

The proposed spec for the transform property says that transform should apply to block-level and inline-level elements. I've checked WebKit's implementation of the transform property and it seems like they're not adhering very well to these rules... it's possible to transform a table cell, for example, but not a <span>. That said, it might be nice to have the transform property apply to all elements, not just block-level and inline-level ones. However, this causes some problems when interacting with the fact that transformed elements are supposed to be abs-pos and fixed-pos containing blocks. Some elements, like <table>s, don't have an abs-pos child list in our current implementation, so letting them get transformed leads to other issues where child frames of transformed <table>s disappear. Should I try to make the transform property only apply to block-level and inline-level elements, or should it apply to all elements? If it does apply to all elements, what should we do about elements that can't normally support an abs-pos list?

Tables are block-level. There are a few options: a) make transform only apply to display:block and display:inline-block (nsBlockFrame) b) make transform only apply to display:block, display:inline-block and display:inline (nsBlockFrame and nsPositionedInlineFrame) c) add nsAbsoluteContainingBlock functionality to nsTable(Outer?)Frame and make transform apply to tables, blocks and inlines (which would make us correct per-spec AFAIK) d) refactor abs-pos logic so that any element can be an abs-pos container and then make transforms apply to anything e) make transforms apply to tables, blocks and inlines, but don't bother making tables abs-pos containers, just push an null abs-pos containing block so that abs-pos children are not positioned. a) would avoid most problems with continuations. b) is closest to what you already have and is about as close to the spec as Webkit is, from what you say, although from a different direction. d) is more work than we want to do for an initial landing of this feature. c) probably isn't much harder than e) and brings us very close to the spec so I think c) makes the most sense ... unless making tables abs-pos containers is harder than I think. The patch in bug 438987 should be a good template.

(In reply to comment #36) > c) add nsAbsoluteContainingBlock functionality to nsTable(Outer?)Frame FWIW, these bugs seem related: bug 320865 bug 391153 (on making tables absolute/fixed-position containers, in the specific case of the root element).

(In reply to comment #35) > The proposed spec for the transform property says that transform should apply > to block-level and inline-level elements. I've checked WebKit's implementation > of the transform property and it seems like they're not adhering very well to > these rules... it's possible to transform a table cell, for example, but not a > <span>. That said, it might be nice to have the transform property apply to > all elements, not just block-level and inline-level ones. However, this causes > some problems when interacting with the fact that transformed elements are > supposed to be abs-pos and fixed-pos containing blocks. Some elements, like > <table>s, don't have an abs-pos child list in our current implementation, so > letting them get transformed leads to other issues where child frames of > transformed <table>s disappear. > > Should I try to make the transform property only apply to block-level and > inline-level elements, or should it apply to all elements? If it does apply to > all elements, what should we do about elements that can't normally support an > abs-pos list? > We'll fix the spec. Basically we expect transforms to be applicable to inline-block, inline-table, inline-box, inline replaced elements, but not to inline flows like spans.

If you disagree with this conclusion, then we basically need to to decide how a transform works on objects that can span multiple lines. You basically have to decide whether each line box transforms independently, or whether you use the entire bounding box, etc. (for stuff like transform-origin). For now we just punted on it and didn't bother supporting it. Be warned that our implementation in WebKit is very much under construction as well. We don't really work right with iframes or frames for example, or with native controls like scrollbars. (AppKit itself doesn't work with transforms.) If you use NSViews, you're going to run into trouble on the Mac with this like we have.

We do use NSViews, but we have tricks. We already transform iframes and native controls pretty well using SVG <foreignobject>, even on Mac. (And we will get rid of NSViews later to make stuff like this easier.) It sounds like you want to disallow transform on elements that create multiple CSS boxes. But then how should transforms interact with vertical breaking, such as columns? I know you don't create multiple boxes there but we do, and conceptually the CSS spec does too.

We punted on columns. That will need to be worked out. I suspect we may end up needing a property similar to the -break properties in CSS3 Borders and Backgrounds that will say whether the transform applies to the entire bounding box or to individual boxes.

If we're going to do that for blocks, then why not for inlines as well?

Transforming blocks split across columns or inlines split across lines is pretty much the same problem. It seems to me like the ideal behavior would be having the origin of the transform be determined using the union of all the boxes involved (although that is a little difficult for us to implement). That's what I suggested to Keith a few weeks ago, anyway. We want to refactor stuff to allow anything to be an abs-pos container anyway; we have existing (old) bugs on that (e.g., a relatively positioned table or internal table element should be an abs pos container). But I'd like to keep that separate from this patch.

Potential Patch #1 This patch addresses all issues marked above that aren't marked nofix, plus a few others. I've attempted to clean up the code and make sure the comments match the code. Asking for review by dbaron.

Comment on attachment 332449 [details] [diff] [review] Potential Patch #1 roc should review this as well

+ /* We want the element to be an absolute containing block if it's positioned. + * Normally, we'd want it to also be an abs-pos containing block, but until + * all frame types are capable of supporting abs-pos lists, this results in + * buggy behavior. Consequently, we'll check both that the element is positioned + * and that it's not transformed. + if (display->IsPositioned() && !display->mTransformPresent) { aState.PushAbsoluteContainingBlock(newFrame, absoluteSaveState); I'm confused. What is this trying to say? What's the difference between an "abs-pos containing block" and an "absolute containing block"? Why are we making positioned+transformed elements *not* be an abs-pos containing block when it would have been an abs-pos containing block if it had no transform? + // Create the inline frame. If it has a transform, make a positioned frame. + // Otherwise, just make a regular frame. + newFrame = (aDisplay->mTransformPresent ? NS_NewPositionedInlineFrame(mPresShell, aStyleContext) : + NS_NewInlineFrame(mPresShell, aStyleContext)); Why not just reuse the existing positioned-inline-frame path? + NS_ASSERTION(aState->mItemBuffer.Length() == static_cast<PRUint32>(itemBufferStart), Slightly simpler to use a constructor-style cast. Is nsUnitConverter really necessary? The only data that needs to be encapsulated is a scale factor. I'd prefer to just pass that around and use helper functions if necessary. Seems like you could be using gfxRect::RoundOut here. + return floorf(static_cast<float>(aCoord * aVal)); +#else + return static_cast<PRInt32>(aCoord * aVal); Constructor casts + const nscoord dx = (NSCoordMultiplyGfxFloat(bounds.width, disp->mTransformX[0]) + + NSCoordMultiplyGfxFloat(bounds.height, disp->mTransformY[0])); + const nscoord dy = (NSCoordMultiplyGfxFloat(bounds.width, disp->mTransformX[1]) + + NSCoordMultiplyGfxFloat(bounds.height, disp->mTransformY[1])); Why go through NSCoordMultiplyGfxFloat which casts to PRInt32, losing precision? Seems like we can do everything with gfxFloat here (double), and not worry about losing precision. In fact we can drop NSCoordMultiplyGfxFloat and do multiplication. +/* Returns the smallest rectangle containing a frame and all of its continuations. + * For example, if there is a <span> element with several continuations split over + * several lines, this function will return the rectangle containing all of those + * continuations. This rectangle is relative to the origin of the frame's local + * coordinate space. + */ Should mention what really happens when UNIFIED_CONTINUATIONS is not defined. + result.UnionRect(result, nsRect(delta.x + localRect.x, delta.y + localRect.y, + localRect.width, localRect.height)); Use "localRect + delta" + static nsIFrame* GetCrossDocParentFrame(nsIFrame* aFrame) + { + return const_cast<nsIFrame *>(GetCrossDocParentFrame(static_cast<const nsIFrame *const>(aFrame))); + } I'm not sure if it makes sense to try to make a meaningful distinction between "const nsIFrame*" and "nsIFrame*". One problem is that there are so many other pieces of data hanging off frames, making the frame itself const doesn't do anything to protect invariants. In any case, your second static_cast seems unnecessary. + /* First things first - if we're supposed to invalidate the frame on a scroll, + * just do it and skip the rest of the logic here. + */ + if(aScrolledView->IsInvalidateFrameOnScroll()) + { + /* First, invalidate ourselves. */ + GetViewManager()->GetViewObserver()->InvalidateFrameForView(aScrolledView); + + /* Next, adjust child widgets. */ + nsPoint offsetToWidget; + GetNearestWidget(&offsetToWidget); + AdjustChildWidgets(aScrolledView, offsetToWidget, aP2A, PR_TRUE); Can you use the existing path that does this? I've just done a very quick skim over most of it. I'll do some more shortly. Overall it seems very good.

So I got a bit done on the plane as well -- just a quick skim, really -- but I might not get back to it for a few days, so I'll post what I have so far, although I really don't have a whole lot of substantive comments yet. I'll start by skimming some parts of the style system changes, so this is a bit out-of-order: In nsCSSPropList.h, the new properties should sort (alphabetically) between top and unicode-bidi. > \ No newline at end of file You should have the newline at the end of file. (Unix convention uses LF as a line terminator; Windows convention uses CR-LF as a line separator. A number of Unix-ish tools are unhappy when there's anything following the last newline, and version control systems complain.) > diff -r 6c4ee89ddeb8 layout/reftests/moz-transform/reftest.list~ Don't add your backup files. > + * The Initial Developer of the Original Code is > + * Keith Schwarz <kschwarz@mozilla.com> > + * > + * Contributor(s): > + * Keith Schwarz <kschwarz@mozilla.com> I think the Initial Developer should be Mozilla Corporation. Then you can add "(original author)" after your name in the contributors list if you want. > +namespace > +{ What's the point of the unnamed namespace block here? > +#ifndef nsTransformFunction_h___ Don't use "__" in your include guards; all identifiers with "__" in them are reserved for the implementation. > + enum nsMatrixIndex{X_SCALE, X_SKEW, Y_SKEW, Y_SCALE, X_TRANSLATE, Y_TRANSLATE, NUM_ENTRIES}; Wrap this line at less than 80 characters, and use a space before the "{". Likewise for wrapping the declaration of SetToValue just below. > + nsStyleCoord &GetCoord(const PRInt32 index) Put the & before the space rather than after (or space on both sides). Throughout, you should have a space between "if", "for", or "switch" and the "(" following. (But there should be no space for function calls; leave those as you have them.) There are also a bunch of places where you indent the "{" two spaces and then its contents an additional two spaces. Only the contents of the braces should be indented; the braces themselves should be at the same indent as what comes before. Furthermore, in most parts of the code (including, I think, all the ones that you touch), the opening brace should only be on its own line if it's the opening brace for a class or function definition. For example, the switch in nsTransformFunction.cpp, a bunch of things in nsStyleStruct.cpp, nsRuleNode.cpp, etc. That is, do: if (foo) { bar(); } rather than: if (foo) { bar(); } But it's ok to have extra indent inside switches if you want, i.e.: switch (condition) { case 1: bar(); break; case 2: baz(); } (depending on local style in the file) +PRBool CSSParserImpl::ParseFunctionInternals(nsTArray<nsCSSValue> &aOutput, + PRInt32 aAllowedTypes, nsresult &aErrorCode) We tend to put out parameters after inputs. So aAllowedTypes should be first. It should probably also be called aVariantMask for consistency with ParseVariant.

The loops in TotalUntransformPoint and TotalTransformRect seem like a problem. They don't take into account SVG foreignobjects, but adding foreignobject checks there seems like a modularity violation. It seems like we need something more generic than GetOffsetTo that handles foreignobjects and transforms and future stuff. Indeed, in general everywhere we have GetOffsetTo, we should be thinking about what happens if there's a transform in the way! We might want to even warn if GetOffsetTo finds a transform in the way... I'm not sure what that API should look like. Perhaps // Compute a matrix which transforms from this frame's coordinate system to aFrame's coordinate system gfxMatrix GetMatrixTo(nsIFrame* aFrame); // Compute a matrix which transforms from this frame's coordinate system to the root frame's coordinate system virtual gfxMatrix GetMatrixToRoot(nsIFrame* aFrame); ? Then GetMatrixTo can make two calls to GetMatrixToRoot, or possibly use some fast path for the common case where there are no transforms around. Using GetMatrixToRoot instead of GetMatrixToParent to possibly speed things up and also to allow foreignobject to skip over the meaningless SVG frames between itself and its enclosing <svg>. Does that make any sense? I haven't thought about it deeply yet. I suspect things would be better if we made nsDisplayTransform be a leaf display item, whose paint method actually constructs display items for its frame subtree and whose HitTest method does something similar. That would avoid breaking the assumption that the items in a display list are all in the same coordinate system, so GetOffsetTo is safe to use for frames in the same display list. That would mean ClipListToRange doesn't work well for ranges that have one end inside transformed content and the other end outside, but I think we can live with that! + PRBool IsTransformed() const { return GetStyleDisplay() && + GetStyleDisplay()->mTransformPresent; } share the result of GetStyleDisplay(), it's not free!

Nitpick: Watch out for random newlines / edits to existing blank lines that you might have added in your patch. (Attachment 332449 [details] [diff] has blank-line-edits in nsPresShell.cpp, nsFrame.cpp, nsCSSParser.cpp, nsView.cpp, and nsViewManager.cpp)

Keith, it would probably be useful if you posted a new patch that addressed the comments so far; I suspect this will probably require a few iterations in the end.

I'm currently working on replacing calls to GetOffsetTo and the like with a more generic nsLayoutUtils function to change coordinate systems, as per roc's suggestion. I'll try to get some of those changes made and tested, and will hopefully have a revised patch posted by the end of the day.

Fixes in response to roc's and dbaron's comments about the previous patch. Some syntax cleanup to match the module stuff. The biggest changes are some changes to nsIFrame that unified code paths between SVG foreign objects and elements with the -moz-transform property which seem to make the code a whole lot prettier. Also, I've moved much of the content of nsDisplayTransform into nsLayoutUtils where (I believe) it's better suited.

David, if you like, I can take responsibility for reviewing everything except the style system changes. (Which I don't plan to even look at.) // See if it's relatively positioned "or transformed" rv = ConstructBlock(aState, aDisplay, aContent, aParentFrame, nsnull, aStyleContext, &newFrame, - aFrameItems, PR_FALSE); + aFrameItems, aDisplay->mTransformPresent); Can we actually get here if there's a transform present? Seems to me we'd have taken the "relative positioned" path. rv = ConstructInline(aState, aDisplay, aContent, - aParentFrame, aStyleContext, PR_FALSE, newFrame); + aParentFrame, aStyleContext, aDisplay->mTransformPresent, newFrame); Ditto. + const nsRect localRect = currFrame->GetRect(); + const nsPoint delta = currFrame->GetOffsetTo(aFrame) - currFrame->GetPosition(); + + result.UnionRect(result, localRect + delta); This can simplify to result.UnionRect(result, nsRect(currFrame->GetOffsetTo(aFrame), currFrame->GetSize()); + const gfxPoint newOrigin(nsLayoutUtils::AppUnitsToGfxUnits(aOrigin.x, aFactor) + toMozOrigin.x, + nsLayoutUtils::AppUnitsToGfxUnits(aOrigin.y, aFactor) + toMozOrigin.y); newOrigin = ... + toMozOrigin; ? + inline const nsIFrame* GetUnderlyingFrame() const Honestly, I think adding const-friendly code here is not worth it. See what dbaron thinks. + static nsRect TransformRect(const nsRect &untransformedBounds, aUntransformedBounds (occurs several times) + const nsRect *const boundsOverride = nsnull); aBoundsOverride + * TransformPoint takes in as parameters a point (in app space) and returns the transformed What is "app space"? Do TransformPoint and TransformRect really need an aOrigin parameter? It seems the caller could just as easily subtract aOrigin from the input point/rect. + /* Although most nsDisplay* constructors that take in an nsDisplayList + * modify that list somehow, the nsDisplayTransform constructor DOES + * NOT. Instead, the caller relinquishes control of the list to the + * nsDisplayTransform, which from that point forward is responsible + * for it. This is a bit odd, but it eliminates problems where inside + * the constructor the nsDisplayTransform finds that it doesn't have + * enough memory to allocate a new list for itself. Why can't nsDisplayTransform just contain an nsDisplayList as a direct member? + /** + * Given a frame, computes the net bounding rectangle for that frame. The net bounding + * rectangle is the rectangle, where (0, 0) is the frame's origin, containing the frame and + * all of its continuations. + */ + static nsRect GetNetFrameBounds(const nsIFrame *const aFrame); You should mention the effect of UNIFIED_CONTINUATIONS. Also, I'm not super happy about the name GetNetFrameBounds, but I can't think of a better one right now. +nsLayoutUtils::RoundGfxRectToAppRect(const gfxRect &aRect, const PRInt32 aFactor) Might as well make aFactor a double, since it will have to be converted anyway. Same for all these helpers. + /* Now just typecast everything! */ + return nsRect(nscoord(scaledRect.pos.x), nscoord(scaledRect.pos.y), + nscoord(scaledRect.size.width), nscoord(scaledRect.size.height)); Might want some assertions here to check that everything's in range. + /* Since we apply transforms from the innermost transform to the outermost transform, + * we need to invert the transforms from the outermost transform to the innermost transform. + * We'll do this by ascending upward and finding all of the transform frames, storing them in + * a stack, and inverting in reverse order. + */ Why not build up the global transform matrix, then invert it at the end and apply to the point? In fact, why not have one function that calculates the global transform matrix, and then have this function invert it and apply to the point? + for (PRInt32 index = static_cast<PRInt32>(frameStack.Length() - 1); index >= 0; --index) { Constructor cast + static PRBool HasMozTransform(const nsIFrame *const aFrame) + { + const nsStyleDisplay *const disp = aFrame->GetStyleDisplay(); + return disp && disp->mTransformPresent; + } You don't need to null-check disp. I'm not sure if this is worth having, "nsLayoutUtils::HasMozTransform(frame)" isn't much shorter than "aFrame->GetStyleDisplay()->HasTransform()". (Especially if GetStyleDisplay is already available in a local variable in callers.) And it shouldn't be called MozTransform in any case; there's no need to use vendor prefixes in the names in our own code. + static inline nscoord GfxUnitsToAppUnits(const gfxFloat aPos, const PRInt32 aFactor) + { + return nscoord(NSToIntRound(float(aPos)) * aFactor); + } What's wrong with NSFloatPixelsToAppUnits? + /* Converts a number of app units into graphics pixels. */ + static inline gfxFloat AppUnitsToGfxUnits(const nscoord aPos, const PRInt32 aFactor) + { + return gfxFloat(aPos) / aFactor; + } Why not use NSAppUnitsToFloatPixels? +PRBool +nsIFrame::IsTransformed() const +{ + return nsLayoutUtils::HasMozTransform(this); +} Might as well just return GetStyleDisplay()->IsTransformed() here. + if (nsLayoutUtils::HasMozTransform(this)) { I'm not sure how this will look in the next iteration, but we should probably use a local variable to cache isTransformed. Your changes to Invalidate and FinishAndStoreOverflow are going to conflict with similar changes in my patch in bug 450340. I'm not sure who's going to land first, but someone will have to clean up a little bit. I think it shouldn't be a problem, just giving you a heads-up. +PRBool +nsIFrame::NeedsView() +{ + return nsLayoutUtils::HasMozTransform(this); You could probably move this up to where we call NeedsView (IIRC there's only one call site), we probably already have the display style struct there. Up to nsGfxScrollFrame.cpp

+ /** + * Returns a transformation matrix that converts points in a frame's canonical + * coordinate system (e.g. the coordinate system inhabited by its frame rect) + * into points in its actual coordinate system. The matrix is expressed such + * that points transformed by the matrix should be expressed in device pixels. This still reads a bit ambiguous to me. I don't think "canonical" is the right word. You might want to explain that this can only be called for frames that return true for IsTransformed(), and maybe give an example of how this should be used. GetTransformMatrix is a weird API because it gives us a matrix whose meaning is unclear. I think it should probably return not just a matrix but also the ancestor frame that the matrix transforms coordinates to. For foreignObject that ancestor is the SVG subtree root (the nsSVGOuterSVGFrame), for CSS transforms it should be the parent frame (with the matrix adjusted to suit). Does that sound reasonable? +++ b/layout/reftests/moz-transform/abspos-1-ref.html The test directory should be called "transform". + /* Regrettably, we have to do a const_cast here to strip the constness off of ourselves. + * Even though GetTMIncludingOffset is semantically const, because it goes through + * nsCOMPtr and getter_AddRefs, we cannot have the method marked const. This is unsightly + * and hopefully, when we const-correct everything, this will go away. I kinda feel this isn't worth the effort and we should make GetTransformMatrix non-const. + const nsPoint delta = child ? -GetOffsetTo(child) : nsPoint(0, 0); I think the child's always at 0,0 so we don't need this. + /** + * Foreign objects can return a transform matrix, obtained by + * converting the stored SVGMatrix into a gfxMatrix. Not really "stored", so I'd just remove the second clause. IsInvalidateFrameOnScroll isn't a great name. I'd call it NeedsInvalidateOnScroll. Also, I don't know why we need to check it from CanScrollWithBitBlit as well as nsScrollPortView::Scroll, why is that? That's all I have! Amazing work!

Comment on attachment 335962 [details] [diff] [review] Potential Patch #2 I think the parsing code for -moz-transform-origin should just reuse the parsing code for background-origin. (background-origin is a value pair, which I think -moz-transform-origin should be as well. And the spec currently has some slight differences in the grammar, but I think they ought to be the same. We should probably discuss this with the WebKit folks.) (Does WebKit not implement the Z component either?) ===== >+ * Returns whether the frame is transformed from the -moz-transform property. >+ */ >+ static PRBool HasMozTransform(const nsIFrame *const aFrame) >+ { >+ const nsStyleDisplay *const disp = aFrame->GetStyleDisplay(); >+ return disp && disp->mTransformPresent; >+ } GetStyleDisplay is guaranteed never to return null, so you can just return GetStyleDisplay()->mTransformParent (despite comment 48). And it seems like this could stay a method on nsIFrame rather than in nsLayoutUtils (not sure whether it should be inline -- probably not). ===== nsCSSFrameConstructor.cpp: >+ nsAbsoluteItems& GetFixedItems() >+ { >+ return const_cast<nsAbsoluteItems &>(static_cast<const nsFrameConstructorState *>(this)->GetFixedItems()); >+ } >+ const nsAbsoluteItems& GetFixedItems() const >+ { >+ return mFixedPosIsAbsPos ? mAbsoluteItems : mFixedItems; >+ } >+ I think it would be cleaner just to repeat the contents of the method twice. If you really don't want to do that, I think your static_cast can be a const_cast. >+ /* Positioned elements should act as abs-pos containing blocks. >+ * Normally, we treat transformed elements as though they're >+ * abs-pos containers, but because most frame classes can't >+ * support abs-pos lists, we'll ignore that for now because >+ * the other functions (e.g. ConstructBlock / ConstructInline) >+ * will handle abs-pos containment correctly. >+ */ >+ if (display->IsPositionedIgnoringTransform()) { This seems like an odd asymmetry. We do enter this case for positioned elements that don't support being an absolute containing block, but we don't for transformed ones? Why the difference? (I'm not saying your way is necessarily wrong, but the difference seems wrong.) That said, I suspect currently nothing constructed in ConstructHTMLFrame supports being an abs pos container. So maybe it's just the comment that's wrong. nsDisplayList.cpp: >- NS_ASSERTION(aState->mItemBuffer.Length() == itemBufferStart, >+ NS_ASSERTION(aState->mItemBuffer.Length() == PRUint32(itemBufferStart), really what probably should happen here is that itemBufferStart and i are both PRUint32, and the for loop changes to: for (PRInt32 i = aState->mItemBuffer.Length(); i-- != itemBufferStart; ) { although that's a bit ugly too. Not sure what roc thinks; he owns this code. (I'm assuming you were just fixing a compiler warning.) >+// Write #define UNIFIED_CONTINUATIONS here to have the transform property try to transform >+// content with continuations as one unified block instead of several smaller ones. >+// This is currently disabled because it doesn't work correctly, since when the frames >+// are initially being reflowed, their continuations all compute their bounding rects >+// independently of each other and consequently get the wrong value. >+// Write #define DEBUG_HIT here to have the nsDisplayTransform class dump out a bunch of >+// information about hit detection. Traditionally we actually have the #define in question either: * written, commented out, or * written as an #undef (and generally with two spaces after the undef so that it can be replaced by define plus one space) >+#ifdef DEBUG >+/* Helper function to print out a rectangle and some extra info about it. */ >+static void PrintRect(const nsIFrame *const aFrame, const char *const aMsg, const nsRect &newRect) >+{ >+ printf("Frame %p: '%s': (%f, %f), (%f, %f)\n", >+ dynamic_cast<const void *>(aFrame), aMsg, >+ nsPresContext::AppUnitsToGfxCSSPixels(newRect.x), >+ nsPresContext::AppUnitsToGfxCSSPixels(newRect.y), >+ nsPresContext::AppUnitsToGfxCSSPixels(newRect.x + newRect.width), >+ nsPresContext::AppUnitsToGfxCSSPixels(newRect.y + newRect.height)); >+} >+#endif Not sure if this is still needed. You have two versions of it in two different places, but no code that uses them. If you want to leave it in, it should probably just be DEBUG_kschwarz (or even DEBUG_kschwarz_off). >+static gfxMatrix GetTransformMatrix(const nsIFrame *const aFrame, >+ const PRInt32 aScaleFactor, >+ const nsRect *const aBoundsOverride) You should definitely document what aScaleFactor is, and probably what aBoundsOverride is. (It's not obvious to me why aScaleFactor is used in some places and not others.) >+#ifdef UNIFIED_CONTINUATIONS It's probably clearer to #ifdef the whole function and just return nsRect(nsPoint(0, 0), aFrame->GetSize()) in the not-UNIFIED_CONTINUATIONS case. In GetDeltaToMozTransformOrigin (and GetResultingTransformMatrix), I wonder whether you'd be better off combining the transform and the transform origin here, since transform origin can be just specified as a pre- and post- transform. Can you reuse other code for multiplying matrices and for handling the percentage transforms? >+ * 1. Check for degenerate cases. I don't see any such checks, and I'm not sure what they would be. It also might be a clearer commenting style to scatter the numbered list through the code that it's describing. This makes it a little less likely that the comments will get out of date (as comments often do). >+/* The transform is opaque iff the skewX and skewY components of the matrix are >+ * both zero and the wrapped list is opaque. Referring to skewX and skewY components confused me a little. The b and c components in | a c e | | b d f | | 0 0 1 | are used to represent both skew and rotation. Maybe it's clearer to say "if the transform represents only scaling and translation". Also, given the way that the overflow rect contains both the transformed and untransformed overflow rects, aren't IsUniform and IsOpaque incorrect for scale transformations that scale smaller? (Or is that not the latest solution for the overflow rect?) In nsDisplayTransform::UntransformRect: >+ if (matrix.IsSingular()) >+ return nsRect(0, 0, 0, 0); maybe you just want "return nsRect()", to distinguish that you really want empty, not 0,0,0,0, which is what we currently use to represent empty? (But you don't want the same for nsPoint, where the default constructor produces unitialized rather than empty. I wonder whether UntransformPoint needs to produce some sort of error result.) Then again, I don't see any callers of nsDisplayTransform::TransformPoint and UntransformPoint. Maybe they should just be removed instead? (If not, why do we need them?) nsDisplayList.h: >+ /* In case someone wants to refcount us, let them support it. */ This comment doesn't make any sense to me. nsLayoutUtils.cpp: For PrintMatrix, see comment about PrintRect above. >- // If it is, or is a descendant of, an SVG foreignobject frame, >- // then we need to do extra work >+ /* If it is, or is a descendant of, an SVG foreignobject frame, >+ * then we need to do extra work. Also, as we're going up the chain >+ * to the root frame, if we find anything that has the -moz-transform >+ * property, we should take note of this for later. >+ */ >+ /* Crawl up the frame tree to find the root frame. If at any point we encounter >+ * a transformed element, we need to mark that, since we'll end up inverting the >+ * transform at the end. >+ */ I'm not sure you need to be this verbose. Perhaps just: // We need to do extra work if the frame or one of its ancestors is // an SVG foreignobject frame or has a transform. >+ /* These are translation matrices from world-to-origin of relative frame and >+ * vice-versa. Although I could use the gfxMatrix::Translate function to accomplish >+ * this, I suspect that this is faster since it doesn't involve any matrix multiplication >+ * behind the scenes. >+ */ This comment seems false, given that you do two matrix multiplications a few lines below. Is "basis" a technical term here, or should ChangeMatrixBasis be called something more like "CombineOriginIntoTransform"? (And I think I asked for the creation of a function much like this one in my comments above.) Somebody should look at the new functions in nsLayoutUtils more closely, but I'm hoping roc will. nsPresShell.cpp: You should not have a blank line between the declarations of WillPaint and InvalidateFrameForView -- this makes it clear that InvalidateFrameForView is an nsIViewObserver method. InvalidateFrameForView could use nsLayoutUtils::GetFrameFor (which does the cast). nsFrame.cpp: PrintRect again (see above). >+ if(!newList) >+ return NS_ERROR_OUT_OF_MEMORY; Space between "if" and "(". >+ /* Make the transform, fail if we can't. */ >+ nsDisplayTransform *transform = new (aBuilder) nsDisplayTransform(this, newList); >+ if (!transform) >+ return NS_ERROR_OUT_OF_MEMORY; In theory, you should delete newList before this early return. >+ /* The image is composited if it's partially transparent or if there's a >+ * transform applied to it (since the rotation might have us drawing over >+ * whatever's below us. >+ */ I'm not sure I'd use the word "image" here. I haven't reviewed the tests. However, I'd name the directory "transform" rather than "moz-transform", since the former makes more sense in the long run and is sensible now. layout/style/Makefile.in: >+ nsTransformFunction.h\ Stick a space before the backslash like other lines. >+ -I$(srcdir)/../../gfx/thebes/public \ Why is this needed? (I'd note that the transform function names don't need to be nsCSSKeywords, but what you did looks OK and I suppose doesn't hurt anything.) nsCSSParser.cpp: In ParseProperty, could you put your new cases after the text_shadow case, and move the box_shadow case up before clip? In ParseSingleValueProperty, could you put your new cases after text_shadow (and not inside the MOZ_SVG ifdef)? >+PRBool CSSParserImpl::ParseFunctionInternals(PRInt32 aVariantMask, nsTArray<nsCSSValue> &aOutput, nsresult &aErrorCode) Could you wrap at under 80 characters? >+{ >+ /* Keep looping until we get something interesting. */ This comment doesn't add anything useful; remove it. >+ while(PR_TRUE) { Space between while and "(". >+ /* Try to read a comma. If we can, then all's well. Otherwise, we need >+ * to see if we just read a closing parenthesis. >+ */ >+ if (!ExpectSymbol(aErrorCode, ',', PR_TRUE)) { >+ /* Push the token back, then try reading a ')'. If we can, >+ * then we're done reading. >+ */ >+ if (ExpectSymbol(aErrorCode, ')', PR_TRUE)) >+ return PR_TRUE; >+ >+ /* Otherwise, very bad things happened. */ >+ return PR_FALSE; >+ } I don't think the comments here add much, except perhaps for having "// parse error" at the end of the "return PR_FALSE" line. The "Push the token back" comment is wrong (since ExpectSymbol already did that for you). >+ * On error, the return value is nsnull. You should say this includes parse errors (for which aErrorCode is untouched) and allocation failures (aErrorCode set to NS_ERROR_OUT_OF_MEMORY). However, I think it would be better to have an nsCSSValue& out parameter (rather than using heap allocation) and make the return value PRBool, like much of the rest of the parser. (Especially given that all the caller does with it is assign another CSS value the value *newCell.) (Alternatively, it could take an nsRefPtr<nsCSSValue::Array>&.) >+ * @param aAllowedTypes The types of CSS values that are allowed to be in this function. >+ * Reading an element that's not of the allowed type will result in the function >+ * returning nsnull. >+ * @param aMinElems Minimum number of elements to read. Reading fewer than this many >+ * elements will result in the function returning nsnull. >+ * @param aMaxElems Maximum number of elements to read. Reading more than this many Wrap this at less than 80 characters. >+ const arrlen_t MAX_ALLOWED_ELEMS = 0xFFFE; // 2^16 - 2, so that if we have 2^16 - 2 transforms >+ // plus the name, we have exactly 2^16 - 1 elements. Wrap this at less than 80 characters. (It doesn't need to be to the right of the code.) >+ const nsString functionName = aFunction; It might be better to use the constructor rather than operator=. >+ /* Now, convert this nsTArray into an nsCSSValue::Array object. We'll need N + 1 spots, >+ * one for the function name and the rest for the arguments. In case the user has given >+ * us more than 2^16 - 2 transform functions, we'll truncate them at 2^16 - 2 functions. >+ */ This is about the number of arguments, not the number of transform functions. >+ const PRUint16 numTransforms = (foundValues.Length() <= MAX_ALLOWED_ELEMS ? >+ foundValues.Length() + 1 : MAX_ALLOWED_ELEMS); You should probably call |numTransforms| |numElements|, since ParseFunction isn't really transform-specific, and it's really one more than the number of arguments. Also, it's probably clearer to do: PRUint16 numElements = foundValues.Length() + 1; if (numElements > MAX_ALLOWED_ELEMENTS) numElements = MAX_ALLOWED_ELEMENTS; >+static PRBool IsInvalidMatrix(const nsCSSValue &aValue) It's probably generally easier to reason about code if you call this function IsValidMatrix and reverse the return values and the handling in the callers. It also seems like it would be clearer to just write for (PRUint16 index = 0; index < 4; ++index) { // check for number } for (PRUint16 index = 4; index < 6; ++index) { // check for length/percent } I think parsing all 6 values of the matrix function with the same variant mask is a mistake. I think it's better to make the data structure obey stricter invariants -- and thus I think you should either extend ParseFunction to take an array of variant masks (of length aMaxElems) or not use ParseFunction to parse matrix values. Then you don't have to worry about the zero-lengths issue here (and probably in other places). >+ /* Here's how this is going to work: >+ * 1. Read a token in from the stream. This SHOULD be a transform function. >+ * 2. If this isn't a transform function, report an error. >+ * 3. Otherwise, based on the transform function, read in the correct >+ * data (including type and number). >+ * 4. If unable to do so, fail. >+ * 5. Chain the final data object on to the end of the list. >+ * 6. Report success! It worked! >+ */ I'd skip the introductory comment. >+ /* First, read in a token and see if we found a transform function. >+ * If we can't read in anything, then we've hit a serious problem. >+ */ s/serious problem/end of file/. But, actually, I'd just skip the comment. >+ /* Check to make sure that we've read a function. Identifiers work too >+ * since the only difference is the placement of the parenthesis. >+ */ >+ if (mToken.mType != eCSSToken_Function && mToken.mType != eCSSToken_Ident) { >+ UngetToken(); >+ return PR_FALSE; >+ } I don't think identifiers should work too; CSS mandates that there's no space between the name of the function and its opening parenthesis. Does WebKit do otherwise? If so, we should probably file a WebKit bug. >+ case eCSSKeyword_translatex: >+ newCell = ParseFunction(mToken.mIdent, VARIANT_LENGTH | VARIANT_PERCENT, >+ static_cast<PRUint16>(1), static_cast<PRUint16>(1), aErrorCode); >+ break; Throughout, I'd just use "1U" rather than static_cast<PRUint16>(1), and just rely on constant folding to reduce the unsigned int to an unsigned short. This will also fix the fact that the lines are wrapped at longer than 80 characters. >+ /* Finally, chain it to the end of the list. */ >+ if (!list) >+ list = toAppend.forget(); >+ else { >+ /* Traverse down the list until we hit the last cell. */ >+ // TODO: This is inefficient. Rewrite it. >+ nsCSSValueList *curr = list; >+ for(; curr->mNext != nsnull; curr = curr->mNext) >+ ; >+ curr->mNext = toAppend.forget(); >+ } You've got a literal tab on the second line, which you shouldn't. But you can simplify this to: nsCSSValueList **tail = &list; while (*tail) tail = &(*tail)->mNext; *tail = toAppend.forget(); and get rid of the if/else. However (to address your "TODO"), it's probably better to modify the caller and change the function parameter to being the list tail (probably nsCSSValueList** rather than nsCSSValueList*&), so that it's O(N) rather than O(N^2). Your comment at the start of HasMoreTransforms that it doesn't modify the stream isn't quite true, since it does eat up whitespace. However, I didn't review the rest of HasMoreTransforms since I think you should just remove the entire function, and replace its callsite with a call to !ExpectEndProperty(aErrorCode) -- and then remove the call to ExpectEndProperty a few lines below. (Though really you should probably refactor ExpectEndProperty so that all but the error reporting are in a sub-function called CheckEndProperty, and then make your caller, and the callers in ParseBorderColors and ParseContent and ParseMarks and ParseDasharray, the second caller in ParseCounterData, and the first caller in ParseQuotes , use CheckEndProperty instead.) You should remove the function HandleKeywordMozTransform and replace the call to it (and the operationSucceeded variable) with a call to ParseVariant, with the mask set to VARIANT_INHERIT | VARIANT_NONE. (That also gets rid of your next TAB literal.) Once you've removed operationSucceeded (which is currently written once and checked twice) and HasMoreTransforms, you should also be able to remove autoCleanupTransformList, since there will be only one early return, and you can just do a manual delete there. ParseMozTransformOrigin can also be simplified a lot by using ParseVariant. However, I think you can basically get rid of the whole thing by slightly refactoring the split between ParseBackgroundPosition and ParseBackgroundPositionValues (so that the nsCSSValuePair to store into is passed in) so that you can reuse ParseBackgroundPositionValues. In turn, -moz-transform-origin should be stored in nsCSSDisplay as a value pair rather than as a value list, with appropriate changes to the code in nsRuleNode.cpp, nsCSSPropList.h, and nsCSSStruct. nsComputedDOMStyle.cpp: You should be able to simplify GetMozTransformOrigin a lot by using SetValueToCoord. You'd need to write a (width, height) pair of percentage-base getters that use GetNetFrameBounds. >+ if(!valueList) Missing space. Also a bunch of 80th-column violations in this function. >+/* If we're dealing with a keyword transform, hands it back "as-is." Otherwise, computes >+ * the aggregate transform matrix and hands it back in a "matrix" wrapper. >+ */ Not sure what you mean by a "keyword transform", but you should really say that it returns either 'none' or a 'matrix()' function. Also, this comment goes past the 80th column, as do some others in this function. >+ resultString.Append(NS_LITERAL_STRING("px")); >+ >+ /* Tack on the closing ) character. */ >+ resultString.Append(')'); No need for these to be two separate appends. nsRuleNode.h: >+ // Expose this so nsTransformFunctions can use it. >+ static nscoord CalcLength(const nsCSSValue& aValue, >+ nsStyleContext* aStyleContext, >+ nsPresContext* aPresContext, >+ PRBool& aInherited); The last three lines should line up with the parameter on the line before. (jumping to review nsStyleStruct before nsRuleNode) nsStyleStruct.h: nsStyleCoord is a union type; every use in nsStyleStruct.h is labeled with the union types allowed for that variable. You should do this for mTransformOrigin, which I think should say "coord, percent". However, for mTransform, I don't think you should be using nsStyleCoord at all. The first four elements are always floats; the last two are always coords. Therefore, you should replace nsStyleCoord mTransform[6] with gfxFloat mTransformFactors[4] and nscoord mTransformCoords[2], or something like that. This will require updating all the places that use mTransform, but I don't think it should take too much time. However, if you want to do it in a followup patch, that's ok. However, probably even better would be to group all four of these arrays (mTransformFactors, mTransformCoords, mTransformX, mTransformY) in a single struct (perhaps called nsStyleTransformMatrix, although that's a bit long), that would also be used for the storage inside nsTransformFunction (and returned, const, from its getters). You should also have a comment showing a transformation matrix: | a c e | | b d f | | 0 0 1 | and saying how the a..f values are related to the different member variables (i.e., a..d from mTransformFactors, e..f from mTransformCoords, mTransformX, mTransformY, and the dimensions of the frame). nsStyleStruct.cpp: >+ if (mTransformPresent != aOther.mTransformPresent) >+ NS_UpdateHint(hint, nsChangeHint_ReconstructFrame); Looks like a TAB snuck in. nsRuleNode.cpp: > static nscoord CalcLength(const nsCSSValue& aValue, > nsStyleContext* aStyleContext, > nsPresContext* aPresContext, > PRBool& aInherited) This existing function should now also be marked as inline. In ReadTransforms, I think you should assign aList->mValue to a temporary (const nsCSSValue&) for the 4 places you use it. And I think it's probably better to use a separate variable for the list iteration rather than changing aList. >+ const PRInt32 NUM_ENTRIES = 6; >+ const PRInt32 FACTOR_THRESHOLD = 4; >+ const PRInt32 NUM_DELTA_ENTRIES = 2; >+ for (PRInt32 index = 0; index < NUM_ENTRIES; ++index) { >+ /* Clear the X and Y percent matrices. */ >+ if (index < NUM_DELTA_ENTRIES) >+ aDisplay->mTransformX[index] = aDisplay->mTransformY[index] = 0.0f; >+ >+ if (index < FACTOR_THRESHOLD) >+ aDisplay->mTransform[index].SetFactorValue((index == 0 || index == 3) ? 1.0f : 0.0f); >+ else >+ aDisplay->mTransform[index].SetCoordValue(static_cast<nscoord>(0)); >+ } This would probably be simpler as a loop up to 2, and then 4 assignments (1.0f, 0.0f, 0.0f, 1.0f, for the 4 factors). Given the above suggestion about a common transform matrix struct, I'm not sure nsTransformFunctions need to exist as objects with virtual function pointers; the transform matrix struct could have methods for SetToIdentity and then a setter method for each of the functions, and the switch in nsRuleNode could call those methods (which would match the current virtual method) instead of creating an object. But again, that could also be in a followup patch. You should remove your ValueToCoord function and use the existing SetCoord which does the same thing (and more). >+ /////////////////////////////////////////////// >+ // -moz-transform parsing >+ // This isn't parsing. You should instead use a comment like the ones before all the other properties. Same for -moz-transform-origin. >+ if(parentDisplay->mTransformPresent) { missing space before "(" I think it's clearer to set display->mTransformPresent to PR_TRUE at the callsite of ComputeTransformMatrix than inside it. You should change ReadTransforms to take the array as a parameter so you don't have to do array copying. nsStyleStruct.h, again: >+ /* We're positioned if we're absolutely positioned or there's a transform in effect. */ s/absolutely positioned/positioned/, since this returns true for relative as well. Then again, the comment then seems to be stating something that doesn't make much sense, so you probably also want s/We're positioned/Returns true/. nsTransformFunction.cpp: What's the use of the unnamed namespace block? Maybe just mark the functions and constants as static instead? Missing spaces before parentheses in the following lines (spread throughout the file): >+ if(cosTheta >= 0 && cosTheta < kEpsilon) >+ else if(cosTheta < 0 && cosTheta >= -kEpsilon) >+ switch(aValue.GetUnit()) >+ switch(aValue.GetUnit()) >+ for(nsMatrixIndex index = static_cast<nsMatrixIndex>(0); index < NUM_ENTRIES; >+ if(index < FACTOR_CUTOFF) >+ if(aData->Item(static_cast<PRUint16>(index + 1)).GetUnit() == eCSSUnit_Percent) >+ if(index == DELTA_X_POS) >+ if(aData->Item(1).GetUnit() != eCSSUnit_Percent) >+ if(aData->Item(1).GetUnit() != eCSSUnit_Percent) >+ if(dx.GetUnit() == eCSSUnit_Percent) >+ if(dy.GetUnit() == eCSSUnit_Percent) >+ switch(aValue.GetUnit()) >+ { But the opening brace on the same line as the switch, and the close brace lined up with the switch (CSSToRadians, SetToValue, and on ifs and elses in ProcessData, in which some bodies should only get one set of two-space indent rather than two). It seems like SetToValue ought to work only for X_TRANSLATE and Y_TRANSLATE and not have the number case, and the nsScale*Function callers should just call SetFactorValue directly (or just assign the correct value, if you've changed the data types). But once you do that, all the callers are also doing the Percent test and the SetCoordX / SetCoordY calls, so it seems like SetToValue *should* handle that part. (And if X_TRANSLATE is 0 and Y_TRANSLATE is 1, that can be easier.) Can you avoid having to add the known failures in the layout/style/test mochitests by adding a prerequisites line for -moz-transform-origin (probably setting display: block, width: 123px, height: 78px)? nsViewManager.cpp: >+ /* If we're supposed to invalidate the frame on a scroll, don't blt. */ s/blt/blit/ r=dbaron with those comments addressed

(In reply to comment #55) > >- NS_ASSERTION(aState->mItemBuffer.Length() == itemBufferStart, > >+ NS_ASSERTION(aState->mItemBuffer.Length() == PRUint32(itemBufferStart), > > really what probably should happen here is that itemBufferStart > and i are both PRUint32, and the for loop changes to: > for (PRInt32 i = aState->mItemBuffer.Length(); i-- != itemBufferStart; ) { > although that's a bit ugly too. Not sure what roc thinks; he owns this > code. I think the cast is about as good, especially since it's in debug code. > Also, given the way that the overflow rect contains both the transformed > and untransformed overflow rects, aren't IsUniform and IsOpaque > incorrect for scale transformations that scale smaller? (Or is that not > the latest solution for the overflow rect?) I think you're right. > Somebody should look at the new functions in nsLayoutUtils more closely, > but I'm hoping roc will. I've looked at them, some comments above.

Revised to address review comments from roc and dbaron. Integrates better with the style system, pushed a good deal into nsLayoutUtils, unified code paths a bit with SVG foreignObjects.

Update to Potential Patch #3 that allows the patch to be cleanly applied to mozilla-central, following the recent patch to the CSS parser.

Yet another update. This addresses the addition of SVG effects. No other changes.

// See if it's relatively positioned - else if ((NS_STYLE_POSITION_RELATIVE == aDisplay->mPosition) && + else if ((NS_STYLE_POSITION_RELATIVE == aDisplay->mPosition || + aDisplay->HasTransform()) && Comment still needs to be fixed + (newOrigin + toMozOrigin, + disp->mTransform.GetThebesMatrix(bounds, aFactor)); Fits on one line +nsRect nsDisplayTransform::TransformRect(const nsRect &untransformedBounds, +nsRect nsDisplayTransform::UntransformRect(const nsRect &untransformedBounds, + static nsRect TransformRect(const nsRect &untransformedBounds, + static nsRect UntransformRect(const nsRect &untransformedBounds, aUntransformedBounds + const nsRect *const + aBoundsOverride = nsnull); One line >+nsLayoutUtils::RoundGfxRectToAppRect(const gfxRect &aRect, const PRInt32 >aFactor) > >Might as well make aFactor a double, since it will have to be converted anyway. >Same for all these helpers. I still think you should do this. Then just use division by aFactor instead of NSAppUnitsToFloatPixels. + if (IsTransformed()) { + *aOutAncestor = nsLayoutUtils::GetCrossDocParentFrame(this); This line happens whether IsTransformed() is true or not, so hoist it out above the 'if'. + /* Compute the delta to the parent, which we need because we are converting + * coordinates to our parent. + */ + const nsPoint delta = GetOffsetTo(*aOutAncestor); Hmm. I don't think you can get transforms on the root frame (the nsViewportFrame) since there's no way for authors to style it, so I'd add NS_ASSERTION(*aOutAncestor, "root frame can't be transformed") Honestly, I think making local variables and parameters 'const' is a waste of time and since we generally don't do it, I'd like you to take them out. It's generally easy to see where a local variable or a parameter is modified in a function, so most of the time 'const' is just adding stuff that people have to read for no particular benefit. (Pointers and references to const are OK and make a lot more sense since they promise the caller that this function won't modify through the pointer/reference.) + const nsIFrame* ReferenceFrame() const { return mReferenceFrame; } I don't think we need to mess around with const nsDisplayListBuilders. + static const nsIFrame* GetCrossDocParentFrame(const nsIFrame *aFrame, + nsPoint* aCrossDocOffset = nsnull) Or this. I think I mentioned before that const nsIFrames are really not very useful, because you're usually interested in a whole frame tree and const doesn't have any way of extending transitively to the parent or children. + if (GetStyleDisplay()->HasTransform()) { Seems to me that everywhere we do nsIFrame::GetStyleDisplay()->HasTransform(), we should just call nsIFrame::IsTransformed(). Furthermore, IsTransformed should probably check the state bit before checking GetStyleDisplay(). GetTransformMatrix is very nice, thanks! + if(f->IsTransformed()) Space between 'if' and '(' + gfxMatrix worldToOrigin(static_cast<gfxFloat>(1.0), + static_cast<gfxFloat>(0.0), + static_cast<gfxFloat>(0.0), + static_cast<gfxFloat>(1.0), + -aOrigin.x, -aOrigin.y); + gfxMatrix originToWorld(static_cast<gfxFloat>(1.0), + static_cast<gfxFloat>(0.0), + static_cast<gfxFloat>(0.0), + static_cast<gfxFloat>(1.0), + aOrigin.x, aOrigin.y); You can just write 1.0 etc here. I don't think the static_cast is doing anything useful. (And if it was, a constructor cast would look better.) Now I suppose I need to check how you addressed dbaron's comments...

The following comments mostly arose from me looking at David's comments and how you addressed them in the latest patch. Overall I think you did what he intended, although I can't be 100% sure in some cases. IsPositionedIgnoringTransform is actually unused and you should remove it. It would be good if you could write a test that combines CSS transforms with an SVG filter effect on the same element. I think it will work, but the combination is a little tricky. layout/reftests/svg-integration has tests that might help. + for (;;) { + /* Might be transformed; stop iterating. */ + if ((*aOutAncestor)->mState & NS_FRAME_MAY_BE_TRANSFORMED) + break; It might make sense to write this as while (!((*aOutAncestor)->mState & NS_FRAME_MAY_BE_TRANSFORMED) > Also, it's probably clearer to do: > > PRUint16 numElements = foundValues.Length() + 1; > if (numElements > MAX_ALLOWED_ELEMENTS) > numElements = MAX_ALLOWED_ELEMENTS; You still need to fix this. + nsCSSValueList *transformList = nsnull; I'd move this down to just before we declare 'tail', so it's clear this isn't used until then. How does nsCSSDisplay::mTransform not leak? + if (mTransformPresent != aOther.mTransformPresent) + NS_UpdateHint(hint, nsChangeHint_ReconstructFrame); + + /* Otherwise, if we've kept the property lying around and we already had a + * transform, we need to see whether or not we've changed the transform. + * If so, we need to do a reflow and a repaint. The reflow is to recompute + * the overflow rect (which probably changed if the transform changed) + * and to redraw within the bounds of that new overflow rect. + */ + else if(mTransformPresent) { + if (mTransform != aOther.mTransform) + NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_ReflowFrame, + nsChangeHint_RepaintFrame)); + + for (PRUint8 index = 0; index < 2; ++index) + if (mTransformOrigin[index] != aOther.mTransformOrigin[index]) { + NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_ReflowFrame, + nsChangeHint_RepaintFrame)); + break; + } + } Reformat this with braces around the first "if" clause so it's clear the else is associate with the if. > nsStyleCoord is a union type; every use in nsStyleStruct.h is labeled > with the union types allowed for that variable. You should do this > for mTransformOrigin, which I think should say "coord, percent". Seems like you should still do this. +nsStyleTransformMatrix::SetToTransformFunction(const nsCSSValue::Array *const + aData, + nsStyleContext*const aContext, + nsPresContext*const aPresContext) Tabs in here need to be replaced with spaces. Still a few occurrences of "if(" hanging around. + /* This section is for the full property. Uncomment it when you're ready. */ What does this mean? It is uncommented already.

Seems like the remaining issues here are almost entirely cosmetic. One more revision should have us pretty much done. I'd like David to have a chance to check how his comments were addressed, but assuming nothing major crops up there, we should be able to land this. If it doesn't land tomorrow, I can clean up whatever else is needed and land it next week. Thanks!!!!!!!!!!

>>+nsLayoutUtils::RoundGfxRectToAppRect(const gfxRect &aRect, const PRInt32 >>aFactor) >> >>Might as well make aFactor a double, since it will have to be converted anyway. >>Same for all these helpers. > >I still think you should do this. Then just use division by aFactor instead of >NSAppUnitsToFloatPixels. The main reason I pass everything around as a PRInt32 is so that I can use NSFloatPixelsToAppUnits, which has very nice rounding behavior, without running into problems with float <-> int rounding issues. Would you still recommend changing it? >+ /* Compute the delta to the parent, which we need because we are >converting >+ * coordinates to our parent. >+ */ >+ const nsPoint delta = GetOffsetTo(*aOutAncestor); > >Hmm. I don't think you can get transforms on the root frame (the >nsViewportFrame) since there's no way >for authors to style it, so I'd add NS_ASSERTION(*aOutAncestor, "root frame >can't be transformed") I think that it might be more useful to have this just do an early return rather than asserting if we're dealing with the root frame, since otherwise all implementations of GetTransformMatrix need to check to see that the ancestor frame they find isn't the root frame and to adjust it if it is. Any thoughts? >+ if (GetStyleDisplay()->HasTransform()) { > >Seems to me that everywhere we do nsIFrame::GetStyleDisplay()->HasTransform(), >we should just call nsIFrame::IsTransformed(). Furthermore, IsTransformed >should probably check the state bit before checking GetStyleDisplay(). You're definitely right that I should check the frame bit first, so I've updated the code to do that. However, I think that I probably want to keep GetStyleDisplay()->HasTransform and IsTransform() separate, since the former explicitly checks for -moz-transform while the latter also includes SVG foreignObjects. Since the two have different layout and graphical effects, I'd like to keep them separate if possible. Does that sound reasonable? >> Also, it's probably clearer to do: >> >> PRUint16 numElements = foundValues.Length() + 1; >> if (numElements > MAX_ALLOWED_ELEMENTS) >> numElements = MAX_ALLOWED_ELEMENTS; > >You still need to fix this. It seems like making this change could lead to problems if foundValues.Length() was sufficiently large, since storing it in a PRUint16 could lead to overflow errors that would make the check numElements > MAX_ALLOWED_ELEMENTS highly unlikely to occur. It won't lead to crashes, but I think that with long transforms it might act as though the list is getting truncated. Thoughts? >How does nsCSSDisplay::mTransform not leak? I think the style system explicitly invokes the destructor for this object. Adding a delete statement in the dtor causes segfaults and the Mochitest leak-checker doesn't seem to pick anything up. I'll try to get an updated patch posted ASAP.

I just discovered that mixing certain types of SVG effects with CSS transforms leads to graphical glitches. I _think_ the problem has to do with the fact that nsDisplaySVGEffects stores the underlying frame's overflow rect as its bounds. This leads to problems when mixed with transforms, since transformed frames store their bounding rects in the transformed coordinate space, so stacking an nsDisplayTransform and an nsDisplaySVGEffects will cause the transform to be applied twice, leaving us with the wrong bounding rectangle. Does this analysis seem correct? If so, should I leave it for bug 452496 to fix, or should I try to find a workaround?

(In reply to comment #63) > The main reason I pass everything around as a PRInt32 is so that I can use > NSFloatPixelsToAppUnits, which has very nice rounding behavior, without > running into problems with float <-> int rounding issues. Would you still > recommend changing it? Actually I think the best thing to do is to make NSFloatPixelsToAppUnits's aAppUnitsPerPixel parameter be a float. It gets coerced to a float internally anyway. > >+ /* Compute the delta to the parent, which we need because we are > >converting > >+ * coordinates to our parent. > >+ */ > >+ const nsPoint delta = GetOffsetTo(*aOutAncestor); > > > >Hmm. I don't think you can get transforms on the root frame (the > >nsViewportFrame) since there's no way > >for authors to style it, so I'd add NS_ASSERTION(*aOutAncestor, "root frame > >can't be transformed") > > I think that it might be more useful to have this just do an early return > rather than asserting if we're dealing with the root frame, since otherwise all > implementations of GetTransformMatrix need to check to see that the ancestor > frame they find isn't the root frame and to adjust it if it is. Any thoughts? The assertion would only be inside the IsTransformed() case. Since root frames can't be transformed, it should never fire no matter what the caller does. Am I missing something? > >+ if (GetStyleDisplay()->HasTransform()) { > > > >Seems to me that everywhere we do nsIFrame::GetStyleDisplay()->HasTransform(), > >we should just call nsIFrame::IsTransformed(). Furthermore, IsTransformed > >should probably check the state bit before checking GetStyleDisplay(). > > You're definitely right that I should check the frame bit first, so I've > updated the code to do that. However, I think that I probably want to keep > GetStyleDisplay()->HasTransform and IsTransform() separate, since the former > explicitly checks for -moz-transform while the latter also includes SVG > foreignObjects. Since the two have different layout and graphical effects, I'd > like to keep them separate if possible. Does that sound reasonable? Yes, you're absolutely right. > >> Also, it's probably clearer to do: > >> > >> PRUint16 numElements = foundValues.Length() + 1; > >> if (numElements > MAX_ALLOWED_ELEMENTS) > >> numElements = MAX_ALLOWED_ELEMENTS; > > > >You still need to fix this. > > It seems like making this change could lead to problems if foundValues.Length() > was sufficiently large, since storing it in a PRUint16 could lead to overflow > errors that would make the check numElements > MAX_ALLOWED_ELEMENTS highly > unlikely to occur. It won't lead to crashes, but I think that with long > transforms it might act as though the list is getting truncated. Thoughts? Yeah, you're right. Good call. > >How does nsCSSDisplay::mTransform not leak? > > I think the style system explicitly invokes the destructor for this object. > Adding a delete statement in the dtor causes segfaults and the Mochitest > leak-checker doesn't seem to pick anything up. OK.

(In reply to comment #64) > I just discovered that mixing certain types of SVG effects with CSS transforms > leads to graphical glitches. I _think_ the problem has to do with the fact > that nsDisplaySVGEffects stores the underlying frame's overflow rect as its > bounds. This leads to problems when mixed with transforms, since transformed > frames store their bounding rects in the transformed coordinate space, so > stacking an nsDisplayTransform and an nsDisplaySVGEffects will cause the > transform to be applied twice, leaving us with the wrong bounding rectangle. OK. Just leave it for now, but file a bug on that specific issue. I'll work on that myself.

Updates to address more review comments. Another step in the asymptotic progression towards perfection!

+nsIFrame::IsTransformed() const +{ + return GetStyleDisplay()->HasTransform(); I actually want to check the state bit here. + static nsRect TransformRect(const nsRect &untransformedBounds, + static nsRect UntransformRect(const nsRect &untransformedBounds, These still need to be fixed to aUntransformedBounds. Other than that everything looks good! I'll make those changes myself and try landing the patch.

Pushed b827e694565d!

There were test failures on Linux: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1221300057.1221304481.21478.gz REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2slave/trunk_linux-3/build/layout/reftests/transform/percent-1d.html | REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2slave/trunk_linux-3/build/layout/reftests/transform/percent-1e.html | REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2slave/trunk_linux-3/build/layout/reftests/transform/percent-1f.html | These look like subtle rasterization differences. and on Windows: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1221299663.1221305971.24645.gz REFTEST TEST-UNEXPECTED-FAIL | file:///D:/builds/slave/trunk_win2k3i-01/build/layout/reftests/transform/translatex-1b.html | REFTEST TEST-UNEXPECTED-FAIL | file:///D:/builds/slave/trunk_win2k3i-01/build/layout/reftests/transform/translatey-1b.html | REFTEST TEST-UNEXPECTED-FAIL | file:///D:/builds/slave/trunk_win2k3i-01/build/layout/reftests/transform/translate-1b.html | These are off by one pixel for some reason. Since these are platform dependent, and minor, and not regressions, we just marked the tests random for now. Bug 455138 tracks those.

The translate*.html failures also happened on Mac (but not the percent-*.html failures). http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1221301010.1221313878.32760.gz

I notice you left two unused variables: PRInt32 cssPxWidth = 0, cssPxHeight = 0; in nsComputedDOMStyle.cpp. I'm not sure if you intended to use them or if they can be taken out.

I've noticed a change in the behavior of attachment 328422 [details]. The change happened between the 2008-09-20 and 2008-09-21 nightly. http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-20+02%3A05%3A51&enddate=2008-09-21+02%3A03%3A52 My guess is that it's caused by bug 455403, but there was some other transform checkins that day so I'm not sure. My question is, is it an intended change or not.

The change in the behavior is due to bug 455403. The testcase was written when the bug had not yet been fixed, so it would have appeared to have the correct behavior. With the fix checked in, the test case no longer works as initially expected, but still operates as it should.

/tests/layout/base/tests/test_bug435293-interaction.html, /tests/layout/base/tests/test_bug435293-scale.html and /tests/layout/base/tests/test_bug435293-skew.html are all passing on all OSs: Firefox 9.0 (Beta 2): https://tbpl.mozilla.org/php/getParsedLog.php?id=7438809&full=1&branch=mozilla-beta https://tbpl.mozilla.org/php/getParsedLog.php?id=7438682&full=1&branch=mozilla-beta https://tbpl.mozilla.org/php/getParsedLog.php?id=7444774&full=1&branch=mozilla-beta https://tbpl.mozilla.org/php/getParsedLog.php?id=7444772&full=1&branch=mozilla-beta Firefox 10.0 (Aurora): https://tbpl.mozilla.org/php/getParsedLog.php?id=7442540&full=1&branch=mozilla-aurora https://tbpl.mozilla.org/php/getParsedLog.php?id=7440940&full=1&branch=mozilla-aurora https://tbpl.mozilla.org/php/getParsedLog.php?id=7443889&full=1&branch=mozilla-aurora https://tbpl.mozilla.org/php/getParsedLog.php?id=7443791&full=1&branch=mozilla-aurora Firefox 11.0 (Nightly): https://tbpl.mozilla.org/php/getParsedLog.php?id=7408736&full=1&branch=services-central https://tbpl.mozilla.org/php/getParsedLog.php?id=7406146&full=1&branch=services-central https://tbpl.mozilla.org/php/getParsedLog.php?id=7407021&full=1&branch=services-central https://tbpl.mozilla.org/php/getParsedLog.php?id=7407009&full=1&branch=services-central