Last Comment Bug 777519 - nsHTMLReflowState::InitResizeFlags unnecessarily checks for min-height:auto and max-height:auto
: nsHTMLReflowState::InitResizeFlags unnecessarily checks for min-height:auto a...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 585715
Blocks: 763689
  Show dependency treegraph
 
Reported: 2012-07-25 14:55 PDT by Daniel Holbert [:dholbert]
Modified: 2012-07-28 18:35 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (1.20 KB, patch)
2012-07-25 15:04 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix v1a (1.58 KB, patch)
2012-07-25 15:22 PDT, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-07-25 14:55:55 PDT
In bug 763689, I'm adding support for the new min-width / min-height: auto keyword. Before that bug's patches, we don't support "auto" for these properties.

However, nsHTMLReflowState::InitResizeFlags currently has this check:
> 533   bool dependsOnCBHeight =
[...]
> 537     (mStylePosition->MinHeightDependsOnContainer() &&
> 538      // FIXME: condition this on not-abspos?
> 539      mStylePosition->mMinHeight.GetUnit() != eStyleUnit_Auto) ||
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#533
from this cset:
http://hg.mozilla.org/mozilla-central/rev/fcc2aa4bd451#l6.11

That "mMinHeight.GetUnit() != eStyleUnit_Auto" check is trivially true right now.

Moreover, after bug 763689 lets us have "auto" in computed-style, we explicitly *won't* want the check for 'auto' here. (since the used value of "min-height:auto" is either 0 or the item's intrinsic height -- it never depends on the container's height).

So, I think we want to just drop this line.
Comment 1 Daniel Holbert [:dholbert] 2012-07-25 14:58:56 PDT
(In reply to Daniel Holbert [:dholbert] from comment #0)
> That "mMinHeight.GetUnit() != eStyleUnit_Auto" check is trivially true right
> now.

That's true of the mMaxHeight check in the same chunk of code, too, actually.

max-height doesn't accept "auto", as shown here:
  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h#2064
Comment 2 Daniel Holbert [:dholbert] 2012-07-25 15:04:35 PDT
Created attachment 645896 [details] [diff] [review]
fix v1
Comment 3 Daniel Holbert [:dholbert] 2012-07-25 15:22:34 PDT
Created attachment 645904 [details] [diff] [review]
fix v1a

(reposting patch with 8 lines of context, instead of 3, to make it clearer)
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-27 15:33:21 PDT
Comment on attachment 645904 [details] [diff] [review]
fix v1a

r=dbaron
Comment 5 Daniel Holbert [:dholbert] 2012-07-27 15:54:25 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/83a4dfef9a7d
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-07-28 18:35:27 PDT
https://hg.mozilla.org/mozilla-central/rev/83a4dfef9a7d

Note You need to log in before you can comment on or make changes to this bug.