Integrate the linear constraint solver into Sky as a RenderBox subclass.

Integrate the linear constraint solver into Sky as a RenderBox subclass.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/fff7b0ae784f577e8e53fb27a48a26dde02d58e1

[Chinmay, 2015-07-08 21:40:24]

chinmaygarde@google.com changed reviewers:
+ abarth@chromium.org, eseidel@chromium.org, ianh@google.com

[Chinmay, 2015-07-08 21:40:24]

Uses the Cassowary dart package to implement auto layout in Sky

[abarth-chromium, 2015-07-08 22:22:02]

lgtm

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/example/rendering/s...
File sky/sdk/example/rendering/simple_autolayout.dart (right):

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/example/rendering/s...
sky/sdk/example/rendering/simple_autolayout.dart:10: import
'package:cassowary/cassowary.dart' as AL;
I think the usual dart convention is to use lower case for these names.

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
File sky/sdk/lib/rendering/auto_layout.dart (right):

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:7: import
'package:cassowary/cassowary.dart' as AL;
ditto

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:25: this.position = new
Point(_leftEdge.value, _topEdge.value);
No need for |this.|

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:58: AL.Result res =
_solver.addConstraints(constraints);
s/res/result/

Please use complete words in variable names.

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:103: }
You shouldn't need to override the size setter.  The contract for RenderBox is
that the size property is set only by that RenderBox and only during layout.

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:108: size = constraints.smallest;
Typically we default to constraints.biggest because constraints.smallest often
is 0x0.

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:157: abstract class _AutoLayoutParamMixin
{
I would move this up in the file given that it's referenced early on.

[Chinmay, 2015-07-08 23:01:01]

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/example/rendering/s...
File sky/sdk/example/rendering/simple_autolayout.dart (right):

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/example/rendering/s...
sky/sdk/example/rendering/simple_autolayout.dart:10: import
'package:cassowary/cassowary.dart' as AL;
On 2015/07/08 at 22:22:01, abarth-chromium wrote:
> I think the usual dart convention is to use lower case for these names.

ack

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
File sky/sdk/lib/rendering/auto_layout.dart (right):

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:7: import
'package:cassowary/cassowary.dart' as AL;
On 2015/07/08 at 22:22:02, abarth-chromium wrote:
> ditto

ack

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:25: this.position = new
Point(_leftEdge.value, _topEdge.value);
On 2015/07/08 at 22:22:01, abarth-chromium wrote:
> No need for |this.|

ack

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:58: AL.Result res =
_solver.addConstraints(constraints);
On 2015/07/08 at 22:22:01, abarth-chromium wrote:
> s/res/result/
> 
> Please use complete words in variable names.

ack

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:103: }
On 2015/07/08 at 22:22:02, abarth-chromium wrote:
> You shouldn't need to override the size setter.  The contract for RenderBox is
that the size property is set only by that RenderBox and only during layout.

ack. Good to know.

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:108: size = constraints.smallest;
On 2015/07/08 at 22:22:01, abarth-chromium wrote:
> Typically we default to constraints.biggest because constraints.smallest often
is 0x0.

ack

