Last Comment Bug 732610 - Make nsIFrame::ComputeSize take a bitfield |aFlags| instead of |bool aShrinkWrap|
: Make nsIFrame::ComputeSize take a bitfield |aFlags| instead of |bool aShrinkW...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 1096260
Blocks: css3-flexbox
  Show dependency treegraph
 
Reported: 2012-03-02 15:37 PST by Daniel Holbert [:dholbert]
Modified: 2014-11-10 04:45 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (31.74 KB, patch)
2012-03-02 15:40 PST, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-03-02 15:37:52 PST
At the beginning of the flexbox algorithm, we need to get the computed size (possibly the shrinkwrapped size) of each child-frames, *without* taking min/max width/height into consideration.[1]

We can't easily do this right now, though. nsIFrame::ComputeSize is the obvious method to use, but it applies min/max constraints to the returned values.

THEREFORE: I'd like to allow nsIFrame::ComputeSize to optionally ignore min/max.  To do that, I'd like to replace the final |bool aShrinkWrap| ComputeSize argument with a "flags" bitfield (with eShrinkWrap being the first flag in that field).

[1] The flexbox algorithm starts with these sizes, _then_ assigns extra space based on flexibility, and _then_ clamps based on min/max constraints.  If we apply min/max constraints too early, it'll produce different, incorrect behavior, since there will be a different amount of available space.
Comment 1 Daniel Holbert [:dholbert] 2012-03-02 15:40:10 PST
Created attachment 602516 [details] [diff] [review]
fix v1

Not sure this is complete/correct, but it compiles.  Will request review after a bit more sanity-checking.
Comment 2 Mozilla RelEng Bot 2012-03-02 17:38:56 PST
Autoland Patchset:
	Patches: 602516
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=65c185a8ba68
Try run started, revision 65c185a8ba68. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=65c185a8ba68
Comment 3 Mozilla RelEng Bot 2012-03-02 19:31:14 PST
Try run for 65c185a8ba68 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=65c185a8ba68
Results (out of 6 total builds):
    success: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-65c185a8ba68
Comment 4 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-03-05 15:00:29 PST
Comment on attachment 602516 [details] [diff] [review]
fix v1

r=dbaron

I wonder if you should make the same conversion apply to ComputeAutoSize, though.

We should definitely use MOZ_OVERRIDE more aggressively in layout.
Comment 5 Daniel Holbert [:dholbert] 2012-03-05 15:43:08 PST
(In reply to David Baron [:dbaron] from comment #4)
> I wonder if you should make the same conversion apply to ComputeAutoSize,
> though.

Possibly, yeah.

I didn't do that yet because the one flag that I'm interested in adding (for "ignore min/max width/height properties") isn't needed in ComputeAutoSize.  (That method already ignores min/max constraints.)

It might be good for consistency & future-proofing, though.  Let me know if you'd like me to do that as part of this bug; otherwise I'll hold off on it for now.

> We should definitely use MOZ_OVERRIDE more aggressively in layout.

Agreed.  I've just filed bug 733186 on that.
Comment 6 Daniel Holbert [:dholbert] 2012-03-16 11:07:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd17c20d8e9
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-17 17:07:01 PDT
https://hg.mozilla.org/mozilla-central/rev/cfd17c20d8e9

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