Closed Bug 366531 Opened 13 years ago Closed 12 years ago

deCOMtaminate nsBox.h [BoundsCheck()]

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: anlan, Assigned: ldx)

References

()

Details

Attachments

(3 files, 1 obsolete file)

From bug 243370 comment 29 and 33 by roc:

> IMHO Andreas should fix this by deCOMtaminating BoundsCheck.
> void BoundsCheck(nsSize&, nsSize&, nsSize&) should become
> nsSize BoundsCheck(const nsSize&, const nsSize&, const nsSize&)
> void BoundsCheck(nscoord&, nscoord&, nscoord&) should become
> nscoord BoundsCheck(nscoord, nscoord, nscoord).

> Hardly any of the callers need the updated max; the ones that do should call
> BoundsCheckMinMax instead (which itself should be fixed to be "nsSize
> BoundsCheckMinMax(const nsSize&, const nsSize&)".
I'm starting some work on this as my first foray into Mozilla contribution. Here goes nothing. :)
Attached patch First attemptSplinter Review
I've done as suggested.

Also I'm renamed BoundsCheck to BoundsClamp in an attempt to be more descriptive.
The next step is to request review, have a look at the activity log for other bugs (why not parent bug 243370) to see the process.

Note that I have no weight what so ever as reviewer, but here are a few things you should fix first to not waste the reviewer's time:
- A proper diff, use something like "cvs diff -utp8"
- http://www.mozilla.org/hacking/mozilla-style-guide.html, no else after return
- rv is used as in "nsresult rv", don't use it for anything else

Nice of you to pitch in! Keep on going!
No problem, I'll fix that when I get the opportunity.
Jared, 
are you still working on this?
No, not at the moment. :)
Is it ok for you if I give this a shot?
or do you intend to finish it at a later time?
Yeah, go for it! Feel free to reuse my work or start on your own, whatever you like. Have fun! :)

First attempt for this bug.
I changed the signature and implementation of
both BoundsCheck methods, and of the BoundsCheckMinMax methods.

I then adjusted all code that uses these methods accordingly.
Attachment #294427 - Flags: review?(Jan.Varga)
I don't think Jan Varga has been active lately; you should probably just request review from roc@ocallahan.org.
Comment on attachment 294427 [details] [diff] [review]
BoundsCheck, BoundsCheckMinMax and methods that use these methods

changed review request
Attachment #294427 - Flags: review?(Jan.Varga) → review?(roc)
+  return nsSize(aMaxSize.width < aMinSize.width ? aMinSize.width : aMaxSize.width,
+                aMaxSize.height < aMinSize.height ? aMinSize.height : aMaxSize.height);

