Bug 171225 - [css-grid] Remove most of the usage of SizingOperation
Summary: [css-grid] Remove most of the usage of SizingOperation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-24 09:25 PDT by Sergio Villar Senin
Modified: 2017-04-26 08:34 PDT (History)
9 users (show)

See Also:


Attachments
Patch (38.09 KB, patch)
2017-04-24 09:30 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (38.05 KB, patch)
2017-04-25 03:43 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (12.77 MB, application/zip)
2017-04-25 05:52 PDT, Build Bot
no flags Details
Patch (37.81 KB, patch)
2017-04-26 08:07 PDT, Sergio Villar Senin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2017-04-24 09:25:02 PDT
[css-grid] Remove most of the usage of SizingOperation
Comment 1 Sergio Villar Senin 2017-04-24 09:30:44 PDT
Created attachment 307983 [details]
Patch
Comment 2 Javier Fernandez 2017-04-25 02:09:17 PDT
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
Comment 3 Sergio Villar Senin 2017-04-25 03:40:05 PDT
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.
Comment 4 Sergio Villar Senin 2017-04-25 03:43:15 PDT
Created attachment 308085 [details]
Patch
Comment 5 Build Bot 2017-04-25 05:52:22 PDT
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
Comment 6 Build Bot 2017-04-25 05:52:24 PDT
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
Comment 7 Manuel Rego Casasnovas 2017-04-25 07:51:41 PDT
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.
Comment 8 Sergio Villar Senin 2017-04-26 07:37:12 PDT
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.
Comment 9 Sergio Villar Senin 2017-04-26 08:07:09 PDT
Created attachment 308246 [details]
Patch
Comment 10 Sergio Villar Senin 2017-04-26 08:34:49 PDT
Committed r215800: <http://trac.webkit.org/changeset/215800>