366531 - deCOMtaminate nsBox.h [BoundsCheck()]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andreas Lange]

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

Comment 1

[Jared Moore]

I'm starting some work on this as my first foray into Mozilla contribution. Here goes nothing. :)

Comment 2

[Jared Moore]

Attachment: First attempt

I've done as suggested.

Also I'm renamed BoundsCheck to BoundsClamp in an attempt to be more descriptive.

Comment 3

[Andreas Lange]

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!

Comment 4

[Jared Moore]

No problem, I'll fix that when I get the opportunity.

Comment 5

[Lorenzo Dieryckx]

Jared, 
are you still working on this?

Comment 6

[Jared Moore]

No, not at the moment. :)

Comment 7

[Lorenzo Dieryckx]

Is it ok for you if I give this a shot?
or do you intend to finish it at a later time?

Comment 8

[Jared Moore]

Yeah, go for it! Feel free to reuse my work or start on your own, whatever you like. Have fun! :)

Comment 9

[Lorenzo Dieryckx]

Attachment: BoundsCheck, BoundsCheckMinMax and methods that use these methods

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.

Comment 10

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

I don't think Jan Varga has been active lately; you should probably just request review from [e-mail].

Comment 11

[Lorenzo Dieryckx]

Comment on attachment 294427 [details] [diff] [review]
BoundsCheck, BoundsCheckMinMax and methods that use these methods

changed review request

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  return nsSize(aMaxSize.width < aMinSize.width ? aMinSize.width : aMaxSize.width,
+                aMaxSize.height < aMinSize.height ? aMinSize.height : aMaxSize.height);

Use PR_MAX here. Otherwise great!

Comment 13

[Lorenzo Dieryckx]

Attachment: Adjustments proposed by :roc

Replaced x < y ? y : x by PR_MAX(x, y)

Comment 14

[Reed Loden [:reed]]

Needs approval first before the checkin-needed keyword can be added.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

roc: can we get a risk/reward statement for this? 

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The risk and reward are both pretty small. We can defer until we've branched FF3 off.

Comment 17

[Reed Loden [:reed]]

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

Comment 18

[[e-mail]]

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.

Comment 19

[Lorenzo Dieryckx]

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

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we just fix nsGridRowLeafLayout to do a couple of PR_MAXs of its own? That should be simple enough.

Comment 21

[Reed Loden [:reed]]

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? :)

Comment 22

[Lorenzo Dieryckx]

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.

Comment 23

[Lorenzo Dieryckx]

Attachment: additional fixes

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

Comment 24

[Reed Loden [:reed]]

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