Last Comment Bug 774835 - Refactor ApplyMinMaxConstraints in nsBlockFrame
: Refactor ApplyMinMaxConstraints in nsBlockFrame
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: fantasai
:
Mentors:
Depends on:
Blocks: 743402
  Show dependency treegraph
 
Reported: 2012-07-17 13:06 PDT by Scott Johnson (:jwir3)
Modified: 2012-07-19 07:36 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.45 KB, patch)
2012-07-17 13:51 PDT, fantasai
roc: review+
Details | Diff | Review
patch with review comments addressed (8.47 KB, patch)
2012-07-18 07:04 PDT, fantasai
no flags Details | Diff | Review

Description Scott Johnson (:jwir3) 2012-07-17 13:06:33 PDT
ApplyMinMaxConstraints in nsBlockFrame applies computed min/max height and width. We want to do these together in only one place. Instead, we should refactor this to be two functions: ApplyMinMaxHeight and ApplyMinMaxWidth.
Comment 1 fantasai 2012-07-17 13:51:36 PDT
Created attachment 643127 [details] [diff] [review]
patch
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-17 20:14:58 PDT
Comment on attachment 643127 [details] [diff] [review]
patch

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

::: layout/generic/nsHTMLReflowState.h
@@ +418,4 @@
>     */
> +  nscoord ApplyMinMaxWidth(nscoord aWidth) const {
> +    if (NS_UNCONSTRAINEDSIZE != mComputedMaxWidth)
> +        aWidth = NS_MIN(aWidth, mComputedMaxWidth);

{}

@@ +426,5 @@
> +   * size computed so far.
> +   */
> +  nscoord ApplyMinMaxHeight(nscoord aHeight) const {
> +    if (NS_UNCONSTRAINEDSIZE != mComputedMaxHeight)
> +        aHeight = NS_MIN(aHeight, mComputedMaxHeight);

{}
Comment 3 fantasai 2012-07-18 06:35:05 PDT
I left out the extra braces to keep it extra compact since it's inlined in the header. Are you sure you want them in?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-18 06:40:30 PDT
Yes. Keeping it compact doesn't matter.
Comment 5 fantasai 2012-07-18 07:04:43 PDT
Created attachment 643362 [details] [diff] [review]
patch with review comments addressed
Comment 6 Scott Johnson (:jwir3) 2012-07-18 07:12:32 PDT
fantasai, if you add a commit message to your patch, I'll check it in for you.
Comment 7 Scott Johnson (:jwir3) 2012-07-18 07:30:00 PDT
Nevermind, I've added it and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e038e82f416
Comment 8 Ed Morley [:emorley] 2012-07-19 07:36:03 PDT
https://hg.mozilla.org/mozilla-central/rev/1e038e82f416

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