Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1151293002:
WIP Flexbox Layout Manager for Sky framework. (Closed)
DescriptionWIP Flexbox Layout Manager for Sky framework.
This only handles really basic cases and doesn't understand
justification, wrapping, and so on. Submitting for code
review so I can get some early feedback before I invest
more time into making it correct.
R=ianh@google.com, abarth
Committed: https://chromium.googlesource.com/external/mojo/+/9687376ca32408d564be59f37b13b7d5917d9a73
Patch Set 1 : Initial WIP #
Total comments: 18
Patch Set 2 : Implement CR feedback from Adam and Ojan #
Total comments: 14
Patch Set 3 : CR feedback from hixie #
Total comments: 3
Patch Set 4 : more CR feedback from hixie #Patch Set 5 : Rebasing onto master #
Total comments: 2
Messages
Total messages: 19 (7 generated)
https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... File sky/sdk/lib/framework/layout2.dart (right): https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:738: _direction = value; Should early out when the newDirection and _direction match. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:739: relayout(); Should markForLayout instead of calling relayout. The markForLayout function marks this node as dirty and in need of a layout. If you call relayout here, we'll relayout immediately, which could be wasted work. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:755: } layout and relayout don't quite seem right. You might want to talk it over with hixie to see what he has in mind for these functions. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:757: int getFlex(RenderBox child) { s/getFlex/_getFlex/ to mark the function as private to this library. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:775: child.layout(new BoxConstraints(minWidth: width, maxWidth: width), Should the BoxConstraints depend on the flex direction? For example, is the width really this constrained for a row flex direction? https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:776: relayoutSubtreeRoot: relayoutSubtreeRoot); I think you're supposed to call getIntrinsicDimensions here because you just want an estimate of the size of the elements. Again, hixie is the right person to ask. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:793: constraints = new BoxConstraints(maxHeight: height, maxWidth: spaceForChild); Do we want to set the minWidth to spaceForChild as well to force the child to be exactly that wide? https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:796: constraints = new BoxConstraints(maxHeight: spaceForChild, maxWidth: width); Same here for minHeight?
ojan@chromium.org changed reviewers: + ojan@chromium.org
https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... File sky/sdk/lib/framework/layout2.dart (right): https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:792: case FlexDirection.Row: Not directly related to this patch, but I noticed it while looking it over. We should s/Row/Horizontal/ and s/Column/Vertical/. The web has these really confusing names because it's trying to be generic WRT vertical writing modes which we don't have and if we do add them, we'll have Horizontal and Vertical be the logical values instead of the physical ones.
https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... File sky/sdk/lib/framework/layout2.dart (right): https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:738: _direction = value; On 2015/05/22 06:26:23, abarth wrote: > Should early out when the newDirection and _direction match. Acknowledged. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:739: relayout(); On 2015/05/22 06:26:23, abarth wrote: > Should markForLayout instead of calling relayout. The markForLayout function > marks this node as dirty and in need of a layout. If you call relayout here, > we'll relayout immediately, which could be wasted work. Acknowledged. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:755: } On 2015/05/22 06:26:22, abarth wrote: > layout and relayout don't quite seem right. You might want to talk it over with > hixie to see what he has in mind for these functions. Acknowledged. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:757: int getFlex(RenderBox child) { On 2015/05/22 06:26:22, abarth wrote: > s/getFlex/_getFlex/ to mark the function as private to this library. Acknowledged. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:775: child.layout(new BoxConstraints(minWidth: width, maxWidth: width), On 2015/05/22 06:26:23, abarth wrote: > Should the BoxConstraints depend on the flex direction? For example, is the > width really this constrained for a row flex direction? Acknowledged. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:776: relayoutSubtreeRoot: relayoutSubtreeRoot); On 2015/05/22 06:26:23, abarth wrote: > I think you're supposed to call getIntrinsicDimensions here because you just > want an estimate of the size of the elements. Again, hixie is the right person > to ask. I talked to hixie, he said that it's ok to call layout here as long as we're only calling it once since we're going to have to call it once at some point in this method. If we find ourselves calling layout more than once then I'll change it to use getIntrinsicDimensions instead. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:792: case FlexDirection.Row: On 2015/05/22 06:48:11, ojan wrote: > Not directly related to this patch, but I noticed it while looking it over. We > should s/Row/Horizontal/ and s/Column/Vertical/. The web has these really > confusing names because it's trying to be generic WRT vertical writing modes > which we don't have and if we do add them, we'll have Horizontal and Vertical be > the logical values instead of the physical ones. Acknowledged. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:793: constraints = new BoxConstraints(maxHeight: height, maxWidth: spaceForChild); On 2015/05/22 06:26:22, abarth wrote: > Do we want to set the minWidth to spaceForChild as well to force the child to be > exactly that wide? Acknowledged. https://codereview.chromium.org/1151293002/diff/1/sky/sdk/lib/framework/layou... sky/sdk/lib/framework/layout2.dart:796: constraints = new BoxConstraints(maxHeight: spaceForChild, maxWidth: width); On 2015/05/22 06:26:22, abarth wrote: > Same here for minHeight? Acknowledged.
ianh@google.com changed reviewers: + ianh@google.com
https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... File sky/examples/raw/simple_render_tree.dart (right): https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:31: class RenderSolidColorBlock extends RenderSolidColor { Don't need two classes here, just have the one class then put the flex info into the parent data on the outside. https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:51: parentData = new FlexBoxParentData(); children should never create their parent's parentdata. https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:90: root.add(new RenderSolidColorFlex(0xFFFFFF00, 1)); Have a helper function here which creates the node, adds it to root, then sets child.parentData.flex. https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... File sky/sdk/lib/framework/layout2.dart (right): https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:464: bool handlePointer(sky.PointerEvent event, { double x: 0.0, double y: 0.0 }) { This can't be in a class above ContainerRenderNodeMixin in the hierarchy. To reuse code use a helper function. https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:754: width = clamp(min: _constraints.minWidth, max: _constraints.maxWidth); width = _constraints.constrainWidth(0.0); https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:794: double spacePerFlex = freeSpace / totalFlex; check what happens when totalFlex is 0.0 and either way put a comment describing what happens. https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:837: RenderBox child = _firstChild; maybe do the same as for handlePointer() (with a helper function used by BlockBox as well)
https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... File sky/examples/raw/simple_render_tree.dart (right): https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:31: class RenderSolidColorBlock extends RenderSolidColor { On 2015/05/22 20:32:16, Hixie wrote: > Don't need two classes here, just have the one class then put the flex info into > the parent data on the outside. Acknowledged. https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:51: parentData = new FlexBoxParentData(); On 2015/05/22 20:32:16, Hixie wrote: > children should never create their parent's parentdata. Acknowledged. https://codereview.chromium.org/1151293002/diff/20001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:90: root.add(new RenderSolidColorFlex(0xFFFFFF00, 1)); On 2015/05/22 20:32:16, Hixie wrote: > Have a helper function here which creates the node, adds it to root, then sets > child.parentData.flex. Acknowledged. https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... File sky/sdk/lib/framework/layout2.dart (right): https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:464: bool handlePointer(sky.PointerEvent event, { double x: 0.0, double y: 0.0 }) { On 2015/05/22 20:32:16, Hixie wrote: > This can't be in a class above ContainerRenderNodeMixin in the hierarchy. To > reuse code use a helper function. Acknowledged. https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:754: width = clamp(min: _constraints.minWidth, max: _constraints.maxWidth); On 2015/05/22 20:32:16, Hixie wrote: > width = _constraints.constrainWidth(0.0); Acknowledged. https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:794: double spacePerFlex = freeSpace / totalFlex; On 2015/05/22 20:32:16, Hixie wrote: > check what happens when totalFlex is 0.0 and either way put a comment describing > what happens. Acknowledged. https://codereview.chromium.org/1151293002/diff/20001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:837: RenderBox child = _firstChild; On 2015/05/22 20:32:16, Hixie wrote: > maybe do the same as for handlePointer() (with a helper function used by > BlockBox as well) Acknowledged.
lgtm https://codereview.chromium.org/1151293002/diff/40001/sky/examples/raw/simple... File sky/examples/raw/simple_render_tree.dart (right): https://codereview.chromium.org/1151293002/diff/40001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:70: void addFlexChild(RenderFlex renderFlex, int backgroundColor, { int flex: 0 }) { renderFlex => parent https://codereview.chromium.org/1151293002/diff/40001/sky/examples/raw/simple... sky/examples/raw/simple_render_tree.dart:73: renderFlex.setParentData(child); add() implies setParentData() https://codereview.chromium.org/1151293002/diff/40001/sky/sdk/lib/framework/l... File sky/sdk/lib/framework/layout2.dart (right): https://codereview.chromium.org/1151293002/diff/40001/sky/sdk/lib/framework/l... sky/sdk/lib/framework/layout2.dart:587: ChildType child = _lastChild; _lastChild => lastChild
The CQ bit was checked by jackson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ianh@google.com Link to the patchset: https://codereview.chromium.org/1151293002/#ps60001 (title: "more CR feedback from hixie")
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
Patchset #5 (id:80001) has been deleted
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as 9687376ca32408d564be59f37b13b7d5917d9a73 (presubmit successful).
Message was sent while issue was closed.
sethladd@google.com changed reviewers: + sethladd@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1151293002/diff/100001/sky/examples/raw/simpl... File sky/examples/raw/simple_render_tree.dart (right): https://codereview.chromium.org/1151293002/diff/100001/sky/examples/raw/simpl... sky/examples/raw/simple_render_tree.dart:16: : desiredHeight = desiredHeight, Did you consider using this. in your constructor? It can eliminate some boilerplate. For example: RenderSolidColor(int backgroundColor, {this.desiredHeight: double.INFINITY}) : super(new BoxDecoration(backgroundColor: backgroundColor));
Message was sent while issue was closed.
https://codereview.chromium.org/1151293002/diff/100001/sky/examples/raw/simpl... File sky/examples/raw/simple_render_tree.dart (right): https://codereview.chromium.org/1151293002/diff/100001/sky/examples/raw/simpl... sky/examples/raw/simple_render_tree.dart:16: : desiredHeight = desiredHeight, On 2015/05/23 03:14:10, sethladd wrote: > Did you consider using this. in your constructor? It can eliminate some > boilerplate. > > For example: > > RenderSolidColor(int backgroundColor, {this.desiredHeight: double.INFINITY}) > : super(new BoxDecoration(backgroundColor: backgroundColor)); > Good call, I'll fix that. I'm new to Dart, as you can probably guess. :)
Message was sent while issue was closed.
No worries! Stop by any time, happy to do code reviews (both for micro reviews and macro reviews). And welcome! |
