Extend Sky's RenderFlex with intrinsic sizes and compute cross-size height

Extend Sky's RenderFlex with intrinsic sizes and compute cross-size height

Renders interactive_flex and stocks demos fine. Could use more testing,
so I'm going to work on that after I land this.

R=abarth@chromium.org, abarth

Committed: https://chromium.googlesource.com/external/mojo/+/6d1905dc4ca59780bedc8029347882ae6a935905

[abarth-chromium, 2015-06-10 02:05:53]

https://codereview.chromium.org/1173493003/diff/40001/sky/examples/lib/solid_...
File sky/examples/lib/solid_color_box.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/examples/lib/solid_...
sky/examples/lib/solid_color_box.dart:21: return
constraints.constrainWidth(0.0);
Why this change?

[abarth-chromium, 2015-06-10 04:23:20]

lgtm

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/box.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:598: return
_sizeForConstraints(constraints).width;
.height

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/flex.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:30: typedef double
ChildSizingFunction(RenderBox child, BoxConstraints constraints);
_ChildSizingFunction

The _ scopes the definition to this file.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:152: inflexibleSpace += mainSize;
Is this line missing from the other case?

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:155: inflexibleSpace += mainSize;
Wait, it seems like we're doing this twice in the vertical case.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:175: crossSize =
child.getMaxIntrinsicHeight(childConstraints);
It's odd that we use getMaxIntrinsicHeight here but getMinIntrinsicHeight in the
inflexible case.  I understand that it shouldn't matter...  Maybe we should use
getMaxIntrinsicHeight for consistency and then assert that getMinIntrinsicHeight
gives the same answer.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:184: maxCrossSize =
math.max(maxCrossSize, crossSize);
Again, we seem to have the problem with this line being duplicated above (on
line 176).  In this case, it doesn't matter for correctness, but should probably
be fixed anyway.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:202: FlexDirection.horizontal,
Shouldn't we use the flex direction of the container?

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:210: FlexDirection.horizontal,
ditto

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:218: FlexDirection.vertical,
ditto

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:226: FlexDirection.vertical,
ditto

[jackson, 2015-06-10 04:33:39]

https://codereview.chromium.org/1173493003/diff/40001/sky/examples/lib/solid_...
File sky/examples/lib/solid_color_box.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/examples/lib/solid_...
sky/examples/lib/solid_color_box.dart:21: return
constraints.constrainWidth(0.0);
On 2015/06/10 02:05:53, abarth wrote:
> Why this change?

It seemed like most users of RenderSolidColorBox were setting desiredSize to
Size.infinite, but clearly they didn't need infinite space to display their
contents. Probably what we should do is special case infinity and use zero
instead. I can do that.

[jackson, 2015-06-10 04:44:46]

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/box.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/box.dart:598: return
_sizeForConstraints(constraints).width;
On 2015/06/10 04:23:19, abarth wrote:
> .height

Acknowledged.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/flex.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:30: typedef double
ChildSizingFunction(RenderBox child, BoxConstraints constraints);
On 2015/06/10 04:23:20, abarth wrote:
> _ChildSizingFunction
> 
> The _ scopes the definition to this file.

Acknowledged.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:152: inflexibleSpace += mainSize;
On 2015/06/10 04:23:20, abarth wrote:
> Is this line missing from the other case?

Acknowledged.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:155: inflexibleSpace += mainSize;
On 2015/06/10 04:23:20, abarth wrote:
> Wait, it seems like we're doing this twice in the vertical case.

Acknowledged.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:175: crossSize =
child.getMaxIntrinsicHeight(childConstraints);
On 2015/06/10 04:23:20, abarth wrote:
> It's odd that we use getMaxIntrinsicHeight here but getMinIntrinsicHeight in
the
> inflexible case.  I understand that it shouldn't matter...  Maybe we should
use
> getMaxIntrinsicHeight for consistency and then assert that
getMinIntrinsicHeight
> gives the same answer.

