Closed Bug 437335 Opened 12 years ago Closed 12 years ago

remove nsLayoutUtils::GetAbsoluteCoord

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: zwol, Assigned: zwol)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch to remove GetAbsoluteCoord (obsolete) — Splinter Review
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.
Attachment #323807 - Flags: review?(dbaron)
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.
Attachment #323807 - Flags: superreview+
Attachment #323807 - Flags: review?(dbaron)
Attachment #323807 - Flags: review+
Attached patch revised patchSplinter Review
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.
Attachment #323807 - Attachment is obsolete: true
Attachment #325935 - Flags: superreview?(dbaron)
Attachment #325935 - Flags: review?(dbaron)
Comment on attachment 325935 [details] [diff] [review]
revised patch

r+sr=dbaron
Attachment #325935 - Flags: superreview?(dbaron)
Attachment #325935 - Flags: superreview+
Attachment #325935 - Flags: review?(dbaron)
Attachment #325935 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
You need to log in before you can comment on or make changes to this bug.