Remove NS_CSS_MINMAX in favour of mozilla::clamped

RESOLVED WONTFIX

Status

()

Core
Layout
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
In the places where we use NS_CSS_MINMAX:

http://mxr.mozilla.org/mozilla-central/ident?i=NS_CSS_MINMAX&filter=

we should use mozilla::clamped (from <http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsAlgorithm.h>) instead.
I'm not so sure; NS_CSS_MINMAX has the advantage that it tells you something about the expected order of the arguments.
(Assignee)

Comment 2

5 years ago
That's a fair point; maybe we should just rename mozilla::clamped first.
Created attachment 633236 [details] [diff] [review]
Patch(v1)
Attachment #633236 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 4

5 years ago
Comment on attachment 633236 [details] [diff] [review]
Patch(v1)

Review of attachment 633236 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing f? until it's clear we want it.
Attachment #633236 - Flags: feedback?(Ms2ger)
(In reply to David Baron [:dbaron] from comment #1)
> I'm not so sure; NS_CSS_MINMAX has the advantage that it tells you something
> about the expected order of the arguments.

It seems to me that the order min then max is common sense, just like x
always comes before y, width always comes before height, etc.

Fwiw, mozilla::clamped does have:
  NS_ABORT_IF_FALSE(max >= min, "clamped(): max must be greater than or equal to min");

(In reply to :Ms2ger from comment #2)
> That's a fair point; maybe we should just rename mozilla::clamped first.

I wonder why we didn't follow our naming convention that method/function names
start with a capital letter.

My 2 cents: leave mozilla::clamped as is, and replace NS_CSS_MINMAX with it
(Assignee)

Comment 6

5 years ago
I don't have a strong opinion about the exact name, but having one function instead of two would be nice

Comment 7

5 years ago
Hello guys,

May I ask what is the final decision about NS_CSS_MINMAX / mozilla::clamped?

Cheers,

Comment 8

5 years ago
I am interested to solve the bug further

Comment 9

5 years ago
Created attachment 703231 [details] [diff] [review]
Initial Patch !
Attachment #703231 - Flags: review?(Ms2ger)
One comment on the patch:
 * we should probably be |using namespace mozilla;| rather than sprinkling mozilla:: everywhere

I guess, given that std::minmax() is something different (returns a pair<> with min and max), we should probably stick with either mozilla::clamp() or mozilla::clamped().  (Where did the -ed come from, anyway?)
(Assignee)

Comment 11

5 years ago
Comment on attachment 703231 [details] [diff] [review]
Initial Patch !

Review of attachment 703231 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is an improvement over what we have now, at least, so I think we should take it.

::: layout/forms/nsHTMLButtonControlFrame.cpp
@@ +232,2 @@
>                                          aReflowState.mComputedMinHeight,
>                                          aReflowState.mComputedMaxHeight);

You'll want to fix the indentation here and below.

::: layout/generic/nsHTMLReflowState.h
@@ +38,5 @@
>      result = aMaxValue;
>    if (aMinValue > result)
>      result = aMinValue;
>    return result;
>  }

Just remove the implementation
Attachment #703231 - Flags: review?(dbaron)
Attachment #703231 - Flags: review?(Ms2ger)
Attachment #703231 - Flags: feedback+
Comment on attachment 703231 [details] [diff] [review]
Initial Patch !

Could you update the patch with the above comments and request review on the new patch?
Attachment #703231 - Flags: review?(dbaron) → review-

Comment 13

