Fill out some more documentation about building RenderBox subclasses.

Fill out some more documentation about building RenderBox subclasses.

TBR=collin,hansmuller

Committed: https://chromium.googlesource.com/external/mojo/+/71f0ecc7a2a2fc3d8c2ed6adcda17e05300f6439

[hansmuller, 2015-06-24 19:53:29]

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

[hansmuller, 2015-06-24 19:53:31]

My notes about whatever puzzled me.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
File sky/sdk/lib/rendering/README.md (right):

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:126: * If its constructor takes any nodes to be
used as children, the
I wasn't sure what "nodes" were here. It looks like they are
RenderObject "ChildType" subclasses per mixins like
ContainerRenderObjectMixin<ChildType extends RenderObject, ...>

Do children need to be RenderObjects or RenderBoxes or can they be any
AbstractNode subclass?

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:129: * If it has any data to store on its
children, define a BoxParentData
"it" is the the RenderBox subclass

The setParentData() method below implies that each child must be a RenderBox?

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:137: void setParentData(RenderBox child) {
I found this method name confusing; I'd expected the parameters to be (RenderBox
child, ParentData data). Maybe it should be called initParentData()?

Who's responsible for calling setParentData() on each child?

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:149: positions for each child.
It also causes each child's size to be set?

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:151: ** The algorithm can decide the Size in one
of two ways: either
It would be nice to explain why RenderBoxes make the sizedByParent distinction.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:157: returns true, and it must have a
`performResize()` method that uses
I'm not entirely clear on what a performResize() override needs to do. Not clear
what the name is trying to convey either.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:170: return the width that fits within the given
constraints below which
This is somewhat hard to parse. Maybe you could substitute "constrained width"
for "the width that fits within the given constraints". Using horizontal text as
an example might clarify this; like the minimum intrinsic width of "a b cd e"
would be the width of "cd" ...

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:186: out within itself.
This constraint might also be explained with a text layout example. I assume the
min intrinsic height of "a b cd e" is the line height.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:196: * The box must have a `performLayout()`
method that walks over the
A slightly simplified version of RenderBlock's performLayout might help clarify
this section. For example it would clarify what you mean by "walk over the
object's children".

Also: I think it would helpful to start this section by saying that the
performLayout() is responsible for computing the size and position of each child
and setting its own size.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:197: object's children, if any, and for each one
calls `child.layout()`
It would be helpful to note here that child.layout() causes the child's
performLayout() method to run, after which child.size is the child's preferred
(?) size. I assume that the box is free to reset the child's size?

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:205: the object (by setting `size`), otherwise,
the size must be left
...must also size the child?

If sizedByParent is false then the parent must set the child's size?

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:208: * The `size` member must never be set to an
infinite value.
At some point I think we're going to need to explain how size units relate to
pixels and font dimensions.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
File sky/sdk/lib/rendering/block.dart (right):

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
sky/sdk/lib/rendering/block.dart:23: add(child);
https://www.dartlang.org/articles/style-guide/#do-use-curly-braces-for-all-fl...

[hansmuller, 2015-06-24 20:30:14]

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
File sky/sdk/lib/rendering/block.dart (right):

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
sky/sdk/lib/rendering/block.dart:22: for (RenderBox child in children)
Maybe just define addAll(children) in ContainerRenderObjectMixin and have it do
all of this, even the null check?

