332922 - better way of dealing with computed padding and margin

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

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

We have lots of bugs scattered throughout the tree related to percentage margin and (especially) padding, since we only have the amount that we compute percentage margins and padding to around during reflow.  We should fix this by putting the computed margin/padding in a frame property when percentages are involved.  This would work roughly as follows:

 * at the point in reflow where we set a frame's rect, we use a function (called, e.g., PlaceFrame) that:
   + sets the rect
   + checks if the frame has percentage margin / percentage padding
   + clears the padding/margin frame properties if they're unneeded and sets them if they're needed

Then we can add APIs to nsIFrame (all of which would assert that neither the frame nor one of its ancestors has NS_FRAME_IS_DIRTY set and that the frame does not have NS_FRAME_HAS_DIRTY_CHILDREN set -- such an assertion makes sense post-reflow-branch):

nsMargin GetComputedMargin() const;
nsMargin GetComputedBorder() const;  /* for symmetry */
nsMargin GetComputedPadding() const;
nsRect GetMarginRect() const;
nsRect GetPaddingRect() const;
nsRect GetBorderRect() const;

and fix tons of these bugs more easily.

How does that sound?

Comment 1

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

Or, then again, maybe CalcPaddingFor, etc., aren't so bad, and we should just give them a cleaner API.  With this approach I'd propose removing them.  But either way I'd want the API on nsIFrame to make dealing with this stuff easier.

Comment 2

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

I'm also uncomfortable with two pieces of code, the stuff in nsHTMLReflowState and CalcPaddingFor, etc., that do the same thing, since they can easily become inconsistent.

Comment 3

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

I want those nsIFrame APIs. I don't particularly want CalcPaddingFor, and I don't want duplication.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Removing Calc*For sounds great.  We should have this code in one and only one place, and an nsIFrame api sounds like exactly what we want.

Comment 5

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

I need to remember to make these APIs call GetSkipSides() as well.

Comment 6

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

