per bug 363706 comment 11, nsLayoutUtils::GetAbsoluteCoord is not necessary after the removal of eStyleUnit_Chars. This patch removes it and makes some simplifications to callers that then become possible. Note that I assumed various classes' mRenderingContext fields were still used. GetAbsoluteCoord survives as a static function in nsLayoutUtils.cpp, because there were a bunch of places in that file that did something like if (GetAbsoluteCoord(..., coord) || GetOtherKindOfCoord(..., coord) { do stuff with coord } so removing it altogether would have meant I needed to duplicate the "do stuff" bit.

Comment on attachment 323807 [details] [diff] [review] patch to remove GetAbsoluteCoord >+static PRBool GetAbsoluteCoord(const nsStyleCoord& aStyle, nscoord& aResult) >+{ >+ if (eStyleUnit_Coord == aStyle.GetUnit()) { >+ aResult = aStyle.GetCoordValue(); >+ return PR_TRUE; >+ } else >+ return PR_FALSE; else-after-return is considered unnecessary in Mozilla (thanks to Brendan's influence). I'd also put the shorter part in the if, making it: if (aStyle.GetUnit() != eStyleUnit_Coord) { return PR_FALSE; } aResult = aStyle.GetCoordValue(); return PR_TRUE; >diff --git a/layout/generic/nsColumnSetFrame.cpp b/layout/generic> static nscoord > GetColumnGap(nsColumnSetFrame* aFrame, >- const nsStyleColumn* aColStyle, >- nsIRenderingContext* aRenderingContext) >+ const nsStyleColumn* aColStyle) > { > nscoord colGap; > if (eStyleUnit_Normal == aColStyle->mColumnGap.GetUnit()) > return aFrame->GetStyleFont()->mFont.size; >- else if (nsLayoutUtils::GetAbsoluteCoord(aColStyle->mColumnGap, >- aRenderingContext, >- aFrame, colGap)) { >+ else if (eStyleUnit_Coord == aColStyle->mColumnGap.GetUnit()) { >+ colGap = aColStyle->mColumnGap.GetCoordValue(); You can move the declaration of colGap up to its first use. >diff --git a/layout/generic/nsHTMLReflowState.cpp b/layout/generic/nsHTMLReflowState.cpp >+ padding.left = nsLayoutUtils::ComputeWidthDependentValue(aContainingBlockWidth, >+ mStylePadding->mPadding.GetLeft()); >+ padding.right = nsLayoutUtils::ComputeWidthDependentValue(aContainingBlockWidth, >+ mStylePadding->mPadding.GetRight()); Find some way to wrap this (and a few more below it) at < 80 characters. > ComputeLineHeight(nsIRenderingContext* aRenderingContext, >+ return lhCoord.GetCoordValue(); >+ } else if (lhCoord.GetUnit() == eStyleUnit_Factor) { Replace the else with a newline. >+ // For factor units the computed value of the line-height property >+ // is found by multiplying the factor by the font's computed size >+ // (adjusted for min-size prefs and text zoom). >+ return NSToCoordRound(lhCoord.GetFactorValue() * >+ aStyleContext->GetStyleFont()->mFont.size); >+ } else { And just unindent this else (no else here either). r+sr=dbaron with that This can't land until after bug 363706.

Here's a revised patch which I believe addresses all of your concerns. It also corrects an error which caused the reftest for bug 391979 to fail; the patch in that bug introduced some tricky logic that I didn't follow correctly during the refactor. I have reread this entire patch carefully and I believe there are no other such bugs. Also, while doing the rearrangement of ComputeLineHeight that you requested, I noticed that that function no longer needed a rendering context, so I took that out and adjusted its callers.

Comment on attachment 325935 [details] [diff] [review] revised patch r+sr=dbaron

Though, I'm curious -- where exactly was the bug causing the reftest to fail?

Basically, in the first iteration, I dropped the negation in this conditional: nscoord colWidth; - if (!nsLayoutUtils::GetAbsoluteCoord(colStyle->mColumnWidth, - aRenderingContext, this, colWidth)) { + if (colStyle->mColumnWidth.GetUnit() == eStyleUnit_Coord) { + colWidth = colStyle->mColumnWidth.GetCoordValue(); if (mFrames.FirstChild()) { colWidth = mFrames.FirstChild()->GetPrefWidth(aRenderingContext); which meant that you got the "auto" behavior if you set the column width to an absolute dimension, and it didn't work at all if you set it to "auto".

I guess this can be landed now...?

Done: http://hg.mozilla.org/mozilla-central/index.cgi/rev/d4fa059ca178