437335 - remove nsLayoutUtils::GetAbsoluteCoord

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, enhancement)

Description

[Zack Weinberg (:zwol)]

Attachment: patch to remove GetAbsoluteCoord

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 1

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

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.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: revised patch

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 3

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

Comment on attachment 325935 [details] [diff] [review]
revised patch

r+sr=dbaron

Comment 4

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

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

Comment 5

[Zack Weinberg (:zwol)]

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".

Comment 6

[Zack Weinberg (:zwol)]

I guess this can be landed now...?

Comment 7

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

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