(In reply to comment #0)
> nsRect GetBorderRect() const;

Oops, I meant GetContentRect here.

Comment 7

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

(In reply to comment #0)
> nsMargin GetComputedMargin() const;
> nsMargin GetComputedBorder() const;  /* for symmetry */
> nsMargin GetComputedPadding() const;

For consistency with the CSS spec, these should probably be GetUsedMargin, GetUsedBorder, and GetUsedPadding.  And I think I'm going to need to implement at least this part of it on the reflow branch to help with bug 350456.

Comment 8

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

And given the messiness of collapsing margins, I may try to skip GetUsedMargin /  GetMarginRect entirely.

Comment 9

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

Attachment: patch 1

I'm going to land this shortly, but it might be good if roc reviewed it at some point...

Comment 10

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

er, land it *on the reflow branch* shortly

Comment 11

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

Comment on attachment 247768 [details] [diff] [review]
patch 1

Er, except I forgot to write the table part of it...

Comment 12

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

Attachment: patch 1

For real this time -- I'm planning to land this on the reflow branch shortly, but it would probably be good if roc reviewed it at some point.

Comment 13

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

Actually, I think I'm going to back this out of the reflow branch and fix bug 350456 a different (and simpler) way.  But I'm still interested in getting review on this and landing it after the reflow branch.

Comment 14

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

Comment on attachment 247892 [details] [diff] [review]
patch 1

@@ -339,26 +339,27 @@ nsHTMLImageElement::GetWidthHeight()
+    nsMargin margin(0,0,0,0);
+    margin += frame->GetUsedPadding();
+    margin += frame->GetUsedBorder();

nsMargin margin = frame->GetUsedPadding() + frame->GetUsedBorder();
?

@@ -208,25 +208,26 @@ nsDisplayCheckMark::Paint(nsDisplayListB
@@ -196,27 +196,28 @@ static PRBool
@@ -2243,25 +2243,26 @@ nsPrintEngine::ReflowPrintObject(nsPrint
Same sort of thing.

I think we should have a GetUsedBorderPadding helper on nsIFrame (or nsLayoutUtils) that calls the two methods and returns the sum, and use it in the above situations. We could also use it here:
@@ -2725,31 +2725,27 @@ nsComputedDOMStyle::GetHeight(nsIFrame *
@@ -2787,30 +2783,26 @@ nsComputedDOMStyle::GetWidth(nsIFrame *a
@@ -3186,28 +3178,26 @@ nsComputedDOMStyle::GetRelativeOffset(PR

+  nsMargin margin;
+  if (!GetStyleMargin()->GetMargin(margin)) {
+    nsMargin *m = NS_STATIC_CAST(nsMargin*,
+                    GetProperty(nsLayoutAtoms::usedMarginProperty));
+    NS_ASSERTION(m, "used margin property missing (out of memory?)");
+    if (m) {
+      margin = *m;
+    }
+  }
+  return margin;

This returns someting uninitialized if the assertion fails. I'd prefer it to be initialized to zero.

r+sr with that...

Comment 15

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

Attachment: patch 1

This should address roc's comments.

I called the method on nsIFrame GetUsedBorderAndPadding.  (BorderPadding has been known to confuse people; I hope this patch eliminates the term from the tree.  nsBox already uses BorderAndPadding.)  GetUsedBorderAndPadding is currently inline, although I'm not sure whether that's really the best idea.

Comment 16

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

Attachment: leak fix for 1.8 branch

This is a leak fix from within the patch that I should probably get into the 1.8 branch.

Comment 17

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

attachment 248561 [details] [diff] [review] checked in to trunk.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 248562 [details] [diff] [review]
leak fix for 1.8 branch

Yikes.  Looks good!

Comment 19

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

Comment on attachment 248562 [details] [diff] [review]
leak fix for 1.8 branch

Info about this approval request:  this is a simple leak fix that was part of the larger patch above.  Many of the functions in this file start off with this pattern of creating an nsROCSSPrimitiveValue object, in order to return it.  However, this is a helper function that shouldn't return one, but the pattern was mistakenly copied to it as well, so it creates an object and then forgets about it.  This patch fixes that.

Comment 20

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

Except for the fact that attachment 248561 [details] [diff] [review] added two words to every frame vtable, it actually improved codesize by a little over a hundred bytes, however, the vtable penalty was slightly over 1K, for a net increase of 1K.

Comment 21

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

I'm thinking maybe comment 5 was wrong, and GetSkipSides should be separate.  (But we might want a utility function to apply GetSkipSides to an nsMargin.)

I also still need to write GetMarginRect, GetPaddingRect, and GetContentRect.

Comment 22

[Ria Klaassen (not reading all bugmail)]

Firefox crashes on Google Toolbar now, and the regression range seems to point to this bug.
TB27307312H 

Comment 23

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

Oops, I forgot to rev nsIFrame's IID, but I just did.

If Google Toolbar depends on nsIFrame, then they're broken, and you should complain to them, not us.

Comment 24

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

(In reply to comment #22)
> Firefox crashes on Google Toolbar now, and the regression range seems to point
> to this bug.
> TB27307312H 

2006121304 is before this patch was checked in.  If you were using a build from afterwards with some other build's talkback component, then the talkback data aren't useful.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

TB shows two new topcrashers:
nsComputedDOMStyle::GetMarginWidthCoordFor
nsComputedDOMStyle::GetPaddingWidthCoordFor

Unfortunately atm TB server doesn't give me any stack traces.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

nsComputedDOMStyle::GetPaddingWidthCoordFor  [mozilla\layout\style\nscomputeddomstyle.cpp, line 3319]
nsComputedDOMStyle::GetPaddingWidthFor  [mozilla\layout\style\nscomputeddomstyle.cpp, line 3311]
nsComputedDOMStyle::GetPropertyValue  [mozilla\layout\style\nscomputeddomstyle.cpp, line 283]
NS_InvokeByIndex  [mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2162]

Comment 27

[Martijn Wargers (dead)]

(In reply to comment #25)
> TB shows two new topcrashers:
> nsComputedDOMStyle::GetMarginWidthCoordFor
> nsComputedDOMStyle::GetPaddingWidthCoordFor

This was filed as bug 363950.

Comment 28

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #23)
> Oops, I forgot to rev nsIFrame's IID, but I just did.
> 
> If Google Toolbar depends on nsIFrame, then they're broken, and you should
> complain to them, not us.
> 
I was not complaining.

Comment 29

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

Attachment: patch 2

This adds GetMarginRect, GetPaddingRect, GetContentRect, and ApplySkipSides to nsIFrame, and removes nsStyleBorderPadding.

I'm wondering how useful these really are, though.  Some callers would be better off if GetPaddingRect and GetContentRect were in frame coordinates rather than parent coordinates, and many of the callers where they are usable care only about the size.

Comment 30

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

Comment on attachment 249162 [details] [diff] [review]
patch 2

I think this is fine. Computing size only would not save much. I agree that it's better to be consistent with GetRect() than to return frame-relative coordinates.

Comment 31

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

Patch 2 checked in to trunk.

Marking this bug as fixed at this point; new improvements should go in new bugs.

Comment 32

[Jay Patel [:jay]]

Comment on attachment 248562 [details] [diff] [review]
leak fix for 1.8 branch

Approved for 1.8 branch, a=jay for drivers.

Comment 33

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

Comment on attachment 248562 [details] [diff] [review]
leak fix for 1.8 branch

Checked in to MOZILLA_1_8_BRANCH, 2006-12-29 14:43 -0800.

Comment 34

[Johnny Stenback (:jst)]

Adding fixed1.8.1.2 keyword based on the previous comment.

Comment 35

[Tracy Walker [:tracy]]

verified per checkins