[css-grid] Use the margin box for non auto minimum sizes

[css-grid] Use the margin box for non auto minimum sizes

When computing the min-size of items with non-auto minimum height/width we
are incorrectly returning the size of the border box, and thus incorrectly
ignoring the margins of the item.

This is a follow up patch of refs/heads/master@{#383944} were we added the
missing border and paddings for heights. Contrary to that, we were not
including margins for both axis.

This CL requires 3 different interrelated changes:
* Add the margins to the min-size returned by minSizeForChild (might require
a layout)
* Refactor and extract width computations from logicalHeightForChild(); not
totally mandatory but pretty logical and helpful
* Use a new update function to isolate the computation of the override width

Committed: https://crrev.com/6838c4a3bbe2ab48aeb84f54d1ca388425f83f82
Cr-Commit-Position: refs/heads/master@{#385697}

[svillar, 2016-04-01 09:57:32]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, rego@igalia.com,
rune@opera.com

[jfernandez, 2016-04-01 11:00:46]

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:757: bool
overrideWidthHasChanged =
updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData);
Why we can't call this function inside logicalHeightForChild instead of using
the new extra argument ?

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1588: return
LayoutUnit(infinity);
Could you explain this change ?

[jfernandez, 2016-04-01 12:16:58]

After discussing privately all the issues I can give my non-owner LGTM.

[jfernandez, 2016-04-01 13:40:31]

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:151: bool
updateOverrideContainingBlockContentLogicalWidthForChild(LayoutBox&,
GridSizingData&);
Shouldn't we use const for the GridSizingData argument ?

[jfernandez, 2016-04-04 15:23:24]

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:683: LayoutUnit
LayoutGrid::logicalHeightForChild(LayoutBox& child, GridSizingData& sizingData,
bool overrideWidthHasChanged)
We shouldn't need this new argument. We only use to know whether we have to mark
the child for layout. We can do this before calling to logicalHeightForChild.

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:722:
ASSERT(child.hasOverrideContainingBlockLogicalWidth());
Why we need this ASSERT ? 
It's already part overrideContainingBlockContentLogicalWidth() function, so I
don't think it's useful. 

I think it's also weird this way of updating the
OverrideContianerBlockLogicalWidth and mark the child for layout. We are doing
the same inside minContentFrChild + logicalHeightForChild. 

I think we should look for a different way of doing this, verifying only once
whether the overrideWidth ahs changed or not and setting the child for layout.
Then, we will call layoutIfNeeded in the specific functions.

[svillar, 2016-04-05 08:34:30]

Thanks for the review.

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:683: LayoutUnit
LayoutGrid::logicalHeightForChild(LayoutBox& child, GridSizingData& sizingData,
bool overrideWidthHasChanged)
On 2016/04/04 15:23:24, jfernandez wrote:
> We shouldn't need this new argument. We only use to know whether we have to
mark
> the child for layout. We can do this before calling to logicalHeightForChild.

Acknowledged.

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:757: bool
overrideWidthHasChanged =
updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData);
On 2016/04/01 11:00:46, jfernandez wrote:
> Why we can't call this function inside logicalHeightForChild instead of using
> the new extra argument ? 

Because it's pretty weird that a function that returns the logicalHeight does
modify the width as a side effect. This means that you cannot safely ask for the
height of an element without the risk of the width being modified.

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1588: return
LayoutUnit(infinity);
On 2016/04/01 11:00:46, jfernandez wrote:
> Could you explain this change ? 

Sure. This function can now be called just after initializing the tracks' base
and growthLimit sizes. Infinity is represented as -1, so we cannot safely just
sum the sizes of the tracks because infinities will reduce the breadth size
instead of considering that we have an unlimited room.

[svillar, 2016-04-05 08:42:34]

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1854623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1588: return
LayoutUnit(infinity);
On 2016/04/01 11:00:46, jfernandez wrote:
> Could you explain this change ? 

Now that I think about it, base sizes are never initialized to 0, so we should
be safe without that change.

[svillar, 2016-04-05 09:00:40]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-04-05 09:00:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/1854623002/#ps20001
(title: "Javi's review changes")

