Make nsIFrame::ComputeSize take a bitfield |aFlags| instead of |bool aShrinkWrap|

RESOLVED FIXED in mozilla14

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
Attachment #602516 - Flags: review?(dbaron)
Whiteboard: [autoland:-p linux -u reftest]
Whiteboard: [autoland:-p linux -u reftest] → [autoland-try:-p linux -u reftest]

Updated

5 years ago
Whiteboard: [autoland-try:-p linux -u reftest] → [autoland-in-queue]

Comment 2

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
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.
Attachment #602516 - Flags: review?(dbaron) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd17c20d8e9
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/cfd17c20d8e9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 1096260
You need to log in before you can comment on or make changes to this bug.