Use PR_MAX here. Otherwise great!
Replaced x < y ? y : x by PR_MAX(x, y)
Attachment #294427 - Attachment is obsolete: true
Attachment #294955 - Flags: review?(roc)
Attachment #294427 - Flags: review?(roc)
Assignee: nobody → ldx
Attachment #294955 - Flags: approval1.9?
Status: NEW → ASSIGNED
Keywords: checkin-needed
Needs approval first before the checkin-needed keyword can be added.
Keywords: checkin-needed
roc: can we get a risk/reward statement for this? 
The risk and reward are both pretty small. We can defer until we've branched FF3 off.
Attachment #294955 - Flags: approval1.9? → approval1.9+
Checking in layout/xul/base/src/nsBox.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBox.cpp,v  <--  nsBox.cpp
new revision: 1.134; previous revision: 1.133
done
Checking in layout/xul/base/src/nsBox.h;
/cvsroot/mozilla/layout/xul/base/src/nsBox.h,v  <--  nsBox.h
new revision: 1.41; previous revision: 1.40
done
Checking in layout/xul/base/src/nsBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v  <--  nsBoxFrame.cpp
new revision: 1.346; previous revision: 1.345
done
Checking in layout/xul/base/src/nsImageBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsImageBoxFrame.cpp,v  <--  nsImageBoxFrame.cpp
new revision: 1.131; previous revision: 1.130
done
Checking in layout/xul/base/src/nsLeafBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsLeafBoxFrame.cpp,v  <--  nsLeafBoxFrame.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in layout/xul/base/src/nsMenuFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuFrame.cpp,v  <--  nsMenuFrame.cpp
new revision: 1.367; previous revision: 1.366
done
Checking in layout/xul/base/src/nsPopupSetFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp,v  <--  nsPopupSetFrame.cpp
new revision: 1.174; previous revision: 1.173
done
Checking in layout/xul/base/src/nsSplitterFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsSplitterFrame.cpp,v  <--  nsSplitterFrame.cpp
new revision: 1.179; previous revision: 1.178
done
Checking in layout/xul/base/src/nsSprocketLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,v  <--  nsSprocketLayout.cpp
new revision: 1.64; previous revision: 1.63
done
Checking in layout/xul/base/src/nsStackLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsStackLayout.cpp,v  <--  nsStackLayout.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in layout/xul/base/src/grid/nsGrid.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGrid.cpp,v  <--  nsGrid.cpp
new revision: 1.33; previous revision: 1.32
done
Checking in layout/xul/base/src/grid/nsGridRowLeafLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp,v  <--  nsGridRowLeafLayout.cpp
new revision: 1.22; previous revision: 1.21
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Comment on attachment 294955 [details] [diff] [review]
Adjustments proposed by :roc

>         nsSize minSize = child->GetMinSize(aState);
>         nsSize maxSize = child->GetMaxSize(aState);
>-        nsBox::BoundsCheckMinMax(minSize, maxSize);
>+        maxSize = nsBox::BoundsCheckMinMax(minSize, maxSize);
Nit: below, you use
nsSize maxSize = nsBox::BoundsCheckMinMax(minSize, child->GetMaxSize(aState));

>-     nsBox::BoundsCheck(min, pref, max);
>+     pref = nsBox::BoundsCheck(min, pref, max);
>    
>      current = new (aState) nsBoxSize();
>      current->pref = pref;
>      current->min = min;
>      current->max = max;
The old BoundsCheck used to modify max. I looked at the code in nsSplitterFrame and it doesn't use max if pref < min but I'm not so sure about the code in nsGridRowLeafLayout.
Yes,
It looks like you are right.
Sorry about that.

Unfortunately, there is currently no BoundsCheckMinMax method available with nscoord parameters.

What should I do? create a new method
nscoord 
BoundsCheckMinMax(nscoord min, nscoord max)
{
  return PR_MAX(min, max);
}

Or is there a better solution for that?
for example, just inlining the above method in nsGridRowLeafLayout,
since that's the only place in which this would be used?

Please advise
...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we just fix nsGridRowLeafLayout to do a couple of PR_MAXs of its own? That should be simple enough.
Is the problem mentioned in comment #18 bad enough to warrant backing out this entire patch until a new fix can be completed?

On a secondary note, Lorenzo, could you come up with a fix soon, please? :)
Sorry Reed, but I've been busy with just about everything but mozilla development this week. 

I'll do my very best to come up with a fix this weekend.
Attached patch additional fixesSplinter Review
Fixed two occurrences in which nsBox::BoundsCheck with nscoord arguments was used, and where the max parameter was used afterwards.

fixed by just adding PR_MAX(min, max) before the call to BoundsCheck
Attachment #297947 - Flags: review?(roc)
Depends on: 412323
Attachment #297947 - Flags: superreview?(roc)
Attachment #297947 - Flags: superreview?(roc)
Attachment #297947 - Flags: superreview+
Attachment #297947 - Flags: review?(roc)
Attachment #297947 - Flags: review+
Attachment #297947 - Flags: approval1.9+
Checking in layout/xul/base/src/nsSprocketLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,v  <--  nsSprocketLayout.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in layout/xul/base/src/grid/nsGridRowLeafLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp,v  <--  nsGridRowLeafLayout.cpp
new revision: 1.24; previous revision: 1.23
done
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.