Closed Bug 332922 Opened 14 years ago Closed 13 years ago

better way of dealing with computed padding and margin

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: verified1.8.1.2, Whiteboard: [patch])

Attachments

(3 files, 2 obsolete files)

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?
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.
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.
I want those nsIFrame APIs. I don't particularly want CalcPaddingFor, and I don't want duplication.
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.
I need to remember to make these APIs call GetSkipSides() as well.
(In reply to comment #0)
> nsRect GetBorderRect() const;

Oops, I meant GetContentRect here.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
(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.
And given the messiness of collapsing margins, I may try to skip GetUsedMargin /  GetMarginRect entirely.
Attached patch patch 1 (obsolete) — Splinter Review
I'm going to land this shortly, but it might be good if roc reviewed it at some point...
Attachment #247768 - Flags: review?(roc)
er, land it *on the reflow branch* shortly
Comment on attachment 247768 [details] [diff] [review]
patch 1

Er, except I forgot to write the table part of it...
Attachment #247768 - Flags: review?(roc)
Attached patch patch 1 (obsolete) — Splinter Review
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.
Attachment #247768 - Attachment is obsolete: true
Attachment #247892 - Flags: review?(roc)
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.
Whiteboard: [patch]
Attachment #247892 - Attachment description: patch 1 (landing on reflow branch) → patch 1
Attachment #247768 - Attachment description: patch 1 (landing on reflow branch) → patch 1
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...
Attachment #247892 - Flags: superreview+
Attachment #247892 - Flags: review?(roc)
Attachment #247892 - Flags: review+
Attached patch patch 1Splinter Review
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.
Attachment #247892 - Attachment is obsolete: true
This is a leak fix from within the patch that I should probably get into the 1.8 branch.
Attachment #248562 - Flags: superreview?(bzbarsky)
Attachment #248562 - Flags: review?(bzbarsky)
Comment on attachment 248562 [details] [diff] [review]
leak fix for 1.8 branch

Yikes.  Looks good!
Attachment #248562 - Flags: superreview?(bzbarsky)
Attachment #248562 - Flags: superreview+
Attachment #248562 - Flags: review?(bzbarsky)
Attachment #248562 - Flags: review+
Attachment #248562 - Flags: approval1.8.1.2?
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.
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.
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.
Firefox crashes on Google Toolbar now, and the regression range seems to point to this bug.
TB27307312H 
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.
(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.
TB shows two new topcrashers:
nsComputedDOMStyle::GetMarginWidthCoordFor
nsComputedDOMStyle::GetPaddingWidthCoordFor

Unfortunately atm TB server doesn't give me any stack traces.
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]
Depends on: 363950
(In reply to comment #25)
> TB shows two new topcrashers:
> nsComputedDOMStyle::GetMarginWidthCoordFor
> nsComputedDOMStyle::GetPaddingWidthCoordFor

This was filed as bug 363950.
(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.

Attached patch patch 2Splinter Review
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.
Attachment #249162 - Flags: superreview?(roc)
Attachment #249162 - Flags: review?(roc)
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.
Attachment #249162 - Flags: superreview?(roc)
Attachment #249162 - Flags: superreview+
Attachment #249162 - Flags: review?(roc)
Attachment #249162 - Flags: review+
Patch 2 checked in to trunk.

Marking this bug as fixed at this point; new improvements should go in new bugs.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 248562 [details] [diff] [review]
leak fix for 1.8 branch

Approved for 1.8 branch, a=jay for drivers.
Attachment #248562 - Flags: approval1.8.1.2? → approval1.8.1.2+
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.
Depends on: 366021
Depends on: 366952
Blocks: 367458
Adding fixed1.8.1.2 keyword based on the previous comment.
Keywords: fixed1.8.1.2
verified per checkins
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.