https://codereview.chromium.org/1230583003/diff/1/sky/sdk/lib/rendering/auto_...
sky/sdk/lib/rendering/auto_layout.dart:157: abstract class _AutoLayoutParamMixin
{
On 2015/07/08 at 22:22:01, abarth-chromium wrote:
> I would move this up in the file given that it's referenced early on.

ack. I was trying to keep the internal classes towards the end of the file.
Though ideally, I should have just moved these to another file.

[abarth-chromium, 2015-07-08 23:08:35]

LGTM

The only other nit I have with this CL is the reference from the parent data to
the render box.  Ideally we wouldn't have that pointer but I understand why
you've got it.

[Chinmay, 2015-07-08 23:11:39]

Committed patchset #2 (id:20001) manually as
fff7b0ae784f577e8e53fb27a48a26dde02d58e1 (presubmit successful).

[Hixie, 2015-07-09 00:53:15]

lgtm

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/example/renderi...
File sky/sdk/example/rendering/simple_autolayout.dart (right):

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/example/renderi...
sky/sdk/example/rendering/simple_autolayout.dart:13: var c1 = new
RenderDecoratedBox(
don't use 'var'

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
File sky/sdk/lib/rendering/auto_layout.dart (right):

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:15: // initialized before the
constructor. Not sure how to do that using a Mixin
Why can't you just use a constructor?

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:90: _setupLayoutParameters(this);
put constructors before fields

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:93: @override
remove the @override annotations (or add them across the entire codebase ;-) ).

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:94: void
_applyAutolayoutParameterUpdates() {
Put a comment here explaining that this is called by the parent, to lay out the
child, etc.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:113: }
you have a blank line after the class open {, so you should have one before the
class close }.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:123: RenderAutoLayout({List<RenderBox>
children}) {
put spaces inside { }s around parameter arguments.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:126: 
remove this blank line; it doesn't help readability and makes the constructor's
shape harder to see.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:139: 
probably don't need the blank lines here either.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:170: child.parentData = new
AutoLayoutParentData(child);
no { }s around blocks that are only 1 line long

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:177: size = constraints.biggest;
If the size isn't configurable, then you should set sizedByParent and use
performResize to set the size, rather than performLayout.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:198: void hitTestChildren(HitTestResult
result, {Point position}) =>
Don't use fat arrow if the body isn't on the same line as the declaration.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:216: // Call super last as this collects
parent data
it doesn't matter, dropChild doesn't touch parentData.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:222: // Only edits are present on layout
containers
What does this mean?

[Chinmay, 2015-07-09 21:44:21]

The following reviews address the concerns raised:
https://codereview.chromium.org/1230033002/
https://codereview.chromium.org/1226113007/

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/example/renderi...
File sky/sdk/example/rendering/simple_autolayout.dart (right):

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/example/renderi...
sky/sdk/example/rendering/simple_autolayout.dart:13: var c1 = new
RenderDecoratedBox(
On 2015/07/09 at 00:53:14, Hixie wrote:
> don't use 'var'

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
File sky/sdk/lib/rendering/auto_layout.dart (right):

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:90: _setupLayoutParameters(this);
On 2015/07/09 at 00:53:15, Hixie wrote:
> put constructors before fields

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:93: @override
On 2015/07/09 at 00:53:14, Hixie wrote:
> remove the @override annotations (or add them across the entire codebase ;-)
).

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:94: void
_applyAutolayoutParameterUpdates() {
On 2015/07/09 at 00:53:15, Hixie wrote:
> Put a comment here explaining that this is called by the parent, to lay out
the child, etc.

ack. Since this is part of the abstract class that is used as the mixin, I am
documenting it there.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:113: }
On 2015/07/09 at 00:53:14, Hixie wrote:
> you have a blank line after the class open {, so you should have one before
the class close }.

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:123: RenderAutoLayout({List<RenderBox>
children}) {
On 2015/07/09 at 00:53:14, Hixie wrote:
> put spaces inside { }s around parameter arguments.

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:126: 
On 2015/07/09 at 00:53:14, Hixie wrote:
> remove this blank line; it doesn't help readability and makes the
constructor's shape harder to see.

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:139: 
On 2015/07/09 at 00:53:14, Hixie wrote:
> probably don't need the blank lines here either.

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:170: child.parentData = new
AutoLayoutParentData(child);
On 2015/07/09 at 00:53:14, Hixie wrote:
> no { }s around blocks that are only 1 line long

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:177: size = constraints.biggest;
On 2015/07/09 at 00:53:14, Hixie wrote:
> If the size isn't configurable, then you should set sizedByParent and use
performResize to set the size, rather than performLayout.

This is something I am not completely sure of TBH. In an earlier patch, I had an
override of the size setter and did the edit variable update there. But Adam
indicated that these are never configurable. Need to catch up on this.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:198: void hitTestChildren(HitTestResult
result, {Point position}) =>
On 2015/07/09 at 00:53:14, Hixie wrote:
> Don't use fat arrow if the body isn't on the same line as the declaration.

I have been referring to the expression count (instead of the line count) for
the fat arrow syntax. Do you have strong opinions on this? I prefer not to use
the line count as this will eventually depend on what the formatter does to the
code.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:216: // Call super last as this collects
parent data
On 2015/07/09 at 00:53:14, Hixie wrote:
> it doesn't matter, dropChild doesn't touch parentData.

ack

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:222: // Only edits are present on layout
containers
On 2015/07/09 at 00:53:14, Hixie wrote:
> What does this mean?

ack. Elaborated

[Hixie, 2015-07-09 22:05:09]

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
File sky/sdk/lib/rendering/auto_layout.dart (right):

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:15: // initialized before the
constructor. Not sure how to do that using a Mixin
i think you missed this one. :-)

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:177: size = constraints.biggest;
As far as I can tell, you don't let the size be set based on the kids currently.
So if that's the case, then you should set sizedByParent.

https://codereview.chromium.org/1230583003/diff/20001/sky/sdk/lib/rendering/a...
sky/sdk/lib/rendering/auto_layout.dart:198: void hitTestChildren(HitTestResult
result, {Point position}) =>
our style guide says => is for single-line declarations. If it splits across
lines then it's no better than a block, and consistency with other multiline
constructs says we should just use blocks then.