Bug 171225 – [css-grid] Remove most of the usage of SizingOperation

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[css-grid] Remove most of the usage of SizingOperation

[Sergio Villar Senin]

Created attachment 307983 [details]
Patch

[Javier Fernandez]

Comment on attachment 307983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307983&action=review

The change looks good to me, regarding the Grid Layout logic changes.

> Source/WebCore/rendering/RenderGrid.cpp:458
> +    if (!isRowAxis && !availableSize) {

Aren't we missing some cases here ? 
  - isRowAxis
  - !isRowAxis && availableSize

> LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html:28
> +    align-content: start;

nip: we can use the "contentStart" class from the grid-alignment.css resource file

[Sergio Villar Senin]

Comment on attachment 307983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307983&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:458
>> +    if (!isRowAxis && !availableSize) {
> 
> Aren't we missing some cases here ? 
>   - isRowAxis
>   - !isRowAxis && availableSize

No because we're now passing the availableSize as argument, we have passed the responsibility to the caller of the function.

>> LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html:28
>> +    align-content: start;
> 
> nip: we can use the "contentStart" class from the grid-alignment.css resource file

Right, I'll change it.

[Sergio Villar Senin]

Created attachment 308085 [details]
Patch

[Build Bot]

Comment on attachment 308085 [details]
Patch

Attachment 308085 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3601103

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html

[Build Bot]

Created attachment 308096 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Comment on attachment 308085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308085&action=review

> Source/WebCore/rendering/RenderGrid.cpp:307
> +    bool isRowAxis = direction == ForColumns;
> +    const Length& gap = isRowAxis ? style().gridColumnGap() : style().gridRowGap();
> +
> +    std::optional<LayoutUnit> availableSize;
> +    if (gap.isPercent())
> +        availableSize = isRowAxis ? availableLogicalWidth() : availableLogicalHeightForPercentageComputation();

Why not call availableSpaceForGutters() here? This code is duplicated now.

[Sergio Villar Senin]

Comment on attachment 308085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308085&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:307
>> +        availableSize = isRowAxis ? availableLogicalWidth() : availableLogicalHeightForPercentageComputation();
> 
> Why not call availableSpaceForGutters() here? This code is duplicated now.

Right, I'll change it.

[Sergio Villar Senin]

Created attachment 308246 [details]
Patch

[Sergio Villar Senin]

Committed r215800: <http://trac.webkit.org/changeset/215800>