5 years ago
sure... i will upload the patch and give you for review.
(In reply to David Baron [:dbaron] from comment #10)
> (Where did the -ed come from, anyway?)

bug 695303 comment 4 and 5
(In reply to Mats Palmgren [:mats] from comment #5)
> Fwiw, mozilla::clamped does have:
>   NS_ABORT_IF_FALSE(max >= min, "clamped(): max must be greater than or
> equal to min");

Er, actually, doesn't that mean this entire patch isn't going to work?  We actually depend on NS_CSS_MINMAX working when max < min, and doing the correct CSS behavior in that case.
Yeah, good point.  I think we should fix the consumers to not rely on that.

In nsLayoutUtils::ComputeHeightDependentValue right about here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#3146
we should add something like what ComputeAutoSizeWithIntrinsicDimensions does:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#3220

I think the consumers that use mComputedMin/MaxHeight should be OK --
nsHTMLReflowState should have taken care of it.

It appears min/max main- and cross-sizes are ordered, but I'm not familiar
enough with nsFlexContainerFrame.cpp to say for sure.

So it appears nsLayoutUtils.cpp#3146 is the one place we need to fix.

Comment 17

5 years ago
Created attachment 703259 [details] [diff] [review]
updated patch !
Attachment #703231 - Attachment is obsolete: true
Attachment #703259 - Flags: review?(dbaron)
Attachment #703259 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 18

5 years ago
Comment on attachment 703259 [details] [diff] [review]
updated patch !

Review of attachment 703259 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFlexContainerFrame.cpp
@@ +1581,2 @@
>                                  aItem.GetCrossMinSize(),
>                                  aItem.GetCrossMaxSize());

Fix the indentation here

@@ +2060,1 @@
>                        minCrossSize, maxCrossSize);

and here

::: layout/generic/nsHTMLReflowState.h
@@ +38,5 @@
>      result = aMaxValue;
>    if (aMinValue > result)
>      result = aMinValue;
>    return result;
>  }

and remove this

::: layout/xul/base/src/nsLeafBoxFrame.cpp
@@ +287,2 @@
>                                        aReflowState.mComputedMinHeight,
>                                        aReflowState.mComputedMaxHeight);

and the indentation again here.
Attachment #703259 - Flags: feedback?(Ms2ger) → feedback+
(In reply to Mats Palmgren [:mats] from comment #16)
> Yeah, good point.  I think we should fix the consumers to not rely on that.

But the entire *point* of NS_CSS_MINMAX was to encapsulate that.

Thus I think this bug should be WONTFIX.
(Assignee)

Comment 20

5 years ago
(In reply to David Baron [:dbaron] from comment #15)
> (In reply to Mats Palmgren [:mats] from comment #5)
> > Fwiw, mozilla::clamped does have:
> >   NS_ABORT_IF_FALSE(max >= min, "clamped(): max must be greater than or
> > equal to min");
> 
> Er, actually, doesn't that mean this entire patch isn't going to work?  We
> actually depend on NS_CSS_MINMAX working when max < min, and doing the
> correct CSS behavior in that case.

How is this not documented anywhere? I don't think it makes sense to have a generic helper function that follows a different contract than what you'd reasonably expect, and hides that fact too. If there's only one caller that actually relies on this behaviour, we should inline the check.
(In reply to Mats Palmgren [:mats] from comment #16)
> I think the consumers that use mComputedMin/MaxHeight should be OK --
> nsHTMLReflowState should have taken care of it.

If mComputedMax{Height,Width} are intended to reflect the computed values of the CSS properties in question, then the code in nsHTMLReflowState that does this adjustment is buggy.

In any case, I'm comfortable with WONTFIXing this bug at this point; CSS has a clear rule for handling min and max values, where min overrides max.  NS_CSS_MINMAX is designed to encapsulate that rule, allowing its input max to be smaller than its input min (in which case the result must be exactly min).  Any replacement for it must have the same behavior.


Sorry for wasting the time of the people preparing patches for this.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX

Comment 22

5 years ago
felt very much sad to see this :( :( :(
(In reply to :Ms2ger from comment #20)
> How is this not documented anywhere? I don't think it makes sense to have a
> generic helper function that follows a different contract than what you'd
> reasonably expect, and hides that fact too. If there's only one caller that
> actually relies on this behaviour, we should inline the check.

It's documented pretty clearly in the relevant CSS specs; while the code is clear, I agree that a comment pointing out that the min-overrides-max behavior is what is required by the CSS specs would be useful.
Attachment #703259 - Flags: review?(dbaron) → review-
The relevant spec citations are:
http://www.w3.org/TR/CSS21/visudet.html#min-max-widths
http://www.w3.org/TR/CSS21/visudet.html#min-max-heights
(Assignee)

Comment 25

5 years ago
Created attachment 703269 [details] [diff] [review]
Add a comment
Attachment #633236 - Attachment is obsolete: true
Attachment #703259 - Attachment is obsolete: true
Attachment #703269 - Flags: review?(dbaron)
Comment on attachment 703269 [details] [diff] [review]
Add a comment

r=dbaron  Thanks.
Attachment #703269 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5400c00877

Comment 28

5 years ago
https://hg.mozilla.org/mozilla-central/rev/bb5400c00877
Assignee: nobody → Ms2ger
You need to log in before you can comment on or make changes to this bug.