Closed
Bug 763399
Opened 12 years ago
Closed 11 years ago
Remove NS_CSS_MINMAX in favour of mozilla::clamped
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: Ms2ger, Assigned: Ms2ger)
References
Details
(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])
Attachments
(1 file, 3 obsolete files)
962 bytes,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
That's a fair point; maybe we should just rename mozilla::clamped first.
Comment 3•12 years ago
|
||
Attachment #633236 -
Flags: feedback?(Ms2ger)
Assignee | ||
Comment 4•12 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)
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
I don't have a strong opinion about the exact name, but having one function instead of two would be nice
Hello guys, May I ask what is the final decision about NS_CSS_MINMAX / mozilla::clamped? Cheers,
Comment 8•11 years ago
|
||
I am interested to solve the bug further
Comment 9•11 years ago
|
||
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•11 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•11 years ago
|
||
sure... i will upload the patch and give you for review.
Comment 14•11 years ago
|
||
(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.
Comment 16•11 years ago
|
||
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•11 years ago
|
||
Attachment #703231 -
Attachment is obsolete: true
Attachment #703259 -
Flags: review?(dbaron)
Attachment #703259 -
Flags: feedback?(Ms2ger)
Assignee | ||
Comment 18•11 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•11 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
Closed: 11 years ago
Resolution: --- → WONTFIX
Comment 22•11 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•11 years ago
|
||
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+
Comment 28•11 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.
Description
•