[commit-bot: I haz the power, 2016-04-05 09:00:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1854623002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1854623002/20001

[commit-bot: I haz the power, 2016-04-05 10:49:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-05 10:49:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Manuel Rego, 2016-04-05 12:41:23]

I guess you should update the description before landing.

At least this part seems inaccurate now:
"* gridAreaBreadthForChild() needs to deal with infinities since it could be
now called before they are actually resolved"

[svillar, 2016-04-05 15:53:26]

Description was changed from

==========
[css-grid] Use the margin box for non auto minimum sizes

When computing the min-size of items with non-auto minimum height/width we
are incorrectly returning the size of the border box, and thus incorrectly
ignoring the margins of the item.

This is a follow up patch of refs/heads/master@{#383944} were we added the
missing border and paddings for heights. Contrary to that, we were not
including margins for both axis.

This CL requires 3 different interrelated changes:
* Add the margins to the min-size returned by minSizeForChild (might require
a layout)
* Refactor and extract width computations from logicalHeightForChild(); not
totally mandatory but pretty logical and helpful
* gridAreaBreadthForChild() needs to deal with infinities since it could be
now called before they are actually resolved
==========

to

==========
[css-grid] Use the margin box for non auto minimum sizes

When computing the min-size of items with non-auto minimum height/width we
are incorrectly returning the size of the border box, and thus incorrectly
ignoring the margins of the item.

This is a follow up patch of refs/heads/master@{#383944} were we added the
missing border and paddings for heights. Contrary to that, we were not
including margins for both axis.

This CL requires 3 different interrelated changes:
* Add the margins to the min-size returned by minSizeForChild (might require
a layout)
* Refactor and extract width computations from logicalHeightForChild(); not
totally mandatory but pretty logical and helpful
* Use a new update function to isolate the computation of the override width
==========

[svillar, 2016-04-06 11:08:28]

Although the bots are happy, we should be using
marginIntrinsicLogicalWidthForChild() when computing intrinsic sizes. I need to
rework this patch.

[jfernandez, 2016-04-06 11:10:27]

As agreed, we will need a new approach for this, so not lgtm.

[jfernandez, 2016-04-06 12:22:23]

The new approach is definitively better, much clearer and I think is the way to
address this issue. I've got a couple of questions about how we set grid items
for layout now, comparing to the previous code.

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:693: if
(shouldClearContainingBlockLogicalHeight)
We only use the  TreeScope for the setNeedsForLayout in case of having to clear
the containing block's logical height.  In the old code, we were doing it as
well when the containing block's logical width had changed. 

Now we deal with containing block's logical width changes outsize this function,
which makes sense, indeed. However, we need to be sure that not using the scope
in the cases were we mark the child for layout before computing its logical
height is equivalent.

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:764:
child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
Shouldn't we use the SubtreeLayoutScope here ?

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:787:
child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
Shouldn't we use the SubtreeLayoutScope here ?

[svillar, 2016-04-06 14:55:10]

Thanks for the review.

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:693: if
(shouldClearContainingBlockLogicalHeight)
On 2016/04/06 12:22:23, jfernandez wrote:
> We only use the  TreeScope for the setNeedsForLayout in case of having to
clear
> the containing block's logical height.  In the old code, we were doing it as
> well when the containing block's logical width had changed. 
> 
> Now we deal with containing block's logical width changes outsize this
function,
> which makes sense, indeed. However, we need to be sure that not using the
scope
> in the cases were we mark the child for layout before computing its logical
> height is equivalent.

As you can see in the SubtreeLayoutScope, that class is a helper to ensure that
once a subtree is marked for layout, we do indeed lay it out. As you can see, in
this patch the setNeedsLayout() we add is followed by a layoutIfNeeded() in one
case and by a logicalHeightForChild() in the other (which internally calls
layoutIfNeeded()). That's why I think we're safe here.

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:764:
child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
On 2016/04/06 12:22:23, jfernandez wrote:
> Shouldn't we use the SubtreeLayoutScope here ?

We know that logicalHeightForChild() is going to call layoutIfNeeded(), but I
can change it so that it will assert if we ever remove that from the
logicalHeightForChild() method.

[jfernandez, 2016-04-07 07:49:39]

LGTM, with the suggestions about the SubtreeLayoutSCope to be considered before
landing.

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:693: if
(shouldClearContainingBlockLogicalHeight)
On 2016/04/06 14:55:09, svillar wrote:
> On 2016/04/06 12:22:23, jfernandez wrote:
> > We only use the  TreeScope for the setNeedsForLayout in case of having to
> clear
> > the containing block's logical height.  In the old code, we were doing it as
> > well when the containing block's logical width had changed. 
> > 
> > Now we deal with containing block's logical width changes outsize this
> function,
> > which makes sense, indeed. However, we need to be sure that not using the
> scope
> > in the cases were we mark the child for layout before computing its logical
> > height is equivalent.
> 
> As you can see in the SubtreeLayoutScope, that class is a helper to ensure
that
> once a subtree is marked for layout, we do indeed lay it out. As you can see,
in
> this patch the setNeedsLayout() we add is followed by a layoutIfNeeded() in
one
> case and by a logicalHeightForChild() in the other (which internally calls
> layoutIfNeeded()). That's why I think we're safe here.

Yes, you're right. The change is equivalent to the original code.

https://codereview.chromium.org/1854623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:764:
child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
On 2016/04/06 14:55:09, svillar wrote:
> On 2016/04/06 12:22:23, jfernandez wrote:
> > Shouldn't we use the SubtreeLayoutScope here ?
> 
> We know that logicalHeightForChild() is going to call layoutIfNeeded(), but I
> can change it so that it will assert if we ever remove that from the
> logicalHeightForChild() method.

The SubtreeLAyoutScope approach is used in other places of the grid layout
logic,
so perhaps we can still use it here.

[svillar, 2016-04-07 08:18:53]

Ensure that layout() is called in min|maxContentForChild

[svillar, 2016-04-07 08:20:58]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-04-07 08:20:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/1854623002/#ps120001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-04-07 08:21:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1854623002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1854623002/120001

[commit-bot: I haz the power, 2016-04-07 09:24:56]

Description was changed from

==========
[css-grid] Use the margin box for non auto minimum sizes

When computing the min-size of items with non-auto minimum height/width we
are incorrectly returning the size of the border box, and thus incorrectly
ignoring the margins of the item.

This is a follow up patch of refs/heads/master@{#383944} were we added the
missing border and paddings for heights. Contrary to that, we were not
including margins for both axis.

This CL requires 3 different interrelated changes:
* Add the margins to the min-size returned by minSizeForChild (might require
a layout)
* Refactor and extract width computations from logicalHeightForChild(); not
totally mandatory but pretty logical and helpful
* Use a new update function to isolate the computation of the override width
==========

to

==========
[css-grid] Use the margin box for non auto minimum sizes

When computing the min-size of items with non-auto minimum height/width we
are incorrectly returning the size of the border box, and thus incorrectly
ignoring the margins of the item.

This is a follow up patch of refs/heads/master@{#383944} were we added the
missing border and paddings for heights. Contrary to that, we were not
including margins for both axis.

This CL requires 3 different interrelated changes:
* Add the margins to the min-size returned by minSizeForChild (might require
a layout)
* Refactor and extract width computations from logicalHeightForChild(); not
totally mandatory but pretty logical and helpful
* Use a new update function to isolate the computation of the override width
==========

[commit-bot: I haz the power, 2016-04-07 09:24:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-07 09:26:46]

Description was changed from

==========
[css-grid] Use the margin box for non auto minimum sizes

When computing the min-size of items with non-auto minimum height/width we
are incorrectly returning the size of the border box, and thus incorrectly
ignoring the margins of the item.

This is a follow up patch of refs/heads/master@{#383944} were we added the
missing border and paddings for heights. Contrary to that, we were not
including margins for both axis.

This CL requires 3 different interrelated changes:
* Add the margins to the min-size returned by minSizeForChild (might require
a layout)
* Refactor and extract width computations from logicalHeightForChild(); not
totally mandatory but pretty logical and helpful
* Use a new update function to isolate the computation of the override width
==========

to

==========
[css-grid] Use the margin box for non auto minimum sizes

When computing the min-size of items with non-auto minimum height/width we
are incorrectly returning the size of the border box, and thus incorrectly
ignoring the margins of the item.

This is a follow up patch of refs/heads/master@{#383944} were we added the
missing border and paddings for heights. Contrary to that, we were not
including margins for both axis.

This CL requires 3 different interrelated changes:
* Add the margins to the min-size returned by minSizeForChild (might require
a layout)
* Refactor and extract width computations from logicalHeightForChild(); not
totally mandatory but pretty logical and helpful
* Use a new update function to isolate the computation of the override width

Committed: https://crrev.com/6838c4a3bbe2ab48aeb84f54d1ca388425f83f82
Cr-Commit-Position: refs/heads/master@{#385697}
==========

[commit-bot: I haz the power, 2016-04-07 09:26:47]

Patchset 7 (id:??) landed as
https://crrev.com/6838c4a3bbe2ab48aeb84f54d1ca388425f83f82
Cr-Commit-Position: refs/heads/master@{#385697}