Acknowledged.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:184: maxCrossSize =
math.max(maxCrossSize, crossSize);
On 2015/06/10 04:23:20, abarth wrote:
> Again, we seem to have the problem with this line being duplicated above (on
> line 176).  In this case, it doesn't matter for correctness, but should
probably
> be fixed anyway.

Acknowledged.

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:202: FlexDirection.horizontal,
On 2015/06/10 04:23:20, abarth wrote:
> Shouldn't we use the flex direction of the container?

This argument (sizingDirection) is compared with the container's direction at
the top of _getIntrinsicSize to figure out whether we're determining the main
size or the cross size. Does that make sense? Maybe it would be less confusing
if rather than passing it as an argument, split _getIntrinsicSize into a main
size and cross size method, and decide which to call in getMinIntrinsicWidth.

[abarth-chromium, 2015-06-10 04:52:07]

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/flex.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:202: FlexDirection.horizontal,
On 2015/06/10 at 04:44:46, jackson wrote:
> On 2015/06/10 04:23:20, abarth wrote:
> > Shouldn't we use the flex direction of the container?
> 
> This argument (sizingDirection) is compared with the container's direction at
the top of _getIntrinsicSize to figure out whether we're determining the main
size or the cross size. Does that make sense? Maybe it would be less confusing
if rather than passing it as an argument, split _getIntrinsicSize into a main
size and cross size method, and decide which to call in getMinIntrinsicWidth.

Ah, I see.  I think it's fine this way.  Another choice would be to use a named
argument so that the call site would be self-documenting.

[jackson, 2015-06-10 05:00:49]

On 2015/06/10 04:52:07, abarth wrote:
>
https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
> File sky/sdk/lib/framework/rendering/flex.dart (right):
> 
>
https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
> sky/sdk/lib/framework/rendering/flex.dart:202: FlexDirection.horizontal,
> On 2015/06/10 at 04:44:46, jackson wrote:
> > On 2015/06/10 04:23:20, abarth wrote:
> > > Shouldn't we use the flex direction of the container?
> > 
> > This argument (sizingDirection) is compared with the container's direction
at
> the top of _getIntrinsicSize to figure out whether we're determining the main
> size or the cross size. Does that make sense? Maybe it would be less confusing
> if rather than passing it as an argument, split _getIntrinsicSize into a main
> size and cross size method, and decide which to call in getMinIntrinsicWidth.
> 
> Ah, I see.  I think it's fine this way.  Another choice would be to use a
named
> argument so that the call site would be self-documenting.

Yeah, I thought about doing that as well. I guess I'll do that.

[kulakowski, 2015-06-10 16:07:14]

kulakowski@chromium.org changed reviewers:
+ kulakowski@chromium.org

[kulakowski, 2015-06-10 16:07:14]

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
File sky/sdk/lib/framework/rendering/flex.dart (right):

https://codereview.chromium.org/1173493003/diff/40001/sky/sdk/lib/framework/r...
sky/sdk/lib/framework/rendering/flex.dart:30: typedef double
ChildSizingFunction(RenderBox child, BoxConstraints constraints);
On 2015/06/10 04:44:46, jackson wrote:
> On 2015/06/10 04:23:20, abarth wrote:
> > _ChildSizingFunction
> > 
> > The _ scopes the definition to this file.
> 
> Acknowledged.

Pedantry hopefully useful in the future: There is only library level visibility
in Dart, not file level.

[abarth-chromium, 2015-06-10 16:11:19]

On 2015/06/10 at 16:07:14, kulakowski wrote:
> Pedantry hopefully useful in the future: There is only library level
visibility in Dart, not file level.

Yeah, sorry.  I was unclear.  In this particular case, this file is its own
library.

[jackson, 2015-06-10 17:12:04]

Committed patchset #6 (id:100001) manually as
6d1905dc4ca59780bedc8029347882ae6a935905 (presubmit successful).