Specs: Initial hack at extensible style/layout

Specs: Initial hack at extensible style/layout

Committed: https://chromium.googlesource.com/external/mojo/+/0c0edc5df38af2f322eb384a922613a48fd714dc

[ojan, 2014-11-12 05:13:31]

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

[ojan, 2014-11-12 05:13:33]

Seems reasonable to me at a high level. Left a couple minor comments. Will need
to think about this more. Maybe I'll try just building some of this to see what
the problems are.

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md
File sky/specs/style.md (right):

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md#newco...
sky/specs/style.md:147: These can be combined (without whitespace), with at most
one tagname,
Also at most one id?

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md#newco...
sky/specs/style.md:357: 'inline': sky.InlineLayoutManager
What different between paragraph and inline?

We might also want flex and grid. But we can work that out later.

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md#newco...
sky/specs/style.md:376: void setChildSize(child, width, height); // sets
child.width, child.height
I think we want to be able to set x, y, width, height separately. At least,
that's what the C++ code relies on at the moment.

https://codereview.chromium.org/716013002/diff/60001/sky/specs/style.md
File sky/specs/style.md (right):

https://codereview.chromium.org/716013002/diff/60001/sky/specs/style.md#newco...
sky/specs/style.md:394: void assumeDimensions(Float width, Float height);
I'm not really following what this does.

https://codereview.chromium.org/716013002/diff/60001/sky/specs/style.md#newco...
sky/specs/style.md:402: // returns min-width, width, and max-width, normalised
In practice, in CSS layout, there's only intrinsic min and max, no width. Maybe
that's something we want to change though?

It's not clear to me that we want to return all of these from a single getter. I
can imagine cases where this would cause us to do extra unnecessary work if you
only actually wanted one of the values.

https://codereview.chromium.org/716013002/diff/60001/sky/specs/style.md#newco...
sky/specs/style.md:406: // returns min-height, height, and max-height,
normalised
In practice, in CSS layout, there's only an intrinsic height, no intrinsic min
or max. Maybe that's something we want to change?

[Hixie, 2014-11-12 06:10:41]

I'll upload a modified version that addresses these comments.

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md
File sky/specs/style.md (right):

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md#newco...
sky/specs/style.md:147: These can be combined (without whitespace), with at most
one tagname,
On 2014/11/12 05:13:32, ojan wrote:
> Also at most one id?

Well syntactically there's no reason to limit it, but yeah, we should maybe warn
in debug mode.

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md#newco...
sky/specs/style.md:357: 'inline': sky.InlineLayoutManager
On 2014/11/12 05:13:32, ojan wrote:
> What different between paragraph and inline?

'paragraph' is the sky version of CSS 'block' containing inlines.
'inline' is the sky version of CSS 'inline' (doesn't really do anything).

> We might also want flex and grid. But we can work that out later.

Yeah we'll definitely want to have a bunch of these in the frameworks. Really we
only need 'paragraph' and 'inline' in sky:core.

https://codereview.chromium.org/716013002/diff/20001/sky/specs/style.md#newco...
sky/specs/style.md:376: void setChildSize(child, width, height); // sets
child.width, child.height
On 2014/11/12 05:13:32, ojan wrote:
> I think we want to be able to set x, y, width, height separately. At least,
> that's what the C++ code relies on at the moment.

We can do that. What are the cases where code sets x but not y (or vice versa),
or height but not width (or vice versa)?

[Hixie, 2014-11-12 06:20:10]

https://codereview.chromium.org/716013002/diff/60001/sky/specs/style.md
File sky/specs/style.md (right):

https://codereview.chromium.org/716013002/diff/60001/sky/specs/style.md#newco...
sky/specs/style.md:394: void assumeDimensions(Float width, Float height);
On 2014/11/12 05:13:33, ojan wrote:
> I'm not really following what this does.

It sets the value that's passed to the ValueResolver callback called by
getProperty(), for the case where you're trying to e.g. resolve % dimensions
against the parent's width but the parent hasn't actually been fully laid out
yet (and set its own parent hasn't yet called setChildWidth() on it).

https://codereview.chromium.org/716013002/diff/60001/sky/specs/style.md#newco...
sky/specs/style.md:402: // returns min-width, width, and max-width, normalised
On 2014/11/12 05:13:33, ojan wrote:
> In practice, in CSS layout, there's only intrinsic min and max, no width.
Maybe
> that's something we want to change though?

Well the logic here is that this is how an explicit width gets passed up to the
parent. Otherwise 'width: 50px' doesn't work. Think of the case of a table cell
with a min-width, max-width, and explicit width. If it fits, everything will be
the given explicit width. If it doesn't, some will get squeezed (down to the
min-width). If it fits but the table is supposed to be bigger, the extra space
gets distributed amongst the cells, up to their maximum width.

This just provides the hooks you need to implement that.


> It's not clear to me that we want to return all of these from a single getter.
I
> can imagine cases where this would cause us to do extra unnecessary work if
you
> only actually wanted one of the values.

Yeah, maybe. I'm worried about having too many API end points that authors have
to override because the more we have the more they're likely to get them wrong,
forget one, or whatever. I'm especially worried about this here because if
someone writes a library that implements a new layout type, they won't test it
in all the various ways it could be used, and so it's easy for it to break when
used with another layout that's fancier.

I suspect the way to go, though, is to start with this and then if this is a hot
spot on profiles, hammer it down into more endpoints to avoid the redundancy.

[Hixie, 2014-11-13 22:00:53]

Committed patchset #8 (id:140001) manually as
0c0edc5df38af2f322eb384a922613a48fd714dc.