Closed
Bug 437335
Opened 17 years ago
Closed 16 years ago
remove nsLayoutUtils::GetAbsoluteCoord
Categories
(Core :: Layout: Block and Inline, enhancement)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla1.9.1a1
People
(Reporter: zwol, Assigned: zwol)
References
Details
Attachments
(1 file, 1 obsolete file)
42.32 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | 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+
Assignee | ||
Comment 2•16 years ago
|
||
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?
Assignee | ||
Comment 5•16 years ago
|
||
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".
Assignee | ||
Comment 6•16 years ago
|
||
I guess this can be landed now...?
Status: ASSIGNED → RESOLVED
Closed: 16 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.
Description
•