[hansmuller, 2015-06-24 20:32:50]

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
File sky/sdk/lib/rendering/block.dart (right):

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
sky/sdk/lib/rendering/block.dart:26: void setParentData(RenderBox child) {
Could setParentData() be defined in RenderBoxContainerDefaultsMixin?

[Hixie, 2015-06-25 19:15:32]

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
File sky/sdk/lib/rendering/README.md (right):

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:126: * If its constructor takes any nodes to be
used as children, the
done

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:129: * If it has any data to store on its
children, define a BoxParentData
On 2015/06/24 at 19:53:30, hansmuller wrote:
> "it" is the the RenderBox subclass
> 
> The setParentData() method below implies that each child must be a RenderBox?

Fixed the sample code, clarified the text.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:137: void setParentData(RenderBox child) {
On 2015/06/24 at 19:53:30, hansmuller wrote:
> I found this method name confusing; I'd expected the parameters to be
(RenderBox child, ParentData data). Maybe it should be called initParentData()?
> 
> Who's responsible for calling setParentData() on each child?

Added a paragraph about this. In general parentData isn't well documented yet.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:149: positions for each child.
On 2015/06/24 at 19:53:31, hansmuller wrote:
> It also causes each child's size to be set?

elaborated.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:151: ** The algorithm can decide the Size in one
of two ways: either
On 2015/06/24 at 19:53:30, hansmuller wrote:
> It would be nice to explain why RenderBoxes make the sizedByParent
distinction.

done.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:157: returns true, and it must have a
`performResize()` method that uses
On 2015/06/24 at 19:53:30, hansmuller wrote:
> I'm not entirely clear on what a performResize() override needs to do. Not
clear what the name is trying to convey either.

I don't understand. Doesn't this paragraph exactly describe what it needs to do?
I don't know how to make it clearer...

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:170: return the width that fits within the given
constraints below which
On 2015/06/24 at 19:53:30, hansmuller wrote:
> This is somewhat hard to parse. Maybe you could substitute "constrained width"
for "the width that fits within the given constraints". Using horizontal text as
an example might clarify this; like the minimum intrinsic width of "a b cd e"
would be the width of "cd" ...

Added example. "constrained width" doesn't say constrained by what.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:186: out within itself.
On 2015/06/24 at 19:53:31, hansmuller wrote:
> This constraint might also be explained with a text layout example. I assume
the min intrinsic height of "a b cd e" is the line height.

Added example.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:196: * The box must have a `performLayout()`
method that walks over the
On 2015/06/24 at 19:53:30, hansmuller wrote:
> A slightly simplified version of RenderBlock's performLayout might help
clarify this section. For example it would clarify what you mean by "walk over
the object's children".

I've added a TODO to document how to walk over a ContainerRenderObjectMixin
subclass' children. In general I don't know how to clarify it further, though.
It depends entirely on how the class represents its children.

> Also: I think it would helpful to start this section by saying that the
performLayout() is responsible for computing the size and position of each child
and setting its own size.

I've added text explaining the responsibilities.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:197: object's children, if any, and for each one
calls `child.layout()`
On 2015/06/24 at 19:53:30, hansmuller wrote:
> It would be helpful to note here that child.layout() causes the child's
performLayout() method to run, after which child.size is the child's preferred
(?) size. I assume that the box is free to reset the child's size?

I've clarified that layout() sometimes results in performLayout() being called
recursively on the child.

I've clarified that the parent must not set the child's size.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:205: the object (by setting `size`), otherwise,
the size must be left
On 2015/06/24 at 19:53:30, hansmuller wrote:
> ...must also size the child?
> 
> If sizedByParent is false then the parent must set the child's size?

This is about the parent's size, not the child's size. I've tried to clarify it.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/READM...
sky/sdk/lib/rendering/README.md:208: * The `size` member must never be set to an
infinite value.
On 2015/06/24 at 19:53:30, hansmuller wrote:
> At some point I think we're going to need to explain how size units relate to
pixels and font dimensions.

Added.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
File sky/sdk/lib/rendering/block.dart (right):

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
sky/sdk/lib/rendering/block.dart:22: for (RenderBox child in children)
On 2015/06/24 at 20:30:14, hansmuller wrote:
> Maybe just define addAll(children) in ContainerRenderObjectMixin and have it
do all of this, even the null check?

Good idea. Done.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
sky/sdk/lib/rendering/block.dart:23: add(child);
On 2015/06/24 at 19:53:31, hansmuller wrote:
>
https://www.dartlang.org/articles/style-guide/#do-use-curly-braces-for-all-fl...

I updated our style guide to disagree with this. Adding extra braces where
they're not needed harms readability. There's no dangling-else issue here.

https://codereview.chromium.org/1211573003/diff/1/sky/sdk/lib/rendering/block...
sky/sdk/lib/rendering/block.dart:26: void setParentData(RenderBox child) {
On 2015/06/24 at 20:32:49, hansmuller wrote:
> Could setParentData() be defined in RenderBoxContainerDefaultsMixin?

Sadly not. You can't call "new" on a generic type argument for some reason. If
you could, it'd be in the Container mixin. (The Defaults mixin is just for some
helper functions, it shouldn't override actual methods.)

[Hixie, 2015-06-25 19:25:48]

Committed patchset #3 (id:40001) manually as
71f0ecc7a2a2fc3d8c2ed6adcda17e05300f6439 (presubmit successful).

[abarth-chromium, 2015-06-25 19:41:05]

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

[abarth-chromium, 2015-06-25 19:41:06]

https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
File sky/sdk/lib/rendering/README.md (right):

https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
sky/sdk/lib/rendering/README.md:140: system, then you have to inherit straight
from `RenderObject`.
s/have to inherit straight/should inherit directly/

In theory, you could imagine another class in between that did something else
useful, hence "should" rather than "have to".

https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
sky/sdk/lib/rendering/README.md:143:
example](../../../examples/rendering/sector_layout.dart), which
We should move the examples into the sdk directory (apparently they're supposed
to be in a directory named |example| rather than |examples|).  Otherwise links
like this will break when deployed to sky_sdk.

https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
sky/sdk/lib/rendering/README.md:162: subclass must implement operator== (and
hashCode).
`operator==` and `hashCode` because these are code terms.

https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
sky/sdk/lib/rendering/README.md:255: re-laid-out.
I wonder if we should remove this concept until we have data that it matters for
performance.

[Hixie, 2015-06-25 22:47:56]

On 2015/06/25 at 19:41:06, abarth wrote:
>
https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
> File sky/sdk/lib/rendering/README.md (right):
> 
>
https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
> sky/sdk/lib/rendering/README.md:140: system, then you have to inherit straight
from `RenderObject`.
> s/have to inherit straight/should inherit directly/
> 
> In theory, you could imagine another class in between that did something else
useful, hence "should" rather than "have to".

Done.


>
https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
> sky/sdk/lib/rendering/README.md:143:
example](../../../examples/rendering/sector_layout.dart), which
> We should move the examples into the sdk directory (apparently they're
supposed to be in a directory named |example| rather than |examples|). 
Otherwise links like this will break when deployed to sky_sdk.

Filed https://github.com/domokit/mojo/issues/277


>
https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
> sky/sdk/lib/rendering/README.md:162: subclass must implement operator== (and
hashCode).
> `operator==` and `hashCode` because these are code terms.

Done.


>
https://codereview.chromium.org/1211573003/diff/40001/sky/sdk/lib/rendering/R...
> sky/sdk/lib/rendering/README.md:255: re-laid-out.
> I wonder if we should remove this concept until we have data that it matters
for performance.

Added to my stack. Blocked on 
https://codereview.chromium.org/1213473003